9:29 p.m.
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
9:29 p.m.
New subject: [libvirt] [PATCH 1/2] util: Introduce virFileCdromStatus
This private API helps check cdrom drive status via ioctl().
Signed-off-by: Han Han <hhan(a)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
1:38 a.m.
New subject: [libvirt] [PATCH 1/2] util: Introduce virFileCdromStatus
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(a)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
2:05 a.m.
New subject: [libvirt] [PATCH 1/2] util: Introduce virFileCdromStatus
Thank u for reviewing. I will give patch v2 later.
On Tue, Jul 10, 2018 at 2:38 PM, Michal Privoznik <mprivozn(a)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(a)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(a)redhat.com
Phone: +861065339333
8:28 a.m.
New subject: [libvirt] [PATCH 1/2] util: Introduce virFileCdromStatus
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(a)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(a)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(a)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(a)redhat.com
Phone: +861065339333
--
Best regards,
-----------------------------------
Han Han
Quality Engineer
Redhat.
Email: hhan(a)redhat.com
Phone: +861065339333
8:34 a.m.
New subject: [libvirt] [PATCH 1/2] util: Introduce virFileCdromStatus
Sorry, it should be virFileCdromStatus not virFileCdromState :)
On Tue, Jul 10, 2018 at 9:28 PM, Han Han <hhan(a)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(a)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(a)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(a)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(a)redhat.com
> Phone: +861065339333
>
--
Best regards,
-----------------------------------
Han Han
Quality Engineer
Redhat.
Email: hhan(a)redhat.com
Phone: +861065339333
--
Best regards,
-----------------------------------
Han Han
Quality Engineer
Redhat.
Email: hhan(a)redhat.com
Phone: +861065339333
9:38 a.m.
New subject: [libvirt] [PATCH 1/2] util: Introduce virFileCdromStatus
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
9:29 p.m.
New subject: [libvirt] [PATCH 2/2] storage: Improve error handling on cdrom backend
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(a)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
1:38 a.m.
New subject: [libvirt] [PATCH 2/2] storage: Improve error handling on cdrom backend
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(a)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
9:52 a.m.
New subject: [libvirt] [PATCH 2/2] storage: Improve error handling on cdrom backend
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
9:05 p.m.
New subject: [libvirt] [PATCH 0/2] Add function to check cdrom status
Hello, could anyone help review?
On Tue, Jul 3, 2018 at 10:29 AM, Han Han <hhan(a)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(a)redhat.com
Phone: +861065339333
2327
days inactive
2334
days old
10 comments
3 participants
participants (3)
-
Han Han -
Michal Privoznik -
Peter Krempa