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

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