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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.