[libvirt] [PATCH 1/2] util: Introduce virFileCdromStatus
Han Han
hhan at redhat.com
Tue Jul 10 13:28:12 UTC 2018
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 at 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 at 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 at 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 at redhat.com
> Phone: +861065339333
>
--
Best regards,
-----------------------------------
Han Han
Quality Engineer
Redhat.
Email: hhan at redhat.com
Phone: +861065339333
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180710/b342c1ee/attachment-0001.htm>
More information about the libvir-list
mailing list