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

[Peter Krempa]

On Wed, Jul 11, 2018 at 11:07:52 +0800, Han Han wrote:
> Add enum type of cdrom statuses. Add argument cd_status in
> virFileCheckCDROM to store cdrom status.
> Now virFileCheckCDROM could be used to check the cdrom drive status such
> as no info, no disc, trey open, drive not ready or ok.

tray

Also it does not mention that you've renamed the function.

> 
> Signed-off-by: Han Han <hhan at redhat.com>
> ---
>  src/libvirt_private.syms |  2 +-
>  src/qemu/qemu_domain.c   |  4 ++--
>  src/util/virfile.c       | 44 ++++++++++++++++++++++++++++++++++------
>  src/util/virfile.h       | 11 +++++++++-
>  4 files changed, 51 insertions(+), 10 deletions(-)

[...]

> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index 378d03ecf0..a2199e6a97 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -1990,19 +1990,21 @@ int virFileIsMountPoint(const char *file)
>  
>  #if defined(__linux__)
>  /**
> - * virFileIsCDROM:
> + * virFileCheckCDROM:
>   * @path: File to check
> + * @cd_status: Ptr to store CDROM status; Not to store status if NULL

@cd_status: Filled with the status of the CDROM if non-NULL. See
$ENUMNAME

>   *
>   * 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.
>   */
>  int
> -virFileIsCDROM(const char *path)
> +virFileCheckCDROM(const char *path, int *cd_status)

One line per argument. Also use the proper type since it will be filled
by a value from the enum/

>  {
>      struct stat st;
>      int fd;
>      int ret = -1;
> +    int *status;

You declared this as a pointer without initialization ...

>  
>      if ((fd = open(path, O_RDONLY | O_NONBLOCK)) < 0)
>          goto cleanup;
> @@ -2016,10 +2018,40 @@ 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);

... and now you dereference it? This will crash in most cases.

ioctl() returns an integer, not a pointer to an integer

> +
> +    if (cd_status == NULL) {
> +        if (*status >= 0)
> +            ret = 1;
> +        else
> +            ret = 0;
> +        goto cleanup;
> +    }
> +
> +    if (*status < 0) {
>          ret = 0;
> +        goto cleanup;
> +    }

This is too complicated. In both cases you need to return 0 if the
status returned from ioctl is < 0. Then additionally if 'cd_status' is
not NULL you need to perform the rest:

status = ioctl(fd, CDROM_DRIVE_STATUS, CDSL_CURRENT);

VIR_FORCE_CLOSE(fd);

if (status < 0)
    return 0;

if (!cd_status)
    return 1;

> +
> +    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_TREY_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;

> +
> +    ret = 1;
>  
>   cleanup:
>      VIR_FORCE_CLOSE(fd);
> @@ -2029,7 +2061,7 @@ virFileIsCDROM(const char *path)
>  #else
>  
>  int
> -virFileIsCDROM(const char *path)
> +virFileCheckCDROM(const char *path, int *cd_status)

One argument per line.

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

You probably should add a value of 0 which will mean that it was not
filled.

> +    VIR_FILE_CDROM_DISC_OK = 1,
> +    VIR_FILE_CDROM_NO_INFO,
> +    VIR_FILE_CDROM_NO_DISC,
> +    VIR_FILE_CDROM_TREY_OPEN,

TRAY

> +    VIR_FILE_CDROM_DRIVE_NOT_READY,
> +};

The enum should be named.

> +
> +int virFileCheckCDROM(const char *path)

Hmmm, you are missing the new argument here. Not sure how you managed to
compile it.

>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
>  
>  int virFileGetMountSubtree(const char *mtabpath,
> -- 
> 2.17.1
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180711/e1927fc2/attachment-0001.sig>