Sorry, it should be virFileCdromStatus not virFileCdromState :) On Tue, Jul 10, 2018 at 9:28 PM, Han Han <hhan@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@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@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@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@redhat.com Phone: +861065339333
-- Best regards, ----------------------------------- Han Han Quality Engineer Redhat.
Email: hhan@redhat.com Phone: +861065339333
-- Best regards, ----------------------------------- Han Han Quality Engineer Redhat. Email: hhan@redhat.com Phone: +861065339333