[libvirt] [PATCH 1/2] util: Introduce virFileCdromStatus
Han Han
hhan at redhat.com
Tue Jul 10 13:34:11 UTC 2018
Sorry, it should be virFileCdromStatus not virFileCdromState :)
On Tue, Jul 10, 2018 at 9:28 PM, Han Han <hhan at 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 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
>
--
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/fd0f35b6/attachment-0001.htm>
More information about the libvir-list
mailing list