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

On 10/25/18 11:39 PM, Han Han wrote:

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

Implement virFileCheckCDROM in virStorageBackendVolOpen to check if cdrom backend is ok. Report more detailed error if not ok.

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

diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 318a556656..c9f89b817a 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -1601,6 +1601,38 @@ virStorageBackendVolOpen(const char *path, struct stat *sb, return -1; }

+#if defined(__linux__)

This not what I was expecting... Let's not have conditionals in this module and go with my suggestion from patch1 of changing the non linux version of virFileCheckCDROM to return VIR_FILE_CDROM_DISC_OK when we return 1 and to return/handle VIR_FILE_CDROM_NO_DEVICE here when 0 is returned (even though this code doesn't handle that "yet").

+ virFileCDRomStatus cd_status = VIR_FILE_CDROM_UNKNOWN;

Definition belongs at the top-level...

+ + if (virFileCheckCDROM(path, &cd_status) > 0) { + switch (cd_status) { + case VIR_FILE_CDROM_UNKNOWN: + virReportError(VIR_ERR_NO_STORAGE_VOL, + _("unknown status for CDROM storage vol '%s'"), + path); + return -1; + case VIR_FILE_CDROM_NO_DISC: + virReportError(VIR_ERR_NO_STORAGE_VOL, + _("no disc in CDROM storage vol '%s'"), + path); + return -1; + case VIR_FILE_CDROM_TRAY_OPEN: + virReportError(VIR_ERR_NO_STORAGE_VOL, + _("the tray of CDROM storage vol '%s' is open"), + path); + return -1; + case VIR_FILE_CDROM_DRIVE_NOT_READY: + virReportError(VIR_ERR_NO_STORAGE_VOL, + _("CDROM storage vol '%s' is not ready"), + path); + return -1; + case VIR_FILE_CDROM_DISC_OK: + VIR_INFO("CDROM storage vol %s is OK", path);

Depending on the level of debugging enabled and the times we go through here, this could be chatty... Also, kind of strange to be making a check for a specific issue from the bz description in a generic place. Check all the callers of virStorageBackendVolOpen... I think you may need to move this switch. For callers that may be correctly supplying a CDROM @path we could return a failure now. The switch may be nice as a common/shared method too so that no one is tempted to cut-n-paste whenever the values are needed. John

+ break; + } + } +#endif /* defined(__linux__) */ + /* 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