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@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,

On 07/11/2018 05:44 AM, Han Han wrote:

Implement virFileCheckCDROM in virStorageBackendVolOpen to check if cdrom backend is ready. Skip the error of cdrom not ready.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1596096

Signed-off-by: Han Han <hhan@redhat.com> --- src/storage/storage_util.c | 8 ++++++++ 1 file changed, 8 insertions(+)

diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index da99043e0a..08b625d136 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -1509,6 +1509,7 @@ virStorageBackendVolOpen(const char *path, struct stat *sb, int fd, mode = 0; char *base = last_component(path); bool noerror = (flags & VIR_STORAGE_VOL_OPEN_NOERROR); + virFileCDRomStatus cd_status;

s/;/ = VIR_FILE_CDROM_UNKNOWN;/

if (lstat(path, sb) < 0) { if (errno == ENOENT) { @@ -1545,6 +1546,13 @@ virStorageBackendVolOpen(const char *path, struct stat *sb, return -1; }

+ if (virFileCheckCDROM(path, &cd_status)) { + if (cd_status != VIR_FILE_CDROM_DISC_OK) { + VIR_WARN("ignoring CDROM %s is not ready", path);

And we have no idea why it's not ready ever logged. IOW: You have a cd_status there, but don't log what it is. Also, what if this libvirt is compiled when __linux__ is not default and thus uses the "#else" version of virFileCheckCDROM? The cd_status is not filled in there, so we'd fail always.

+ return -2;

Return of -2 is supposed to be limited to : " If VIR_STORAGE_VOL_OPEN_NOERROR is passed, we return -2 if file mode is unexpected or the volume is a dangling symbolic link." Note that other places returning -2 and using VIR_WARN will use "noerror" which is set at the top of the method. John

+ } + } + /* O_NONBLOCK should only matter during open() for fifos and * sockets, which we already filtered; but using it prevents a * TOCTTOU race. However, later on we will want to read() the