[libvirt] [PATCH 0/2] Add function to check cdrom status
In libvirt, it gives general Input/Output error when reading a not ready cdrom. This error could be more detailed on cdrom, such as drive not ready, no disc, trey open, no information. These patches provide private API virFileCdromStatus to check cdrom storage backend status and improve error handling of virStorageBackendVolOpen on cdrom. Han Han (2): util: Introduce virFileCdromStatus storage: Improve error handling on cdrom backend src/libvirt_private.syms | 1 + src/storage/storage_util.c | 38 ++++++++++++++++++++++++++++ src/util/virfile.c | 52 ++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 10 ++++++++ 4 files changed, 101 insertions(+) -- 2.17.1
This private API helps check cdrom drive status via ioctl(). Signed-off-by: Han Han <hhan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virfile.c | 52 ++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 10 ++++++++ 3 files changed, 63 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5499a368c0..e9f79ad433 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1797,6 +1797,7 @@ virFileActivateDirOverride; virFileBindMountDevice; virFileBuildPath; virFileCanonicalizePath; +virFileCdromStatus; virFileChownFiles; virFileClose; virFileComparePaths; diff --git a/src/util/virfile.c b/src/util/virfile.c index 378d03ecf0..790d9448d2 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2026,6 +2026,58 @@ virFileIsCDROM(const char *path) return ret; } +/** + * virFileCdromStatus: + * @path: Cdrom path + * + * Returns: + * -1 error happends. + * VIR_FILE_CDROM_DISC_OK Cdrom is OK. + * VIR_FILE_CDROM_NO_INFO Information not available. + * VIR_FILE_CDROM_NO_DISC No disc in cdrom. + * VIR_FILE_CDROM_TREY_OPEN Cdrom is trey-open. + * VIR_FILE_CDROM_DRIVE_NOT_READY Cdrom is not ready. + * reported. + */ +int +virFileCdromStatus(const char *path) +{ + int ret = -1; + int fd; + + if ((fd = open(path, O_RDONLY | O_NONBLOCK)) < 0) + goto cleanup; + + /* Attempt to detect CDROM status via ioctl */ + if ((ret = ioctl(fd, CDROM_DRIVE_STATUS, CDSL_CURRENT)) >= 0) { + switch (ret) { + case CDS_NO_INFO: + ret = VIR_FILE_CDROM_NO_INFO; + goto cleanup; + break; + case CDS_NO_DISC: + ret = VIR_FILE_CDROM_NO_DISC; + goto cleanup; + break; + case CDS_TRAY_OPEN: + ret = VIR_FILE_CDROM_TREY_OPEN; + goto cleanup; + break; + case CDS_DRIVE_NOT_READY: + ret = VIR_FILE_CDROM_DRIVE_NOT_READY; + goto cleanup; + break; + case CDS_DISC_OK: + ret = VIR_FILE_CDROM_DISC_OK; + goto cleanup; + break; + } + } + + cleanup: + VIR_FORCE_CLOSE(fd); + return ret; +} #else int diff --git a/src/util/virfile.h b/src/util/virfile.h index 6f1e802fde..9d4701c8d2 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -212,6 +212,16 @@ int virFileIsSharedFS(const char *path) ATTRIBUTE_NONNULL(1); int virFileIsMountPoint(const char *file) ATTRIBUTE_NONNULL(1); int virFileIsCDROM(const char *path) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; +int virFileCdromStatus(const char *path) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; + +enum { + VIR_FILE_CDROM_DISC_OK = 1, + VIR_FILE_CDROM_NO_INFO, + VIR_FILE_CDROM_NO_DISC, + VIR_FILE_CDROM_TREY_OPEN, + VIR_FILE_CDROM_DRIVE_NOT_READY, +}; int virFileGetMountSubtree(const char *mtabpath, const char *prefix, -- 2.17.1
On 07/03/2018 04:29 AM, Han Han wrote:
This private API helps check cdrom drive status via ioctl().
Signed-off-by: Han Han <hhan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virfile.c | 52 ++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 10 ++++++++ 3 files changed, 63 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5499a368c0..e9f79ad433 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1797,6 +1797,7 @@ virFileActivateDirOverride; virFileBindMountDevice; virFileBuildPath; virFileCanonicalizePath; +virFileCdromStatus; virFileChownFiles; virFileClose; virFileComparePaths; diff --git a/src/util/virfile.c b/src/util/virfile.c index 378d03ecf0..790d9448d2 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2026,6 +2026,58 @@ virFileIsCDROM(const char *path) return ret; }
+/** + * virFileCdromStatus: + * @path: Cdrom path + * + * Returns: + * -1 error happends. + * VIR_FILE_CDROM_DISC_OK Cdrom is OK. + * VIR_FILE_CDROM_NO_INFO Information not available. + * VIR_FILE_CDROM_NO_DISC No disc in cdrom. + * VIR_FILE_CDROM_TREY_OPEN Cdrom is trey-open. + * VIR_FILE_CDROM_DRIVE_NOT_READY Cdrom is not ready. + * reported. + */ +int +virFileCdromStatus(const char *path)
This is in "if defined(__linux__)" block. You need to provide non-Linux stub for this function too otherwise we will fail to build on such systems.
+{ + int ret = -1; + int fd; + + if ((fd = open(path, O_RDONLY | O_NONBLOCK)) < 0) + goto cleanup; + + /* Attempt to detect CDROM status via ioctl */ + if ((ret = ioctl(fd, CDROM_DRIVE_STATUS, CDSL_CURRENT)) >= 0) {
So virFileIsCDROM calls the same ioctl(). I wonder whether we should modify that function instead of inventing a new one. It would work like this: virFileISCDROM(const char *path, int *status); if @path is a CDROM, then @status is set to one of VIR_FILE_CDROM_*. However, caller can pass status = NULL which means they are not interested in status rather than plain is CDROM / isn't CDROM fact.
+ switch (ret) { + case CDS_NO_INFO: + ret = VIR_FILE_CDROM_NO_INFO; + goto cleanup; + break;
There is no reason for this goto. break is sufficient.
+ case CDS_NO_DISC: + ret = VIR_FILE_CDROM_NO_DISC; + goto cleanup; + break; + case CDS_TRAY_OPEN: + ret = VIR_FILE_CDROM_TREY_OPEN; + goto cleanup; + break; + case CDS_DRIVE_NOT_READY: + ret = VIR_FILE_CDROM_DRIVE_NOT_READY; + goto cleanup; + break; + case CDS_DISC_OK: + ret = VIR_FILE_CDROM_DISC_OK; + goto cleanup; + break; + } + } + + cleanup: + VIR_FORCE_CLOSE(fd); + return ret; +} #else
int diff --git a/src/util/virfile.h b/src/util/virfile.h index 6f1e802fde..9d4701c8d2 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -212,6 +212,16 @@ int virFileIsSharedFS(const char *path) ATTRIBUTE_NONNULL(1); int virFileIsMountPoint(const char *file) ATTRIBUTE_NONNULL(1); int virFileIsCDROM(const char *path) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; +int virFileCdromStatus(const char *path) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; + +enum { + VIR_FILE_CDROM_DISC_OK = 1, + VIR_FILE_CDROM_NO_INFO, + VIR_FILE_CDROM_NO_DISC, + VIR_FILE_CDROM_TREY_OPEN, + VIR_FILE_CDROM_DRIVE_NOT_READY, +};
Nit pick, the enum should go before the function. The reason is that if we decide to give the enum a name [1], we don't have to move things around. 1: which leads me to even better proposal. How about giving this enum a name, say virFileCDRomStatus and acknowledging this in function header: int virFileISCDROM(const char *path, virFileCDRomStatus *status); The set of return values of the function would remain the same as is now. Michal
Thank u for reviewing. I will give patch v2 later. On Tue, Jul 10, 2018 at 2:38 PM, Michal Privoznik <mprivozn@redhat.com> wrote:
On 07/03/2018 04:29 AM, Han Han wrote:
This private API helps check cdrom drive status via ioctl().
Signed-off-by: Han Han <hhan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virfile.c | 52 ++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 10 ++++++++ 3 files changed, 63 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5499a368c0..e9f79ad433 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1797,6 +1797,7 @@ virFileActivateDirOverride; virFileBindMountDevice; virFileBuildPath; virFileCanonicalizePath; +virFileCdromStatus; virFileChownFiles; virFileClose; virFileComparePaths; diff --git a/src/util/virfile.c b/src/util/virfile.c index 378d03ecf0..790d9448d2 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2026,6 +2026,58 @@ virFileIsCDROM(const char *path) return ret; }
+/** + * virFileCdromStatus: + * @path: Cdrom path + * + * Returns: + * -1 error happends. + * VIR_FILE_CDROM_DISC_OK Cdrom is OK. + * VIR_FILE_CDROM_NO_INFO Information not available. + * VIR_FILE_CDROM_NO_DISC No disc in cdrom. + * VIR_FILE_CDROM_TREY_OPEN Cdrom is trey-open. + * VIR_FILE_CDROM_DRIVE_NOT_READY Cdrom is not ready. + * reported. + */ +int +virFileCdromStatus(const char *path)
This is in "if defined(__linux__)" block. You need to provide non-Linux stub for this function too otherwise we will fail to build on such systems.
+{ + int ret = -1; + int fd; + + if ((fd = open(path, O_RDONLY | O_NONBLOCK)) < 0) + goto cleanup; + + /* Attempt to detect CDROM status via ioctl */ + if ((ret = ioctl(fd, CDROM_DRIVE_STATUS, CDSL_CURRENT)) >= 0) {
So virFileIsCDROM calls the same ioctl(). I wonder whether we should modify that function instead of inventing a new one. It would work like this:
virFileISCDROM(const char *path, int *status);
if @path is a CDROM, then @status is set to one of VIR_FILE_CDROM_*. However, caller can pass status = NULL which means they are not interested in status rather than plain is CDROM / isn't CDROM fact.
+ switch (ret) { + case CDS_NO_INFO: + ret = VIR_FILE_CDROM_NO_INFO; + goto cleanup; + break;
There is no reason for this goto. break is sufficient.
+ case CDS_NO_DISC: + ret = VIR_FILE_CDROM_NO_DISC; + goto cleanup; + break; + case CDS_TRAY_OPEN: + ret = VIR_FILE_CDROM_TREY_OPEN; + goto cleanup; + break; + case CDS_DRIVE_NOT_READY: + ret = VIR_FILE_CDROM_DRIVE_NOT_READY; + goto cleanup; + break; + case CDS_DISC_OK: + ret = VIR_FILE_CDROM_DISC_OK; + goto cleanup; + break; + } + } + + cleanup: + VIR_FORCE_CLOSE(fd); + return ret; +} #else
int diff --git a/src/util/virfile.h b/src/util/virfile.h index 6f1e802fde..9d4701c8d2 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -212,6 +212,16 @@ int virFileIsSharedFS(const char *path) ATTRIBUTE_NONNULL(1); int virFileIsMountPoint(const char *file) ATTRIBUTE_NONNULL(1); int virFileIsCDROM(const char *path) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; +int virFileCdromStatus(const char *path) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; + +enum { + VIR_FILE_CDROM_DISC_OK = 1, + VIR_FILE_CDROM_NO_INFO, + VIR_FILE_CDROM_NO_DISC, + VIR_FILE_CDROM_TREY_OPEN, + VIR_FILE_CDROM_DRIVE_NOT_READY, +};
Nit pick, the enum should go before the function. The reason is that if we decide to give the enum a name [1], we don't have to move things around.
1: which leads me to even better proposal. How about giving this enum a name, say virFileCDRomStatus and acknowledging this in function header:
int virFileISCDROM(const char *path, virFileCDRomStatus *status);
The set of return values of the function would remain the same as is now.
Michal
-- Best regards, ----------------------------------- Han Han Quality Engineer Redhat. Email: hhan@redhat.com Phone: +861065339333
Hello Michel, I agree with you that we could integrate these two APIs into one. If so, I think we should change the API name virFileIsCDROM to **virFileCheckCDROM** or **virFileCdromState**. virFileIsCDROM is confusing and doesn't indicate the function of checking state. On Tue, Jul 10, 2018 at 3:05 PM, Han Han <hhan@redhat.com> wrote:
Thank u for reviewing. I will give patch v2 later.
On Tue, Jul 10, 2018 at 2:38 PM, Michal Privoznik <mprivozn@redhat.com> wrote:
On 07/03/2018 04:29 AM, Han Han wrote:
This private API helps check cdrom drive status via ioctl().
Signed-off-by: Han Han <hhan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virfile.c | 52 ++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 10 ++++++++ 3 files changed, 63 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5499a368c0..e9f79ad433 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1797,6 +1797,7 @@ virFileActivateDirOverride; virFileBindMountDevice; virFileBuildPath; virFileCanonicalizePath; +virFileCdromStatus; virFileChownFiles; virFileClose; virFileComparePaths; diff --git a/src/util/virfile.c b/src/util/virfile.c index 378d03ecf0..790d9448d2 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2026,6 +2026,58 @@ virFileIsCDROM(const char *path) return ret; }
+/** + * virFileCdromStatus: + * @path: Cdrom path + * + * Returns: + * -1 error happends. + * VIR_FILE_CDROM_DISC_OK Cdrom is OK. + * VIR_FILE_CDROM_NO_INFO Information not available. + * VIR_FILE_CDROM_NO_DISC No disc in cdrom. + * VIR_FILE_CDROM_TREY_OPEN Cdrom is trey-open. + * VIR_FILE_CDROM_DRIVE_NOT_READY Cdrom is not ready. + * reported. + */ +int +virFileCdromStatus(const char *path)
This is in "if defined(__linux__)" block. You need to provide non-Linux stub for this function too otherwise we will fail to build on such systems.
+{ + int ret = -1; + int fd; + + if ((fd = open(path, O_RDONLY | O_NONBLOCK)) < 0) + goto cleanup; + + /* Attempt to detect CDROM status via ioctl */ + if ((ret = ioctl(fd, CDROM_DRIVE_STATUS, CDSL_CURRENT)) >= 0) {
So virFileIsCDROM calls the same ioctl(). I wonder whether we should modify that function instead of inventing a new one. It would work like this:
virFileISCDROM(const char *path, int *status);
if @path is a CDROM, then @status is set to one of VIR_FILE_CDROM_*. However, caller can pass status = NULL which means they are not interested in status rather than plain is CDROM / isn't CDROM fact.
+ switch (ret) { + case CDS_NO_INFO: + ret = VIR_FILE_CDROM_NO_INFO; + goto cleanup; + break;
There is no reason for this goto. break is sufficient.
+ case CDS_NO_DISC: + ret = VIR_FILE_CDROM_NO_DISC; + goto cleanup; + break; + case CDS_TRAY_OPEN: + ret = VIR_FILE_CDROM_TREY_OPEN; + goto cleanup; + break; + case CDS_DRIVE_NOT_READY: + ret = VIR_FILE_CDROM_DRIVE_NOT_READY; + goto cleanup; + break; + case CDS_DISC_OK: + ret = VIR_FILE_CDROM_DISC_OK; + goto cleanup; + break; + } + } + + cleanup: + VIR_FORCE_CLOSE(fd); + return ret; +} #else
int diff --git a/src/util/virfile.h b/src/util/virfile.h index 6f1e802fde..9d4701c8d2 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -212,6 +212,16 @@ int virFileIsSharedFS(const char *path) ATTRIBUTE_NONNULL(1); int virFileIsMountPoint(const char *file) ATTRIBUTE_NONNULL(1); int virFileIsCDROM(const char *path) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; +int virFileCdromStatus(const char *path) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; + +enum { + VIR_FILE_CDROM_DISC_OK = 1, + VIR_FILE_CDROM_NO_INFO, + VIR_FILE_CDROM_NO_DISC, + VIR_FILE_CDROM_TREY_OPEN, + VIR_FILE_CDROM_DRIVE_NOT_READY, +};
Nit pick, the enum should go before the function. The reason is that if we decide to give the enum a name [1], we don't have to move things around.
1: which leads me to even better proposal. How about giving this enum a name, say virFileCDRomStatus and acknowledging this in function header:
int virFileISCDROM(const char *path, virFileCDRomStatus *status);
The set of return values of the function would remain the same as is now.
Michal
-- Best regards, ----------------------------------- Han Han Quality Engineer Redhat.
Email: hhan@redhat.com Phone: +861065339333
-- Best regards, ----------------------------------- Han Han Quality Engineer Redhat. Email: hhan@redhat.com Phone: +861065339333
Sorry, it should be virFileCdromStatus not virFileCdromState :) On Tue, Jul 10, 2018 at 9:28 PM, Han Han <hhan@redhat.com> wrote:
Hello Michel, I agree with you that we could integrate these two APIs into one. If so, I think we should change the API name virFileIsCDROM to **virFileCheckCDROM** or **virFileCdromState**. virFileIsCDROM is confusing and doesn't indicate the function of checking state.
On Tue, Jul 10, 2018 at 3:05 PM, Han Han <hhan@redhat.com> wrote:
Thank u for reviewing. I will give patch v2 later.
On Tue, Jul 10, 2018 at 2:38 PM, Michal Privoznik <mprivozn@redhat.com> wrote:
On 07/03/2018 04:29 AM, Han Han wrote:
This private API helps check cdrom drive status via ioctl().
Signed-off-by: Han Han <hhan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virfile.c | 52 ++++++++++++++++++++++++++++++ ++++++++++ src/util/virfile.h | 10 ++++++++ 3 files changed, 63 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5499a368c0..e9f79ad433 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1797,6 +1797,7 @@ virFileActivateDirOverride; virFileBindMountDevice; virFileBuildPath; virFileCanonicalizePath; +virFileCdromStatus; virFileChownFiles; virFileClose; virFileComparePaths; diff --git a/src/util/virfile.c b/src/util/virfile.c index 378d03ecf0..790d9448d2 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2026,6 +2026,58 @@ virFileIsCDROM(const char *path) return ret; }
+/** + * virFileCdromStatus: + * @path: Cdrom path + * + * Returns: + * -1 error happends. + * VIR_FILE_CDROM_DISC_OK Cdrom is OK. + * VIR_FILE_CDROM_NO_INFO Information not available. + * VIR_FILE_CDROM_NO_DISC No disc in cdrom. + * VIR_FILE_CDROM_TREY_OPEN Cdrom is trey-open. + * VIR_FILE_CDROM_DRIVE_NOT_READY Cdrom is not ready. + * reported. + */ +int +virFileCdromStatus(const char *path)
This is in "if defined(__linux__)" block. You need to provide non-Linux stub for this function too otherwise we will fail to build on such systems.
+{ + int ret = -1; + int fd; + + if ((fd = open(path, O_RDONLY | O_NONBLOCK)) < 0) + goto cleanup; + + /* Attempt to detect CDROM status via ioctl */ + if ((ret = ioctl(fd, CDROM_DRIVE_STATUS, CDSL_CURRENT)) >= 0) {
So virFileIsCDROM calls the same ioctl(). I wonder whether we should modify that function instead of inventing a new one. It would work like this:
virFileISCDROM(const char *path, int *status);
if @path is a CDROM, then @status is set to one of VIR_FILE_CDROM_*. However, caller can pass status = NULL which means they are not interested in status rather than plain is CDROM / isn't CDROM fact.
+ switch (ret) { + case CDS_NO_INFO: + ret = VIR_FILE_CDROM_NO_INFO; + goto cleanup; + break;
There is no reason for this goto. break is sufficient.
+ case CDS_NO_DISC: + ret = VIR_FILE_CDROM_NO_DISC; + goto cleanup; + break; + case CDS_TRAY_OPEN: + ret = VIR_FILE_CDROM_TREY_OPEN; + goto cleanup; + break; + case CDS_DRIVE_NOT_READY: + ret = VIR_FILE_CDROM_DRIVE_NOT_READY; + goto cleanup; + break; + case CDS_DISC_OK: + ret = VIR_FILE_CDROM_DISC_OK; + goto cleanup; + break; + } + } + + cleanup: + VIR_FORCE_CLOSE(fd); + return ret; +} #else
int diff --git a/src/util/virfile.h b/src/util/virfile.h index 6f1e802fde..9d4701c8d2 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -212,6 +212,16 @@ int virFileIsSharedFS(const char *path) ATTRIBUTE_NONNULL(1); int virFileIsMountPoint(const char *file) ATTRIBUTE_NONNULL(1); int virFileIsCDROM(const char *path) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; +int virFileCdromStatus(const char *path) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; + +enum { + VIR_FILE_CDROM_DISC_OK = 1, + VIR_FILE_CDROM_NO_INFO, + VIR_FILE_CDROM_NO_DISC, + VIR_FILE_CDROM_TREY_OPEN, + VIR_FILE_CDROM_DRIVE_NOT_READY, +};
Nit pick, the enum should go before the function. The reason is that if we decide to give the enum a name [1], we don't have to move things around.
1: which leads me to even better proposal. How about giving this enum a name, say virFileCDRomStatus and acknowledging this in function header:
int virFileISCDROM(const char *path, virFileCDRomStatus *status);
The set of return values of the function would remain the same as is now.
Michal
-- Best regards, ----------------------------------- Han Han Quality Engineer Redhat.
Email: hhan@redhat.com Phone: +861065339333
-- Best regards, ----------------------------------- Han Han Quality Engineer Redhat.
Email: hhan@redhat.com Phone: +861065339333
-- Best regards, ----------------------------------- Han Han Quality Engineer Redhat. Email: hhan@redhat.com Phone: +861065339333
On 07/10/2018 03:34 PM, Han Han wrote:
Sorry, it should be virFileCdromStatus not virFileCdromState :)
Yes, that makes sense. Also, please do not top post on technical lists. It's common to either reply in line or at the bottom. Michal
Implement virFileCdromStatus() in virStorageBackendVolOpen to show detailed errors or warnings of cdrom instead of general Input/output error. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1596096 Signed-off-by: Han Han <hhan@redhat.com> --- src/storage/storage_util.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index a701a75702..5a7ed4c76f 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -1538,6 +1538,44 @@ virStorageBackendVolOpen(const char *path, struct stat *sb, return -1; } + if (virFileIsCDROM(path) == 1) { + switch (virFileCdromStatus(path)) { + case VIR_FILE_CDROM_NO_INFO : + if (noerror) { + VIR_WARN("ignoring unavailable information of %s", path); + return -2; + } + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cdrom %s information is unavailable"), path); + return -1; + case VIR_FILE_CDROM_NO_DISC : + if (noerror) { + VIR_WARN("ignoring no disc %s", path); + return -2; + } + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cdrom %s has no disc"), path); + return -1; + case VIR_FILE_CDROM_TREY_OPEN : + if (noerror) { + VIR_WARN("ignoring trey open of %s", path); + return -2; + } + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cdrom %s is on trey-open status"), path); + return -1; + case VIR_FILE_CDROM_DRIVE_NOT_READY : + if (noerror) { + VIR_WARN("ignoring %s not ready", path); + return -2; + } + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cdrom %s is on not ready"), path); + return -1; + case VIR_FILE_CDROM_DISC_OK : + VIR_INFO("Cdrom %s status is ok", path); + } + } /* 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 -- 2.17.1
On 07/03/2018 04:29 AM, Han Han wrote:
Implement virFileCdromStatus() in virStorageBackendVolOpen to show detailed errors or warnings of cdrom instead of general Input/output error.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1596096 Signed-off-by: Han Han <hhan@redhat.com> --- src/storage/storage_util.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)
diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index a701a75702..5a7ed4c76f 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -1538,6 +1538,44 @@ virStorageBackendVolOpen(const char *path, struct stat *sb, return -1; }
+ if (virFileIsCDROM(path) == 1) { + switch (virFileCdromStatus(path)) {
See? With my proposal this would be a single function call. Michal
On Tue, Jul 03, 2018 at 10:29:39 +0800, Han Han wrote:
Implement virFileCdromStatus() in virStorageBackendVolOpen to show detailed errors or warnings of cdrom instead of general Input/output error.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1596096 Signed-off-by: Han Han <hhan@redhat.com> --- src/storage/storage_util.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)
diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index a701a75702..5a7ed4c76f 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -1538,6 +1538,44 @@ virStorageBackendVolOpen(const char *path, struct stat *sb, return -1; }
+ if (virFileIsCDROM(path) == 1) { + switch (virFileCdromStatus(path)) { + case VIR_FILE_CDROM_NO_INFO :
I think this is such a corner case that it does not make sense to provide as detailed error messages as you can see below. Treating this as any other storage volume which is lacking the backing file should be sufficient. That means that it might be required to skip the error in case of the CDROM but reporting this status is IMO overkill and definitely not worth the code complexity.
+ if (noerror) { + VIR_WARN("ignoring unavailable information of %s", path); + return -2; + } + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cdrom %s information is unavailable"), path); + return -1; + case VIR_FILE_CDROM_NO_DISC : + if (noerror) { + VIR_WARN("ignoring no disc %s", path); + return -2; + } + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cdrom %s has no disc"), path); + return -1; + case VIR_FILE_CDROM_TREY_OPEN : + if (noerror) { + VIR_WARN("ignoring trey open of %s", path); + return -2; + } + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cdrom %s is on trey-open status"), path); + return -1; + case VIR_FILE_CDROM_DRIVE_NOT_READY : + if (noerror) { + VIR_WARN("ignoring %s not ready", path); + return -2; + } + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cdrom %s is on not ready"), path); + return -1; + case VIR_FILE_CDROM_DISC_OK : + VIR_INFO("Cdrom %s status is ok", path); + } + } /* 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 -- 2.17.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Hello, could anyone help review? On Tue, Jul 3, 2018 at 10:29 AM, Han Han <hhan@redhat.com> wrote:
In libvirt, it gives general Input/Output error when reading a not ready cdrom. This error could be more detailed on cdrom, such as drive not ready, no disc, trey open, no information. These patches provide private API virFileCdromStatus to check cdrom storage backend status and improve error handling of virStorageBackendVolOpen on cdrom.
Han Han (2): util: Introduce virFileCdromStatus storage: Improve error handling on cdrom backend
src/libvirt_private.syms | 1 + src/storage/storage_util.c | 38 ++++++++++++++++++++++++++++ src/util/virfile.c | 52 ++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 10 ++++++++ 4 files changed, 101 insertions(+)
-- 2.17.1
-- Best regards, ----------------------------------- Han Han Quality Engineer Redhat. Email: hhan@redhat.com Phone: +861065339333
participants (3)
-
Han Han -
Michal Privoznik -
Peter Krempa