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

[Han Han]

Thank u for reviewing. I will give patch v2 later.

On Tue, Jul 10, 2018 at 2:38 PM, Michal Privoznik <mprivozn at redhat.com>
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 at redhat.com>
> > ---
> >  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 at redhat.com
Phone: +861065339333
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180710/70ba9285/attachment-0001.htm>