Add enum type of cdrom statuses. Add argument cd_status in
virFileCheckCDROM to store cdrom status.
Now virFileCheckCDROM could be used to check the cdrom drive status such
as no info, no disc, trey open, drive not ready or ok.
Signed-off-by: Han Han <hhan@redhat.com>
---
src/libvirt_private.syms | 2 +-
src/qemu/qemu_domain.c | 4 ++--
src/util/virfile.c | 44 ++++++++++++++++++++++++++++++++++------
src/util/virfile.h | 11 +++++++++-
4 files changed, 51 insertions(+), 10 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index e688981c3e..b03e1f15a7 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1811,7 +1811,7 @@ virFileGetMountSubtree;
virFileHasSuffix;
virFileInData;
virFileIsAbsPath;
-virFileIsCDROM;
+virFileCheckCDROM;
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..a2199e6a97 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -1990,19 +1990,21 @@ int virFileIsMountPoint(const char *file)
#if defined(__linux__)
/**
- * virFileIsCDROM:
+ * virFileCheckCDROM:
* @path: File to check
+ * @cd_status: Ptr to store CDROM status; Not to store status if NULL
*
* 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.
*/
int
-virFileIsCDROM(const char *path)
+virFileCheckCDROM(const char *path, int *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 +2018,40 @@ 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 (cd_status == NULL) {
+ if (*status >= 0)
+ ret = 1;
+ else
+ ret = 0;
+ goto cleanup;
+ }
+
+ if (*status < 0) {
ret = 0;
+ 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_TREY_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;
+ }
+
+ ret = 1;
cleanup:
VIR_FORCE_CLOSE(fd);
@@ -2029,7 +2061,7 @@ virFileIsCDROM(const char *path)
#else
int
-virFileIsCDROM(const char *path)
+virFileCheckCDROM(const char *path, int *cd_status)
{
if (STRPREFIX(path, "/dev/cd") ||
STRPREFIX(path, "/dev/acd"))
diff --git a/src/util/virfile.h b/src/util/virfile.h
index 6f1e802fde..e06ccd8f9f 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -210,7 +210,16 @@ 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)
+
+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 virFileCheckCDROM(const char *path)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
int virFileGetMountSubtree(const char *mtabpath,
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jul 11, 2018 at 11:07:52 +0800, Han Han wrote:
> Add enum type of cdrom statuses. Add argument cd_status in
> virFileCheckCDROM to store cdrom status.
> Now virFileCheckCDROM could be used to check the cdrom drive status such
> as no info, no disc, trey open, drive not ready or ok.
tray
Also it does not mention that you've renamed the function.
>
> Signed-off-by: Han Han <hhan@redhat.com>
> ---
> src/libvirt_private.syms | 2 +-
> src/qemu/qemu_domain.c | 4 ++--
> src/util/virfile.c | 44 ++++++++++++++++++++++++++++++++++------
> src/util/virfile.h | 11 +++++++++-
> 4 files changed, 51 insertions(+), 10 deletions(-)
[...]
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index 378d03ecf0..a2199e6a97 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -1990,19 +1990,21 @@ int virFileIsMountPoint(const char *file)
>
> #if defined(__linux__)
> /**
> - * virFileIsCDROM:
> + * virFileCheckCDROM:
> * @path: File to check
> + * @cd_status: Ptr to store CDROM status; Not to store status if NULL
@cd_status: Filled with the status of the CDROM if non-NULL. See
$ENUMNAME
> *
> * 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.
> */
> int
> -virFileIsCDROM(const char *path)
> +virFileCheckCDROM(const char *path, int *cd_status)
One line per argument. Also use the proper type since it will be filled
by a value from the enum/
> {
> struct stat st;
> int fd;
> int ret = -1;
> + int *status;
You declared this as a pointer without initialization ...
>
> if ((fd = open(path, O_RDONLY | O_NONBLOCK)) < 0)
> goto cleanup;
> @@ -2016,10 +2018,40 @@ 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);
... and now you dereference it? This will crash in most cases.
ioctl() returns an integer, not a pointer to an integer
> +
> + if (cd_status == NULL) {
> + if (*status >= 0)
> + ret = 1;
> + else
> + ret = 0;
> + goto cleanup;
> + }
> +
> + if (*status < 0) {
> ret = 0;
> + goto cleanup;
> + }
This is too complicated. In both cases you need to return 0 if the
status returned from ioctl is < 0. Then additionally if 'cd_status' is
not NULL you need to perform the rest:
status = ioctl(fd, CDROM_DRIVE_STATUS, CDSL_CURRENT);
VIR_FORCE_CLOSE(fd);
if (status < 0)
return 0;
if (!cd_status)
return 1;
> +
> + 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_TREY_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;
> +
> + ret = 1;
>
> cleanup:
> VIR_FORCE_CLOSE(fd);
> @@ -2029,7 +2061,7 @@ virFileIsCDROM(const char *path)
> #else
>
> int
> -virFileIsCDROM(const char *path)
> +virFileCheckCDROM(const char *path, int *cd_status)
One argument per line.
> {
> if (STRPREFIX(path, "/dev/cd") ||
> STRPREFIX(path, "/dev/acd"))
> diff --git a/src/util/virfile.h b/src/util/virfile.h
> index 6f1e802fde..e06ccd8f9f 100644
> --- a/src/util/virfile.h
> +++ b/src/util/virfile.h
> @@ -210,7 +210,16 @@ 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)
> +
> +enum {
You probably should add a value of 0 which will mean that it was not
filled.
> + VIR_FILE_CDROM_DISC_OK = 1,
> + VIR_FILE_CDROM_NO_INFO,
> + VIR_FILE_CDROM_NO_DISC,
> + VIR_FILE_CDROM_TREY_OPEN,
TRAY
> + VIR_FILE_CDROM_DRIVE_NOT_READY,
> +};
The enum should be named.
> +
> +int virFileCheckCDROM(const char *path)
Hmmm, you are missing the new argument here. Not sure how you managed to
compile it.
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
>
> int virFileGetMountSubtree(const char *mtabpath,
> --
> 2.17.1
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jul 11, 2018 at 3:04 PM, Peter Krempa <pkrempa@redhat.com> wrote:
> On Wed, Jul 11, 2018 at 11:07:52 +0800, Han Han wrote:
> > Add enum type of cdrom statuses. Add argument cd_status in
> > virFileCheckCDROM to store cdrom status.
> > Now virFileCheckCDROM could be used to check the cdrom drive status such
> > as no info, no disc, trey open, drive not ready or ok.
>
> tray
>
> Also it does not mention that you've renamed the function.
>
> I will mention it in commit msg.
> >
> > Signed-off-by: Han Han <hhan@redhat.com>
> > ---
> > src/libvirt_private.syms | 2 +-
> > src/qemu/qemu_domain.c | 4 ++--
> > src/util/virfile.c | 44 ++++++++++++++++++++++++++++++++++------
> > src/util/virfile.h | 11 +++++++++-
> > 4 files changed, 51 insertions(+), 10 deletions(-)
>
> [...]
>
> > diff --git a/src/util/virfile.c b/src/util/virfile.c
> > index 378d03ecf0..a2199e6a97 100644
> > --- a/src/util/virfile.c
> > +++ b/src/util/virfile.c
> > @@ -1990,19 +1990,21 @@ int virFileIsMountPoint(const char *file)
> >
> > #if defined(__linux__)
> > /**
> > - * virFileIsCDROM:
> > + * virFileCheckCDROM:
> > * @path: File to check
> > + * @cd_status: Ptr to store CDROM status; Not to store status if NULL
>
> @cd_status: Filled with the status of the CDROM if non-NULL. See
> $ENUMNAME
>
> > *
> > * 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.
> > */
> > int
> > -virFileIsCDROM(const char *path)
> > +virFileCheckCDROM(const char *path, int *cd_status)
>
> One line per argument. Also use the proper type since it will be filled
> by a value from the enum/
>
> > {
> > struct stat st;
> > int fd;
> > int ret = -1;
> > + int *status;
>
> You declared this as a pointer without initialization ...
>
> Yeah. It's not necessary to use pointer here. I will use int instead.
> >
> > if ((fd = open(path, O_RDONLY | O_NONBLOCK)) < 0)
> > goto cleanup;
> > @@ -2016,10 +2018,40 @@ 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);
>
> ... and now you dereference it? This will crash in most cases.
>
> ioctl() returns an integer, not a pointer to an integer
>
> > +
> > + if (cd_status == NULL) {
> > + if (*status >= 0)
> > + ret = 1;
> > + else
> > + ret = 0;
> > + goto cleanup;
> > + }
> > +
> > + if (*status < 0) {
> > ret = 0;
> > + goto cleanup;
> > + }
>
> This is too complicated. In both cases you need to return 0 if the
> status returned from ioctl is < 0. Then additionally if 'cd_status' is
> not NULL you need to perform the rest:
>
> status = ioctl(fd, CDROM_DRIVE_STATUS, CDSL_CURRENT);
>
> VIR_FORCE_CLOSE(fd);
>
> if (status < 0)
> return 0;
>
> if (!cd_status)
> return 1;
>
> Nice advice. It looks simpler here.
> > +
> > + 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_TREY_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;
>
> > +
> > + ret = 1;
> >
> > cleanup:
> > VIR_FORCE_CLOSE(fd);
> > @@ -2029,7 +2061,7 @@ virFileIsCDROM(const char *path)
> > #else
> >
> > int
> > -virFileIsCDROM(const char *path)
> > +virFileCheckCDROM(const char *path, int *cd_status)
>
> One argument per line.
>
> > {
> > if (STRPREFIX(path, "/dev/cd") ||
> > STRPREFIX(path, "/dev/acd"))
> > diff --git a/src/util/virfile.h b/src/util/virfile.h
> > index 6f1e802fde..e06ccd8f9f 100644
> > --- a/src/util/virfile.h
> > +++ b/src/util/virfile.h
> > @@ -210,7 +210,16 @@ 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)
> > +
> > +enum {
>
> You probably should add a value of 0 which will mean that it was not
> filled.
>
> > + VIR_FILE_CDROM_DISC_OK = 1,
> > + VIR_FILE_CDROM_NO_INFO,
> > + VIR_FILE_CDROM_NO_DISC,
> > + VIR_FILE_CDROM_TREY_OPEN,
>
> TRAY
>
> > + VIR_FILE_CDROM_DRIVE_NOT_READY,
> > +};
>
> The enum should be named.
>
> > +
> > +int virFileCheckCDROM(const char *path)
>
> Hmmm, you are missing the new argument here. Not sure how you managed to
> compile it.
>
I forgot to compile and make check... I will follow the contributor
guidelines strictly in the later patches.
Thank you very much. I will fix these and send the version 3.
>
> > ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
> >
> > int virFileGetMountSubtree(const char *mtabpath,
> > --
> > 2.17.1
> >
> > --
> > libvir-list mailing list
> > libvir-list@redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list
>
--
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
© 2016 - 2026 Red Hat, Inc.