[libvirt] [PATCH 1/2] util: Refactor virFileIsCDROM to virFileCheckCDROM
Han Han
hhan at redhat.com
Wed Jul 11 07:55:42 UTC 2018
On Wed, Jul 11, 2018 at 3:04 PM, Peter Krempa <pkrempa at 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 at 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 at redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list
>
--
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/20180711/f1a95716/attachment-0001.htm>
More information about the libvir-list
mailing list