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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
