On Wed, Jul 11, 2018 at 3:04 PM, Peter Krempa <pkre...@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 <h...@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@redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list
>



-- 
Best regards,
-----------------------------------
Han Han
Quality Engineer
Redhat.

Email: h...@redhat.com
Phone: +861065339333
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Reply via email to