[libvirt] [PATCH v4 1/2] util: Refactor virFileIsCDROM to virFileCheckCDROM

[John Ferlan]

On 10/25/18 11:39 PM, Han Han wrote:
> Refactor virFileIsCDROM to virFileCheckCDROM for checking cdrom status.
> Add add enum type virFileCDRomStatus.
> 
> Now virFileCheckCDROM could be used to check the cdrom drive status such
> as ok, no disc, tray open, drive not ready, or unknown.
> 
> Signed-off-by: Han Han <hhan at redhat.com>
> ---
>  src/libvirt_private.syms |  2 +-
>  src/qemu/qemu_domain.c   |  4 ++--
>  src/util/virfile.c       | 36 +++++++++++++++++++++++++++++++-----
>  src/util/virfile.h       | 12 +++++++++++-
>  4 files changed, 45 insertions(+), 9 deletions(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 335210c31d..c61b613325 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1802,6 +1802,7 @@ virFileActivateDirOverride;
>  virFileBindMountDevice;
>  virFileBuildPath;
>  virFileCanonicalizePath;
> +virFileCheckCDROM;
>  virFileChownFiles;
>  virFileClose;
>  virFileComparePaths;
> @@ -1824,7 +1825,6 @@ virFileGetMountSubtree;
>  virFileHasSuffix;
>  virFileInData;
>  virFileIsAbsPath;
> -virFileIsCDROM;
>  virFileIsDir;
>  virFileIsExecutable;
>  virFileIsLink;
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index ba3fff607a..f34243d2b2 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -7806,7 +7806,7 @@ void qemuDomainObjCheckDiskTaint(virQEMUDriverPtr driver,
>  
>      if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM &&
>          virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_BLOCK &&
> -        disk->src->path && virFileIsCDROM(disk->src->path) == 1)
> +        disk->src->path && virFileCheckCDROM(disk->src->path, NULL) == 1)
>          qemuDomainObjTaint(driver, obj, VIR_DOMAIN_TAINT_CDROM_PASSTHROUGH,
>                             logCtxt);
>  
> @@ -8760,7 +8760,7 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
>          if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM &&
>              src->format == VIR_STORAGE_FILE_RAW &&
>              virStorageSourceIsBlockLocal(src) &&
> -            virFileIsCDROM(src->path) == 1)
> +            virFileCheckCDROM(src->path, NULL) == 1)
>              src->hostcdrom = true;
>  
>          ret = 0;
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index f6f9e4ceda..04b4c274dd 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -1957,18 +1957,22 @@ int virFileIsMountPoint(const char *file)
>  
>  #if defined(__linux__)
>  /**
> - * virFileIsCDROM:
> + * virFileCheckCDROM:
>   * @path: File to check
> + * @cd_status: Filled with the status of the CDROM if non-NULL. See
> + * virFileCDRomStatus.
>   *
>   * Returns 1 if @path is a cdrom device 0 if it is not a cdrom and -1 on
>   * error. 'errno' of the failure is preserved and no libvirt errors are
>   * reported.

Again @errno is not preserved, I'll replace that last sentence with:

If is up to the caller to use @cd_status in order to generate any error
to be reported (if desired).

>   */
>  int
> -virFileIsCDROM(const char *path)
> +virFileCheckCDROM(const char *path,
> +                  virFileCDRomStatus *cd_status)
>  {
>      struct stat st;
>      VIR_AUTOCLOSE fd = -1;
> +    int status;
>  
>      if ((fd = open(path, O_RDONLY | O_NONBLOCK)) < 0)
>          return -1;
> @@ -1980,16 +1984,38 @@ virFileIsCDROM(const char *path)
>          return 0;
>  
>      /* Attempt to detect via a CDROM specific ioctl */
> -    if (ioctl(fd, CDROM_DRIVE_STATUS, CDSL_CURRENT) >= 0)
> +    if ((status = ioctl(fd, CDROM_DRIVE_STATUS, CDSL_CURRENT)) < 0)
> +        return 0;
> +
> +    if (!cd_status)
>          return 1;
>  
> -    return 0;
> +    switch (status) {
> +        case CDS_NO_INFO:
> +            *cd_status = VIR_FILE_CDROM_UNKNOWN;
> +            break;

I think perhaps this should be last *and* should included "default:"
(just in case).

> +        case CDS_NO_DISC:
> +            *cd_status = VIR_FILE_CDROM_NO_DISC;
> +            break;
> +        case CDS_TRAY_OPEN:
> +            *cd_status = VIR_FILE_CDROM_TRAY_OPEN;
> +            break;
> +        case CDS_DRIVE_NOT_READY:
> +            *cd_status = VIR_FILE_CDROM_DRIVE_NOT_READY;
> +            break;
> +        case CDS_DISC_OK:
> +            *cd_status = VIR_FILE_CDROM_DISC_OK;
> +            break;
> +    }
> +
> +    return 1;
>  }
>  
>  #else
>  
>  int
> -virFileIsCDROM(const char *path)
> +virFileCheckCDROM(const char *path,
> +                  virFileCDRomStatus *cd_status ATTRIBUTE_UNUSED)
>  {

Like I noted previously you could have :

    if (cd_status)
        *cd_status = VIR_FILE_CDROM_DISK_OK;

>      if (STRPREFIX(path, "/dev/cd") ||
>          STRPREFIX(path, "/dev/acd"))

Then before the return 0 added a:

    if (cd_status)
        *cd_status = VIR_FILE_CDROM_NO_DEVICE;

something new to add to your list below which could be handled by an
error message...

> diff --git a/src/util/virfile.h b/src/util/virfile.h
> index 0f7dece958..1e5127badb 100644
> --- a/src/util/virfile.h
> +++ b/src/util/virfile.h
> @@ -224,7 +224,17 @@ enum {
>  int virFileIsSharedFSType(const char *path, int fstypes) ATTRIBUTE_NONNULL(1);
>  int virFileIsSharedFS(const char *path) ATTRIBUTE_NONNULL(1);
>  int virFileIsMountPoint(const char *file) ATTRIBUTE_NONNULL(1);
> -int virFileIsCDROM(const char *path)
> +
> +typedef enum {
> +    VIR_FILE_CDROM_DISC_OK,
> +    VIR_FILE_CDROM_UNKNOWN,

Order swap... Typically UNKNOWN is first... I can do that for you.

> +    VIR_FILE_CDROM_NO_DISC,
> +    VIR_FILE_CDROM_TRAY_OPEN,
> +    VIR_FILE_CDROM_DRIVE_NOT_READY,

     VIR_FILE_CDROM_NO_DEVICE,

I could make these changes for you, but in thinking about this, I'm not
sure the check added in patch2 is in the proper place.

John

> +} virFileCDRomStatus;
> +
> +int virFileCheckCDROM(const char *path,
> +                      virFileCDRomStatus *cd_status)
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
>  
>  int virFileGetMountSubtree(const char *mtabpath,
>