There are some paths (e.g. /dev/vfio/vfio or /dev/mapper/control)
which are defined in qemu_domain.c and then in qemu_cgroup.c
again. This is suboptimal. Lets move paths into qemu_domain.h and
drop duplicate definitions.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_cgroup.c  | 13 ++++++-------
 src/qemu/qemu_domain.c  | 30 +++++++++++-------------------
 src/qemu/qemu_domain.h  |  7 +++++++
 src/qemu/qemu_hostdev.c |  5 +++--
 4 files changed, 27 insertions(+), 28 deletions(-)

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index ca76c4fdfa..19ca60905a 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -113,8 +113,6 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm,
 }
 
 
-#define DEVICE_MAPPER_CONTROL_PATH "/dev/mapper/control"
-
 static int
 qemuSetupImageCgroupInternal(virDomainObjPtr vm,
                              virStorageSourcePtr src,
@@ -127,8 +125,8 @@ qemuSetupImageCgroupInternal(virDomainObjPtr vm,
     }
 
     if (virStoragePRDefIsManaged(src->pr) &&
-        virFileExists(DEVICE_MAPPER_CONTROL_PATH) &&
-        qemuSetupImagePathCgroup(vm, DEVICE_MAPPER_CONTROL_PATH, false) < 0)
+        virFileExists(QEMU_DEVICE_MAPPER_CONTROL_PATH) &&
+        qemuSetupImagePathCgroup(vm, QEMU_DEVICE_MAPPER_CONTROL_PATH, false) < 0)
         return -1;
 
     return qemuSetupImagePathCgroup(vm, src->path, src->readonly || forceReadonly);
@@ -162,7 +160,7 @@ qemuTeardownImageCgroup(virDomainObjPtr vm,
         return 0;
     }
 
