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

[John Ferlan]

On 07/11/2018 05:44 AM, Han Han wrote:
> Rename virFileIsCDROM to virFileCheckCDROM and add enum type
> virFileCDRomStatus of cdrom statuses.  Add argument cd_status in
> virFileCheckCDROM filled with cdrom status.
> 
> Now virFileCheckCDROM could be used to check the cdrom drive status such
> as no info, no disc, tray open, drive not ready or ok.

s/ or /, or /

> 
> Signed-off-by: Han Han <hhan at redhat.com>
> ---
>  src/libvirt_private.syms |  2 +-
>  src/qemu/qemu_domain.c   |  4 ++--
>  src/util/virfile.c       | 41 ++++++++++++++++++++++++++++++++++------
>  src/util/virfile.h       | 13 ++++++++++++-
>  4 files changed, 50 insertions(+), 10 deletions(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index e688981c3e..ea2452c235 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1789,6 +1789,7 @@ virFileActivateDirOverride;
>  virFileBindMountDevice;
>  virFileBuildPath;
>  virFileCanonicalizePath;
> +virFileCheckCDROM;
>  virFileChownFiles;
>  virFileClose;
>  virFileComparePaths;
> @@ -1811,7 +1812,6 @@ virFileGetMountSubtree;
>  virFileHasSuffix;
>  virFileInData;
>  virFileIsAbsPath;
> -virFileIsCDROM;
>  virFileIsDir;
>  virFileIsExecutable;
>  virFileIsLink;
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index ed76495309..f443eb4d8f 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -7487,7 +7487,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);
>  
> @@ -8394,7 +8394,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 378d03ecf0..869499dc78 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -1990,19 +1990,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.

Technically 'errno' is not preserved - it's lost as soon as something
else that touches is run.  For example, if whatever VIR_FORCE_CLOSE
calls touches errno for some reason, then it's lost.

I would just indicate it's up to the caller to generate the error message.

>   */
>  int
> -virFileIsCDROM(const char *path)
> +virFileCheckCDROM(const char *path,
> +                  virFileCDRomStatus *cd_status)
>  {
>      struct stat st;
>      int fd;
>      int ret = -1;
> +    int status;
>  
>      if ((fd = open(path, O_RDONLY | O_NONBLOCK)) < 0)
>          goto cleanup;
> @@ -2016,10 +2019,35 @@ virFileIsCDROM(const char *path)
>      }
>  
>      /* Attempt to detect via a CDROM specific ioctl */
> -    if (ioctl(fd, CDROM_DRIVE_STATUS, CDSL_CURRENT) >= 0)
> -        ret = 1;
> -    else
> +    status = ioctl(fd, CDROM_DRIVE_STATUS, CDSL_CURRENT);
> +
> +    if (status < 0) {
>          ret = 0;
> +        goto cleanup;
> +    }
> +
> +    ret = 1;
> +
> +    if (!cd_status)
> +        goto cleanup;
> +
> +    switch (status) {
> +        case CDS_NO_INFO:
> +            *cd_status = VIR_FILE_CDROM_NO_INFO;
> +            break;
> +        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;

    default:
        *cd_status = VIR_FILE_CDROM_UNKNOWN;
        break;

(see below)

> +    }
>  
>   cleanup:
>      VIR_FORCE_CLOSE(fd);
> @@ -2029,7 +2057,8 @@ virFileIsCDROM(const char *path)
>  #else
>  
>  int
> -virFileIsCDROM(const char *path)
> +virFileCheckCDROM(const char *path,
> +                  virFileCDRomStatus *cd_status)

This will need an ATTRIBUTE_NOTUSED after *cd_status since it's not used
in this context or you could fill with UNKNOWN on success although
that's partially a lie.

>  {
>      if (STRPREFIX(path, "/dev/cd") ||
>          STRPREFIX(path, "/dev/acd"))
> diff --git a/src/util/virfile.h b/src/util/virfile.h
> index 6f1e802fde..767ee6ebaa 100644
> --- a/src/util/virfile.h
> +++ b/src/util/virfile.h
> @@ -210,7 +210,18 @@ 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_IS_NOT_CDROM,

^^This one is not used, but I think it can be "VIR_FILE_CDROM_UNKNOWN"

John

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