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@xxxxxxxxxx> wrote:
Thank u for reviewing. I will give patch v2 later.--On Tue, Jul 10, 2018 at 2:38 PM, Michal Privoznik <mprivozn@xxxxxxxxxx> wrote:This is in "if defined(__linux__)" block. You need to provide non-LinuxOn 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@xxxxxxxxxx>
> ---
> 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)
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.
Nit pick, the enum should go before the function. The reason is that if
> + 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,
> +};
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,-----------------------------------
--
Best regards,
-----------------------------------
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