-    if (virFileExists(DEVICE_MAPPER_CONTROL_PATH)) {
+    if (virFileExists(QEMU_DEVICE_MAPPER_CONTROL_PATH)) {
         for (i = 0; i < vm->def->ndisks; i++) {
             virStorageSourcePtr diskSrc = vm->def->disks[i]->src;
 
@@ -176,9 +174,10 @@ qemuTeardownImageCgroup(virDomainObjPtr vm,
         if (i == vm->def->ndisks) {
             VIR_DEBUG("Disabling device mapper control");
             ret = virCgroupDenyDevicePath(priv->cgroup,
-                                          DEVICE_MAPPER_CONTROL_PATH, perms, true);
+                                          QEMU_DEVICE_MAPPER_CONTROL_PATH,
+                                          perms, true);
             virDomainAuditCgroupPath(vm, priv->cgroup, "deny",
-                                     DEVICE_MAPPER_CONTROL_PATH,
+                                     QEMU_DEVICE_MAPPER_CONTROL_PATH,
                                      virCgroupGetDevicePermsString(perms), ret);
             if (ret < 0)
                 return ret;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 0f1335de9c..7e6a9764c5 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -117,14 +117,6 @@ VIR_ENUM_IMPL(qemuDomainNamespace,
               "mount",
 );
 
-
-#define PROC_MOUNTS "/proc/mounts"
-#define DEVPREFIX "/dev/"
-#define DEV_VFIO "/dev/vfio/vfio"
-#define DEVICE_MAPPER_CONTROL_PATH "/dev/mapper/control"
-#define DEV_SEV "/dev/sev"
-
-
 struct _qemuDomainLogContext {
     virObject parent;
 
@@ -11872,7 +11864,7 @@ qemuDomainGetHostdevPath(virDomainDefPtr def,
         tmpPerms[0] = perm;
 
         if (includeVFIO) {
-            if (VIR_STRDUP(tmpPaths[1], DEV_VFIO) < 0)
+            if (VIR_STRDUP(tmpPaths[1], QEMU_DEV_VFIO) < 0)
                 goto cleanup;
             tmpPerms[1] = VIR_CGROUP_DEVICE_RW;
         }
@@ -11919,7 +11911,7 @@ qemuDomainGetPreservedMountPath(virQEMUDriverConfigPtr cfg,
 {
     char *path = NULL;
     char *tmp;
-    const char *suffix = mountpoint + strlen(DEVPREFIX);
+    const char *suffix = mountpoint + strlen(QEMU_DEVPREFIX);
     char *domname = virDomainDefGetShortName(vm->def);
     size_t off;
 
@@ -11974,7 +11966,7 @@ qemuDomainGetPreservedMounts(virQEMUDriverConfigPtr cfg,
     char **paths = NULL, **mounts = NULL;
     size_t i, j, nmounts;
 
-    if (virFileGetMountSubtree(PROC_MOUNTS, "/dev",
+    if (virFileGetMountSubtree(QEMU_PROC_MOUNTS, "/dev",
                                &mounts, &nmounts) < 0)
         goto error;
 
@@ -12099,7 +12091,7 @@ qemuDomainCreateDeviceRecursive(const char *device,
      * Otherwise we might get fooled with `/dev/../var/my_image'.
      * For now, lets hope callers play nice.
      */
-    if (STRPREFIX(device, DEVPREFIX)) {
+    if (STRPREFIX(device, QEMU_DEVPREFIX)) {
         size_t i;
 
         for (i = 0; i < data->ndevMountsPath; i++) {
@@ -12113,7 +12105,7 @@ qemuDomainCreateDeviceRecursive(const char *device,
             /* Okay, @device is in /dev but not in any mount point under /dev.
              * Create it. */
             if (virAsprintf(&devicePath, "%s/%s",
-                            data->path, device + strlen(DEVPREFIX)) < 0)
+                            data->path, device + strlen(QEMU_DEVPREFIX)) < 0)
                 goto cleanup;
 
             if (virFileMakeParentPath(devicePath) < 0) {
@@ -12375,7 +12367,7 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,
 
     /* qemu-pr-helper might require access to /dev/mapper/control. */
     if (disk->src->pr &&
-        qemuDomainCreateDevice(DEVICE_MAPPER_CONTROL_PATH, data, true) < 0)
+        qemuDomainCreateDevice(QEMU_DEVICE_MAPPER_CONTROL_PATH, data, true) < 0)
         goto cleanup;
 
     ret = 0;
@@ -12707,7 +12699,7 @@ qemuDomainSetupLaunchSecurity(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,
 
     VIR_DEBUG("Setting up launch security");
 
-    if (qemuDomainCreateDevice(DEV_SEV, data, false) < 0)
+    if (qemuDomainCreateDevice(QEMU_DEV_SEV, data, false) < 0)
         return -1;
 
     VIR_DEBUG("Set up launch security");
@@ -13122,7 +13114,7 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr driver,
     isReg = S_ISREG(data.sb.st_mode) || S_ISFIFO(data.sb.st_mode) || S_ISSOCK(data.sb.st_mode);
     isDir = S_ISDIR(data.sb.st_mode);
 
-    if ((isReg || isDir) && STRPREFIX(file, DEVPREFIX)) {
+    if ((isReg || isDir) && STRPREFIX(file, QEMU_DEVPREFIX)) {
         cfg = virQEMUDriverGetConfig(driver);
         if (!(target = qemuDomainGetPreservedMountPath(cfg, vm, file)))
             goto cleanup;
@@ -13178,7 +13170,7 @@ qemuDomainAttachDeviceMknodRecursive(virQEMUDriverPtr driver,
     }
 # endif
 
-    if (STRPREFIX(file, DEVPREFIX)) {
+    if (STRPREFIX(file, QEMU_DEVPREFIX)) {
         size_t i;
 
         for (i = 0; i < ndevMountsPath; i++) {
@@ -13287,7 +13279,7 @@ qemuDomainDetachDeviceUnlink(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
     int ret = -1;
     size_t i;
 
-    if (STRPREFIX(file, DEVPREFIX)) {
+    if (STRPREFIX(file, QEMU_DEVPREFIX)) {
         for (i = 0; i < ndevMountsPath; i++) {
             if (STREQ(devMountsPath[i], "/dev"))
                 continue;
@@ -13429,7 +13421,7 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
 
     /* qemu-pr-helper might require access to /dev/mapper/control. */
     if (src->pr &&
-        (VIR_STRDUP(dmPath, DEVICE_MAPPER_CONTROL_PATH) < 0 ||
+        (VIR_STRDUP(dmPath, QEMU_DEVICE_MAPPER_CONTROL_PATH) < 0 ||
          VIR_APPEND_ELEMENT_COPY(paths, npaths, dmPath) < 0))
         goto cleanup;
 
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 5cb4a32c0e..3eea8b0f96 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -223,6 +223,13 @@ struct _qemuDomainUnpluggingDevice {
 };
 
 
+#define QEMU_PROC_MOUNTS "/proc/mounts"
+#define QEMU_DEVPREFIX "/dev/"
+#define QEMU_DEV_VFIO "/dev/vfio/vfio"
+#define QEMU_DEV_SEV "/dev/sev"
+#define QEMU_DEVICE_MAPPER_CONTROL_PATH "/dev/mapper/control"
+
+
 typedef enum {
     QEMU_DOMAIN_NS_MOUNT = 0,
     QEMU_DOMAIN_NS_LAST
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index cc0a4574cd..efa4d62f1f 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -25,6 +25,7 @@
 #include <sys/ioctl.h>
 
 #include "qemu_hostdev.h"
+#include "qemu_domain.h"
 #include "virlog.h"
 #include "virerror.h"
 #include "viralloc.h"
@@ -140,7 +141,7 @@ qemuHostdevHostSupportsPassthroughVFIO(void)
         return false;
 
     /* condition 2 - /dev/vfio/vfio exists */
-    if (!virFileExists("/dev/vfio/vfio"))
+    if (!virFileExists(QEMU_DEV_VFIO))
         return false;
 
     return true;
@@ -343,7 +344,7 @@ qemuHostdevPrepareMediatedDevices(virQEMUDriverPtr driver,
     /* Checking for VFIO only is fine with mdev, as IOMMU isolation is achieved
      * by the physical parent device.
      */
-    supportsVFIO = virFileExists("/dev/vfio/vfio");
+    supportsVFIO = virFileExists(QEMU_DEV_VFIO);
 
     for (i = 0; i < nhostdevs; i++) {
         if (virHostdevIsMdevDevice(hostdevs[i])) {
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, 2019-06-25 at 13:38 +0200, Michal Privoznik wrote:
> There are some paths (e.g. /dev/vfio/vfio or /dev/mapper/control)
> which are defined in qemu_domain.c and then in qemu_cgroup.c
> again. This is suboptimal. Lets move paths into qemu_domain.h and

s/Lets/Let's/

[...]
> -#define PROC_MOUNTS "/proc/mounts"

$ git grep '"/proc/mounts"' | grep -vE '^(po|docs)/'
src/lxc/lxc_container.c:    if (virFileGetMountReverseSubtree("/proc/mounts", prefix,
src/lxc/lxc_container.c:    if (!(procmnt = setmntent("/proc/mounts", "r"))) {
src/qemu/qemu_domain.h:#define QEMU_PROC_MOUNTS "/proc/mounts"
src/util/vircgroup.c:    mounts = fopen("/proc/mounts", "r");
src/util/vircgroupv1.c:    if (!(mounts = fopen("/proc/mounts", "r")))
src/util/vircgroupv2.c:    if (!(mounts = fopen("/proc/mounts", "r")))
src/util/virfile.c:    f = setmntent("/proc/mounts", "r");
src/util/virfile.c:# define PROC_MOUNTS "/proc/mounts"
tests/vircgroupmock.c:    if (STREQ(path, "/proc/mounts")) {
tests/vircgroupmock.c:    } else if (STREQ(path, "/proc/mounts")) {

> -#define DEVPREFIX "/dev/"

Too many instances to include here! But quite a few are spurious.

> -#define DEV_VFIO "/dev/vfio/vfio"

$ git grep '"/dev/vfio/vfio"' | grep -vE '^(po|docs)/'
src/qemu/qemu_domain.h:#define QEMU_DEV_VFIO "/dev/vfio/vfio"
src/security/virt-aa-helper.c:        virBufferAddLit(&buf, "  \"/dev/vfio/vfio\" rw,\n");

> -#define DEVICE_MAPPER_CONTROL_PATH "/dev/mapper/control"

This one's good :)

> -#define DEV_SEV "/dev/sev"

$ git grep '"/dev/sev"' | grep -vE '^(po|docs)/'
src/qemu/qemu_cgroup.c:    ret = virCgroupAllowDevicePath(priv->cgroup, "/dev/sev",
src/qemu/qemu_cgroup.c:    virDomainAuditCgroupPath(vm, priv->cgroup, "allow", "/dev/sev",
src/qemu/qemu_domain.h:#define QEMU_DEV_SEV "/dev/sev"
src/security/security_dac.c:#define DEV_SEV "/dev/sev"


So most of the above are used at least once outside of the QEMU
drivers... Should we have a virpath.h to collect all these defines
under a single roof? And possibly a syntax-check rule to ensure
additional uses of the bare string don't slip back in over time?


Anyway, if you feel like going further down the rabbit hole be my
guest, and I'll happily review the resulting patches. If not, you
can just fix the commit message, pick up this

  Reviewed-by: Andrea Bolognani <abologna@redhat.com>

and move on with your life :)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 7/1/19 4:47 PM, Andrea Bolognani wrote:
> On Tue, 2019-06-25 at 13:38 +0200, Michal Privoznik wrote:
>> There are some paths (e.g. /dev/vfio/vfio or /dev/mapper/control)
>> which are defined in qemu_domain.c and then in qemu_cgroup.c
>> again. This is suboptimal. Lets move paths into qemu_domain.h and
> 
> s/Lets/Let's/
> 
> [...]
>> -#define PROC_MOUNTS "/proc/mounts"
> 
> $ git grep '"/proc/mounts"' | grep -vE '^(po|docs)/'
> src/lxc/lxc_container.c:    if (virFileGetMountReverseSubtree("/proc/mounts", prefix,
> src/lxc/lxc_container.c:    if (!(procmnt = setmntent("/proc/mounts", "r"))) {
> src/qemu/qemu_domain.h:#define QEMU_PROC_MOUNTS "/proc/mounts"
> src/util/vircgroup.c:    mounts = fopen("/proc/mounts", "r");
> src/util/vircgroupv1.c:    if (!(mounts = fopen("/proc/mounts", "r")))
> src/util/vircgroupv2.c:    if (!(mounts = fopen("/proc/mounts", "r")))
> src/util/virfile.c:    f = setmntent("/proc/mounts", "r");
> src/util/virfile.c:# define PROC_MOUNTS "/proc/mounts"
> tests/vircgroupmock.c:    if (STREQ(path, "/proc/mounts")) {
> tests/vircgroupmock.c:    } else if (STREQ(path, "/proc/mounts")) {
> 
>> -#define DEVPREFIX "/dev/"
> 
> Too many instances to include here! But quite a few are spurious.
> 
>> -#define DEV_VFIO "/dev/vfio/vfio"
> 
> $ git grep '"/dev/vfio/vfio"' | grep -vE '^(po|docs)/'
> src/qemu/qemu_domain.h:#define QEMU_DEV_VFIO "/dev/vfio/vfio"
> src/security/virt-aa-helper.c:        virBufferAddLit(&buf, "  \"/dev/vfio/vfio\" rw,\n");
> 
>> -#define DEVICE_MAPPER_CONTROL_PATH "/dev/mapper/control"
> 
> This one's good :)
> 
>> -#define DEV_SEV "/dev/sev"
> 
> $ git grep '"/dev/sev"' | grep -vE '^(po|docs)/'
> src/qemu/qemu_cgroup.c:    ret = virCgroupAllowDevicePath(priv->cgroup, "/dev/sev",
> src/qemu/qemu_cgroup.c:    virDomainAuditCgroupPath(vm, priv->cgroup, "allow", "/dev/sev",
> src/qemu/qemu_domain.h:#define QEMU_DEV_SEV "/dev/sev"
> src/security/security_dac.c:#define DEV_SEV "/dev/sev"
> 
> 
> So most of the above are used at least once outside of the QEMU
> drivers... Should we have a virpath.h to collect all these defines
> under a single roof? And possibly a syntax-check rule to ensure
> additional uses of the bare string don't slip back in over time?

Sounds good. But I'll save it for a follow up patch or something. I
needed this patch because of my NVMe work. Therefore I chose to push
this one.

> 
> 
> Anyway, if you feel like going further down the rabbit hole be my
> guest, and I'll happily review the resulting patches. If not, you
> can just fix the commit message, pick up this
> 
>   Reviewed-by: Andrea Bolognani <abologna@redhat.com>
> 
> and move on with your life :)

Thanks,
Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list