Re: [libvirt] [PATCH 1/2] util: Introduce virFileCdromStatus

Hello Michel, I agree with you that we could integrate these two APIs into one. If so, I think we should change the API name virFileIsCDROM to **virFileCheckCDROM** or **virFileCdromState**. virFileIsCDROM is confusing and doesn't indicate the function of checking state. On Tue, Jul 10, 2018 at 3:05 PM, Han Han <hhan(a)redhat.com&gt; wrote: > Thank u for reviewing. I will give patch v2 later. > > On Tue, Jul 10, 2018 at 2:38 PM, Michal Privoznik <mprivozn(a)redhat.com&gt; > wrote: > >> On 07/03/2018 04:29 AM, Han Han wrote: >> > This private API helps check cdrom drive status via ioctl(). >> > >> > Signed-off-by: Han Han <hhan(a)redhat.com&gt; >> > --- >> > src/libvirt_private.syms | 1 + >> > src/util/virfile.c | 52 ++++++++++++++++++++++++++++++ >> ++++++++++ >> > src/util/virfile.h | 10 ++++++++ >> > 3 files changed, 63 insertions(+) >> > >> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms >> > index 5499a368c0..e9f79ad433 100644 >> > --- a/src/libvirt_private.syms >> > +++ b/src/libvirt_private.syms >> > @@ -1797,6 +1797,7 @@ virFileActivateDirOverride; >> > virFileBindMountDevice; >> > virFileBuildPath; >> > virFileCanonicalizePath; >> > +virFileCdromStatus; >> > virFileChownFiles; >> > virFileClose; >> > virFileComparePaths; >> > diff --git a/src/util/virfile.c b/src/util/virfile.c >> > index 378d03ecf0..790d9448d2 100644 >> > --- a/src/util/virfile.c >> > +++ b/src/util/virfile.c >> > @@ -2026,6 +2026,58 @@ virFileIsCDROM(const char *path) >> > return ret; >> > } >> > >> > +/** >> > + * virFileCdromStatus: >> > + * @path: Cdrom path >> > + * >> > + * Returns: >> > + * -1 error happends. >> > + * VIR_FILE_CDROM_DISC_OK Cdrom is OK. >> > + * VIR_FILE_CDROM_NO_INFO Information not available. >> > + * VIR_FILE_CDROM_NO_DISC No disc in cdrom. >> > + * VIR_FILE_CDROM_TREY_OPEN Cdrom is trey-open. >> > + * VIR_FILE_CDROM_DRIVE_NOT_READY Cdrom is not ready. >> > + * reported. >> > + */ >> > +int >> > +virFileCdromStatus(const char *path) >> >> This is in "if defined(__linux__)" block. You need to provide non-Linux >> stub for this function too otherwise we will fail to build on such >> systems. >> >> >> > +{ >> > + int ret = -1; >> > + int fd; >> > + >> > + if ((fd = open(path, O_RDONLY | O_NONBLOCK)) < 0) >> > + goto cleanup; >> > + >> > + /* Attempt to detect CDROM status via ioctl */ >> > + if ((ret = ioctl(fd, CDROM_DRIVE_STATUS, CDSL_CURRENT)) >= 0) { >> >> So virFileIsCDROM calls the same ioctl(). I wonder whether we should >> modify that function instead of inventing a new one. It would work like >> this: >> >> virFileISCDROM(const char *path, int *status); >> >> if @path is a CDROM, then @status is set to one of VIR_FILE_CDROM_*. >> However, caller can pass status = NULL which means they are not >> interested in status rather than plain is CDROM / isn't CDROM fact. >> >> > + switch (ret) { >> > + case CDS_NO_INFO: >> > + ret = VIR_FILE_CDROM_NO_INFO; >> > + goto cleanup; >> > + break; >> >> There is no reason for this goto. break is sufficient. >> >> > + case CDS_NO_DISC: >> > + ret = VIR_FILE_CDROM_NO_DISC; >> > + goto cleanup; >> > + break; >> > + case CDS_TRAY_OPEN: >> > + ret = VIR_FILE_CDROM_TREY_OPEN; >> > + goto cleanup; >> > + break; >> > + case CDS_DRIVE_NOT_READY: >> > + ret = VIR_FILE_CDROM_DRIVE_NOT_READY; >> > + goto cleanup; >> > + break; >> > + case CDS_DISC_OK: >> > + ret = VIR_FILE_CDROM_DISC_OK; >> > + goto cleanup; >> > + break; >> > + } >> > + } >> > + >> > + cleanup: >> > + VIR_FORCE_CLOSE(fd); >> > + return ret; >> > +} >> > #else >> > >> > int >> > diff --git a/src/util/virfile.h b/src/util/virfile.h >> > index 6f1e802fde..9d4701c8d2 100644 >> > --- a/src/util/virfile.h >> > +++ b/src/util/virfile.h >> > @@ -212,6 +212,16 @@ int virFileIsSharedFS(const char *path) >> ATTRIBUTE_NONNULL(1); >> > int virFileIsMountPoint(const char *file) ATTRIBUTE_NONNULL(1); >> > int virFileIsCDROM(const char *path) >> > ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; >> > +int virFileCdromStatus(const char *path) >> > + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; >> > + >> > +enum { >> > + VIR_FILE_CDROM_DISC_OK = 1, >> > + VIR_FILE_CDROM_NO_INFO, >> > + VIR_FILE_CDROM_NO_DISC, >> > + VIR_FILE_CDROM_TREY_OPEN, >> > + VIR_FILE_CDROM_DRIVE_NOT_READY, >> > +}; >> >> Nit pick, the enum should go before the function. The reason is that if >> we decide to give the enum a name [1], we don't have to move things >> around. >> >> 1: which leads me to even better proposal. How about giving this enum a >> name, say virFileCDRomStatus and acknowledging this in function header: >> >> int virFileISCDROM(const char *path, virFileCDRomStatus *status); >> >> The set of return values of the function would remain the same as is now. >> >> Michal >> > > > > -- > Best regards, > ----------------------------------- > Han Han > Quality Engineer > Redhat. > > Email: hhan(a)redhat.com > Phone: +861065339333 > -- Best regards, ----------------------------------- Han Han Quality Engineer Redhat. Email: hhan(a)redhat.com Phone: +861065339333