[libvirt] [PATCH 11/35] util: file: Add helper to determine whether a path is a CDROM

Peter Krempa posted 35 patches 7 years ago
[libvirt] [PATCH 11/35] util: file: Add helper to determine whether a path is a CDROM
Posted by Peter Krempa 7 years ago
Add detection mechanism which will allow to check whether a path to a
block device is a physical CDROM drive. This will be useful once we will
need to pass it to hypervisors.

The linux implementation uses an ioctl to do the detection, while the
fallback uses a simple string prefix match.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/libvirt_private.syms |  1 +
 src/util/virfile.c       | 56 +++++++++++++++++++++++++++++++++++++++++++++++-
 src/util/virfile.h       |  2 ++
 3 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index d2728749fb..4f911c10a8 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1781,6 +1781,7 @@ virFileGetMountSubtree;
 virFileHasSuffix;
 virFileInData;
 virFileIsAbsPath;
+virFileIsCdrom;
 virFileIsDir;
 virFileIsExecutable;
 virFileIsLink;
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 24f866525f..ad59b7015b 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -59,8 +59,9 @@
 # include <sys/statfs.h>
 # if HAVE_DECL_LO_FLAGS_AUTOCLEAR
 #  include <linux/loop.h>
-#  include <sys/ioctl.h>
 # endif
+# include <sys/ioctl.h>
+# include <linux/cdrom.h>
 #endif

 #include "configmake.h"
@@ -1938,6 +1939,59 @@ int virFileIsMountPoint(const char *file)
 }


+#if defined(__linux__)
+/**
+ * virFileIsCdrom:
+ * @filename: File to check
+ *
+ * Returns 1 if @filename 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 *filename)
+{
+    struct stat st;
+    int fd;
+    int ret = -1;
+
+    if ((fd = open(filename, O_RDONLY | O_NONBLOCK)) < 0)
+        goto cleanup;
+
+    if (fstat(fd, &st) < 0)
+        goto cleanup;
+
+    if (!S_ISBLK(st.st_mode)) {
+        ret = 0;
+        goto cleanup;
+    }
+
+    /* Attempt to detect via a CDROM specific ioctl */
+    if (ioctl(fd, CDROM_DRIVE_STATUS, CDSL_CURRENT) >= 0)
+        ret = 1;
+    else
+        ret = 0;
+
+ cleanup:
+    VIR_FORCE_CLOSE(fd);
+    return ret;
+}
+
+#else
+
+int
+virFileIsCdrom(const char *filename)
+{
+    if (STRPREFIX(filename, "/dev/cd", NULL) ||
+        STRPREFIX(filename, "/dev/acd", NULL))
+        return 1;
+
+    return 0;
+}
+
+#endif /* defined(__linux__) */
+
+
 #if defined HAVE_MNTENT_H && defined HAVE_GETMNTENT_R
 static int
 virFileGetMountSubtreeImpl(const char *mtabpath,
diff --git a/src/util/virfile.h b/src/util/virfile.h
index cd2a3867c2..8ab7d2d6a6 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -207,6 +207,8 @@ 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 *filename)
+    ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;

 int virFileGetMountSubtree(const char *mtabpath,
                            const char *prefix,
-- 
2.16.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 11/35] util: file: Add helper to determine whether a path is a CDROM
Posted by John Ferlan 7 years ago

On 04/25/2018 11:15 AM, Peter Krempa wrote:
> Add detection mechanism which will allow to check whether a path to a
> block device is a physical CDROM drive. This will be useful once we will
> need to pass it to hypervisors.
> 
> The linux implementation uses an ioctl to do the detection, while the
> fallback uses a simple string prefix match.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/libvirt_private.syms |  1 +
>  src/util/virfile.c       | 56 +++++++++++++++++++++++++++++++++++++++++++++++-
>  src/util/virfile.h       |  2 ++
>  3 files changed, 58 insertions(+), 1 deletion(-)
> 

Because it's "easier" to find and it sticks out more, please consider
using CDROM instead Cdrom. You could also change @filename to be @path,
but that's just a nit.

I agree this is better than qemuDomainFilePathIsHostCDROM and should be
used instead. Since it's the same logic as qemu it's a fairly safe bet
for having no issues on all the various platforms w/r/t the ioctl -
famous last words though for all the variants we compile on...

Altering the existing code can be a followup - unless of course you have
the urge to post a followup after this patch or the next one.

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 11/35] util: file: Add helper to determine whether a path is a CDROM
Posted by John Ferlan 7 years ago

On 04/25/2018 11:15 AM, Peter Krempa wrote:
> Add detection mechanism which will allow to check whether a path to a
> block device is a physical CDROM drive. This will be useful once we will
> need to pass it to hypervisors.
> 
> The linux implementation uses an ioctl to do the detection, while the
> fallback uses a simple string prefix match.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/libvirt_private.syms |  1 +
>  src/util/virfile.c       | 56 +++++++++++++++++++++++++++++++++++++++++++++++-
>  src/util/virfile.h       |  2 ++
>  3 files changed, 58 insertions(+), 1 deletion(-)
> 

