Sorry, it should be virFileCdromStatus not virFileCdromState :) On Tue, Jul 10, 2018 at 9:28 PM, Han Han <[email protected]> wrote:
> 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 <[email protected]> wrote: > >> Thank u for reviewing. I will give patch v2 later. >> >> On Tue, Jul 10, 2018 at 2:38 PM, Michal Privoznik <[email protected]> >> 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 <[email protected]> >>> > --- >>> > 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: [email protected] >> Phone: +861065339333 >> > > > > -- > Best regards, > ----------------------------------- > Han Han > Quality Engineer > Redhat. > > Email: [email protected] > Phone: +861065339333 > -- Best regards, ----------------------------------- Han Han Quality Engineer Redhat. Email: [email protected] Phone: +861065339333
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
