Re: [libvirt] [PATCH 1/2] util: Refactor virFileIsCDROM to virFileCheckCDROM

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(a)redhat.com&gt; > --- > 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.

> ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; > > int virFileGetMountSubtree(const char *mtabpath, > -- > 2.17.1 > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list