Should this be a replacement for qemuDomainFilePathIsHostCDROM used for
qemuDomainObjCheckDiskTaint?

Not a problem with this code, but I think there should only be one place
that we determine host CDROM and it doesn't matter to me the mechanism.
Just trying to avoid multiple means to get the same answer.

I'm going to stop here for now as it's late for me - besides the answer
here affects patch 12 too in a way.

John


> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index d2728749fb..4f911c10a8 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1781,6 +1781,7 @@ virFileGetMountSubtree;
>  virFileHasSuffix;
>  virFileInData;
>  virFileIsAbsPath;
> +virFileIsCdrom;
>  virFileIsDir;
>  virFileIsExecutable;
>  virFileIsLink;
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index 24f866525f..ad59b7015b 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -59,8 +59,9 @@
>  # include <sys/statfs.h>
>  # if HAVE_DECL_LO_FLAGS_AUTOCLEAR
>  #  include <linux/loop.h>
> -#  include <sys/ioctl.h>
>  # endif
> +# include <sys/ioctl.h>
> +# include <linux/cdrom.h>
>  #endif
> 
>  #include "configmake.h"
> @@ -1938,6 +1939,59 @@ int virFileIsMountPoint(const char *file)
>  }
> 
> 
> +#if defined(__linux__)
> +/**
> + * virFileIsCdrom:
> + * @filename: File to check
> + *
> + * Returns 1 if @filename 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 *filename)
> +{
> +    struct stat st;
> +    int fd;
> +    int ret = -1;
> +
> +    if ((fd = open(filename, O_RDONLY | O_NONBLOCK)) < 0)
> +        goto cleanup;
> +
> +    if (fstat(fd, &st) < 0)
> +        goto cleanup;
> +
> +    if (!S_ISBLK(st.st_mode)) {
> +        ret = 0;
> +        goto cleanup;
> +    }
> +
> +    /* Attempt to detect via a CDROM specific ioctl */
> +    if (ioctl(fd, CDROM_DRIVE_STATUS, CDSL_CURRENT) >= 0)
> +        ret = 1;
> +    else
> +        ret = 0;
> +
> + cleanup:
> +    VIR_FORCE_CLOSE(fd);
> +    return ret;
> +}
> +
> +#else
> +
> +int
> +virFileIsCdrom(const char *filename)
> +{
> +    if (STRPREFIX(filename, "/dev/cd", NULL) ||
> +        STRPREFIX(filename, "/dev/acd", NULL))
> +        return 1;
> +
> +    return 0;
> +}
> +
> +#endif /* defined(__linux__) */
> +
> +
>  #if defined HAVE_MNTENT_H && defined HAVE_GETMNTENT_R
>  static int
>  virFileGetMountSubtreeImpl(const char *mtabpath,
> diff --git a/src/util/virfile.h b/src/util/virfile.h
> index cd2a3867c2..8ab7d2d6a6 100644
> --- a/src/util/virfile.h
> +++ b/src/util/virfile.h
> @@ -207,6 +207,8 @@ 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 *filename)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
> 
>  int virFileGetMountSubtree(const char *mtabpath,
>                             const char *prefix,
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 11/35] util: file: Add helper to determine whether a path is a CDROM
Posted by Peter Krempa 7 years ago
On Tue, May 01, 2018 at 20:25:09 -0400, John Ferlan wrote:
> 
> 
> On 04/25/2018 11:15 AM, Peter Krempa wrote:
> > Add detection mechanism which will allow to check whether a path to a
> > block device is a physical CDROM drive. This will be useful once we will
> > need to pass it to hypervisors.
> > 
> > The linux implementation uses an ioctl to do the detection, while the
> > fallback uses a simple string prefix match.
> > 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> >  src/libvirt_private.syms |  1 +
> >  src/util/virfile.c       | 56 +++++++++++++++++++++++++++++++++++++++++++++++-
> >  src/util/virfile.h       |  2 ++
> >  3 files changed, 58 insertions(+), 1 deletion(-)
> > 
> 
> Should this be a replacement for qemuDomainFilePathIsHostCDROM used for
> qemuDomainObjCheckDiskTaint?

Very good point. This code is actually "inspired" by the code that qemu
uses for CDROM detection, so I think we should actually use it instead
of the string checks.

> Not a problem with this code, but I think there should only be one place
> that we determine host CDROM and it doesn't matter to me the mechanism.
> Just trying to avoid multiple means to get the same answer.

I agree. If it is deemed that it's okay to do ioctl()s on the cdrom
device for libvirt I'll gladly replace the existing code. The advantage
of the ioctl based code is that it works regardless of the name of the
device.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list