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@xxxxxxxxxx>
> ---
> 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,
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
