<div dir="ltr">Thank u for reviewing. I will give patch v2 later. </div><div class="gmail_extra"> <div class="gmail_quote">On Tue, Jul 10, 2018 at 2:38 PM, Michal Privoznik <<a href="mailto:mprivozn@redhat.com" target="_blank">mprivozn@redhat.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">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 <<a href="mailto:hhan@redhat.com">hhan@redhat.com</a>> > --- > 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) </div></div>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. <div><div class="h5"> > + 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, > +}; </div></div>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 </blockquote></div> -- <div class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr">Best regards,</div><div dir="ltr">----------------------------------- </div><div dir="ltr">Han Han Quality Engineer Redhat. Email: <a href="mailto:hhan@redhat.com" target="_blank">hhan@redhat.com</a> Phone: +861065339333 </div></div></div></div></div></div> </div>