[libvirt] [PATCH 0/2] virt-aa-helper for shmem device

Christian Ehrhardt posted 2 patches 4 years, 5 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20191022121858.16871-1-christian.ehrhardt@canonical.com
Test syntax-check passed
There is a newer version of this series
src/security/virt-aa-helper.c | 35 +++++++++++++++++++++++++++++++++++
tests/virt-aa-helper-test     | 15 +++++++++++++++
2 files changed, 50 insertions(+)
[libvirt] [PATCH 0/2] virt-aa-helper for shmem device
Posted by Christian Ehrhardt 4 years, 5 months ago
Cole was recently adding a few of the usual apparmor suspects to BZ 1761645
and I was taking a look at the low hanging fruits of it today. It isn't
perfect, but would resolve the reported issue - so I'd appreciate a
review.

Limitations:
- One could break the path creating elements in qemuBuildShmemBackendMemProps
  and qemuDomainPrepareShmemChardev into extra functions and then use those
  from virt-aa-helper. But I haven't done so yet and unless it is strictly
  required consider it too much for what we want/need to achieve here.
- I haven't covered hotplug of shmem devices yet, it seems there is no
  infrastructure for their labels yet and I wasn't sure how important
  shmem-hotplug would even be to anyone.

Christian Ehrhardt (2):
  virt-aa-helper: add rules for shmem devices
  virt-aa-helper: testcase for shmem devices

 src/security/virt-aa-helper.c | 35 +++++++++++++++++++++++++++++++++++
 tests/virt-aa-helper-test     | 15 +++++++++++++++
 2 files changed, 50 insertions(+)

-- 
2.23.0

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

[libvirt] [PATCH v2 0/3] virt-aa-helper for shmem device
Posted by Christian Ehrhardt 4 years, 4 months ago
Cole was recently adding a few of the usual apparmor suspects to BZ 1761645
and I was taking a look at the low hanging fruits of it today. It isn't
perfect, but would resolve the reported issue - so I'd appreciate a
review.

Limitations:
- One could break the path creating elements in qemuBuildShmemBackendMemProps
  and qemuDomainPrepareShmemChardev into extra functions and then use those
  from virt-aa-helper. But I haven't done so yet and unless it is strictly
  required consider it too much for what we want/need to achieve here.
- I haven't covered hotplug of shmem devices yet, it seems there is no
  infrastructure for their labels yet and I wasn't sure how important
  shmem-hotplug would even be to anyone.

Updates in v2:
- rebase latest master
- drop checking shmems[i]
- switch to use g_strdup_printf
- added an extra patch removing checks like ctl->def->mems[i]
- add reviewed-by tag

Christian Ehrhardt (3):
  virt-aa-helper: add rules for shmem devices
  virt-aa-helper: testcase for shmem devices
  virt-aa-helper: drop pointer checks in get_files

 src/security/virt-aa-helper.c | 166 +++++++++++++++++++---------------
 tests/virt-aa-helper-test     |  15 +++
 2 files changed, 109 insertions(+), 72 deletions(-)

-- 
2.24.0


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

Re: [libvirt] [PATCH v2 0/3] virt-aa-helper for shmem device
Posted by Christian Ehrhardt 4 years, 4 months ago
On Thu, Nov 14, 2019 at 12:20 PM Christian Ehrhardt
<christian.ehrhardt@canonical.com> wrote:
>
> Cole was recently adding a few of the usual apparmor suspects to BZ 1761645
> and I was taking a look at the low hanging fruits of it today. It isn't
> perfect, but would resolve the reported issue - so I'd appreciate a
> review.
>
> Limitations:
> - One could break the path creating elements in qemuBuildShmemBackendMemProps
>   and qemuDomainPrepareShmemChardev into extra functions and then use those
>   from virt-aa-helper. But I haven't done so yet and unless it is strictly
>   required consider it too much for what we want/need to achieve here.
> - I haven't covered hotplug of shmem devices yet, it seems there is no
>   infrastructure for their labels yet and I wasn't sure how important
>   shmem-hotplug would even be to anyone.
>
> Updates in v2:
> - rebase latest master
> - drop checking shmems[i]
> - switch to use g_strdup_printf
> - added an extra patch removing checks like ctl->def->mems[i]
> - add reviewed-by tag

Thanks everyone for the further review and discussion.
Pushing with Ack/Review Tags after a rebuild/recheck


> Christian Ehrhardt (3):
>   virt-aa-helper: add rules for shmem devices
>   virt-aa-helper: testcase for shmem devices
>   virt-aa-helper: drop pointer checks in get_files
>
>  src/security/virt-aa-helper.c | 166 +++++++++++++++++++---------------
>  tests/virt-aa-helper-test     |  15 +++
>  2 files changed, 109 insertions(+), 72 deletions(-)
>
> --
> 2.24.0
>


--
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd


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

[libvirt] [PATCH v2 1/3] virt-aa-helper: add rules for shmem devices
Posted by Christian Ehrhardt 4 years, 4 months ago
Shared memory devices need qemu to be able to access certain paths
either for the shared memory directly (mostly ivshmem-plain) or for a
socket (mostly ivshmem-doorbell).

Add logic to virt-aa-helper to render those apparmor rules based
on the domain configuration.

https://bugzilla.redhat.com/show_bug.cgi?id=1761645

Reviewed-by: Cole Robinson <crobinso@redhat.com>
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---
 src/security/virt-aa-helper.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 5ac9a9eeb8..c6c4bb9bd0 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -929,6 +929,7 @@ get_files(vahControl * ctl)
     int rc = -1;
     size_t i;
     char *uuid;
+    char *mem_path = NULL;
     char uuidstr[VIR_UUID_STRING_BUFLEN];
     bool needsVfio = false, needsvhost = false, needsgl = false;
 
@@ -1192,6 +1193,35 @@ get_files(vahControl * ctl)
         }
     }
 
+    for (i = 0; i < ctl->def->nshmems; i++) {
+        virDomainShmemDef *shmem = ctl->def->shmems[i];
+        /* server path can be on any type and overwrites defaults */
+        if (shmem->server.enabled &&
+            shmem->server.chr.data.nix.path) {
+                if (vah_add_file(&buf, shmem->server.chr.data.nix.path,
+                        "rw") != 0)
+                    goto cleanup;
+        } else {
+            switch (shmem->model) {
+            case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN:
+                /* until exposed, recreate qemuBuildShmemBackendMemProps */
+                mem_path = g_strdup_printf("/dev/shm/%s", shmem->name);
+                break;
+            case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL:
+            case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM:
+                 /* until exposed, recreate qemuDomainPrepareShmemChardev */
+                mem_path = g_strdup_printf("/var/lib/libvirt/shmem-%s-sock",
+                               shmem->name);
+                break;
+            }
+            if (mem_path != NULL) {
+                if (vah_add_file(&buf, mem_path, "rw") != 0)
+                    goto cleanup;
+            }
+        }
+    }
+
+
     if (ctl->def->tpm) {
         char *shortName = NULL;
         const char *tpmpath = NULL;
@@ -1286,6 +1316,7 @@ get_files(vahControl * ctl)
     ctl->files = virBufferContentAndReset(&buf);
 
  cleanup:
+    VIR_FREE(mem_path);
     VIR_FREE(uuid);
     return rc;
 }
-- 
2.24.0


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

[libvirt] [PATCH v2 2/3] virt-aa-helper: testcase for shmem devices
Posted by Christian Ehrhardt 4 years, 4 months ago
Adding build time self tests for basic (deprecated), doorbell and plain mode.

Reviewed-by: Cole Robinson <crobinso@redhat.com>
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---
 tests/virt-aa-helper-test | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test
index 6e674bfe5c..6a6703ecf5 100755
--- a/tests/virt-aa-helper-test
+++ b/tests/virt-aa-helper-test
@@ -384,6 +384,21 @@ testme "0" "dri egl" "-r -u $valid_uuid" "$test_xml" "/dev/dri/testegl1.*rw,$"
 sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</devices>,<graphics type='spice'><gl enable='yes' rendernode='/dev/dri/testegl2'/></graphics></devices>,g" "$template_xml" > "$test_xml"
 testme "0" "dri spice" "-r -u $valid_uuid" "$test_xml" "/dev/dri/testegl2.*rw,$"
 
+sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</devices>,<shmem name='my_shmem0'><model type='ivshmem'/><size unit='M'>4</size></shmem></devices>,g" "$template_xml" > "$test_xml"
+testme "0" "shmem" "-r -u $valid_uuid" "$test_xml" "\"/var/lib/libvirt/shmem-my_shmem0-sock\"\s*rw,$"
+
+sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</devices>,<shmem name='my_shmem0'><model type='ivshmem'/><size unit='M'>4</size><server path='/var/lib/libvirt/ivshmem_socket'/></shmem></devices>,g" "$template_xml" > "$test_xml"
+testme "0" "shmem serverpath" "-r -u $valid_uuid" "$test_xml" "\"/var/lib/libvirt/ivshmem_socket\"\s*rw,$"
+
+sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</devices>,<shmem name='my_shmem0'><model type='ivshmem-plain'/><size unit='M'>4</size></shmem></devices>,g" "$template_xml" > "$test_xml"
+testme "0" "shmem plain" "-r -u $valid_uuid" "$test_xml" "\"/dev/shm/my_shmem0\"\s*rw,$"
+
+sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</devices>,<shmem name='shmem_server'><model type='ivshmem-doorbell'/></shmem></devices>,g" "$template_xml" > "$test_xml"
+testme "0" "shmem doorbell" "-r -u $valid_uuid" "$test_xml" "\"/var/lib/libvirt/shmem-shmem_server-sock\"\s*rw,$"
+
+sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e "s,</devices>,<shmem name='shmem_server'><model type='ivshmem-doorbell'/><server path='/var/lib/libvirt/ivshmem_socket'/></shmem></devices>,g" "$template_xml" > "$test_xml"
+testme "0" "shmem doorbell serverpath" "-r -u $valid_uuid" "$test_xml" "\"/var/lib/libvirt/ivshmem_socket\"\s*rw,$"
+
 testme "0" "help" "-h"
 
 echo "" >$output
-- 
2.24.0


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

[libvirt] [PATCH v2 3/3] virt-aa-helper: drop pointer checks in get_files
Posted by Christian Ehrhardt 4 years, 4 months ago
It was mentioned that the pointers in loops like:
  for (i = 0; i < ctl->def->nserials; i++)
      if (ctl->def->serials[i] ...
will always be !NULL or we would have a much more serious problem.

Simplify the if chains in get_files by dropping these checks.

Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---
 src/security/virt-aa-helper.c | 135 ++++++++++++++++------------------
 1 file changed, 63 insertions(+), 72 deletions(-)

diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index c6c4bb9bd0..17f49a6259 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -965,8 +965,7 @@ get_files(vahControl * ctl)
     }
 
     for (i = 0; i < ctl->def->nserials; i++)
-        if (ctl->def->serials[i] &&
-            (ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
+        if ((ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
              ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV ||
              ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE ||
              ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX ||
@@ -980,8 +979,7 @@ get_files(vahControl * ctl)
                 goto cleanup;
 
     for (i = 0; i < ctl->def->nconsoles; i++)
-        if (ctl->def->consoles[i] &&
-            (ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
+        if ((ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
              ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV ||
              ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE ||
              ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX ||
@@ -993,8 +991,7 @@ get_files(vahControl * ctl)
                 goto cleanup;
 
     for (i = 0; i < ctl->def->nparallels; i++)
-        if (ctl->def->parallels[i] &&
-            (ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
+        if ((ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
              ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV ||
              ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE ||
              ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX ||
@@ -1008,8 +1005,7 @@ get_files(vahControl * ctl)
                 goto cleanup;
 
     for (i = 0; i < ctl->def->nchannels; i++)
-        if (ctl->def->channels[i] &&
-            (ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
+        if ((ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
              ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV ||
              ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE ||
              ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX ||
@@ -1082,76 +1078,74 @@ get_files(vahControl * ctl)
                          "r") != 0)
             goto cleanup;
 
-    for (i = 0; i < ctl->def->nhostdevs; i++)
-        if (ctl->def->hostdevs[i]) {
-            virDomainHostdevDefPtr dev = ctl->def->hostdevs[i];
-            virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb;
-            switch (dev->source.subsys.type) {
-            case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: {
-                virUSBDevicePtr usb =
-                    virUSBDeviceNew(usbsrc->bus, usbsrc->device, NULL);
+    for (i = 0; i < ctl->def->nhostdevs; i++) {
+        virDomainHostdevDefPtr dev = ctl->def->hostdevs[i];
+        virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb;
+        switch (dev->source.subsys.type) {
+        case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: {
+            virUSBDevicePtr usb =
+                virUSBDeviceNew(usbsrc->bus, usbsrc->device, NULL);
 
-                if (usb == NULL)
-                    continue;
-
-                if (virHostdevFindUSBDevice(dev, true, &usb) < 0)
-                    continue;
+            if (usb == NULL)
+                continue;
 
-                rc = virUSBDeviceFileIterate(usb, file_iterate_hostdev_cb, &buf);
-                virUSBDeviceFree(usb);
-                if (rc != 0)
-                    goto cleanup;
-                break;
-            }
+            if (virHostdevFindUSBDevice(dev, true, &usb) < 0)
+                continue;
 
-            case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: {
-                virDomainHostdevSubsysMediatedDevPtr mdevsrc = &dev->source.subsys.u.mdev;
-                switch ((virMediatedDeviceModelType) mdevsrc->model) {
-                    case VIR_MDEV_MODEL_TYPE_VFIO_PCI:
-                    case VIR_MDEV_MODEL_TYPE_VFIO_AP:
-                    case VIR_MDEV_MODEL_TYPE_VFIO_CCW:
-                        needsVfio = true;
-                        break;
-                    case VIR_MDEV_MODEL_TYPE_LAST:
-                    default:
-                        virReportEnumRangeError(virMediatedDeviceModelType,
-                                                mdevsrc->model);
-                        break;
-                }
-                break;
-            }
-
-            case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
-                virPCIDevicePtr pci = virPCIDeviceNew(
-                           dev->source.subsys.u.pci.addr.domain,
-                           dev->source.subsys.u.pci.addr.bus,
-                           dev->source.subsys.u.pci.addr.slot,
-                           dev->source.subsys.u.pci.addr.function);
+            rc = virUSBDeviceFileIterate(usb, file_iterate_hostdev_cb, &buf);
+            virUSBDeviceFree(usb);
+            if (rc != 0)
+                goto cleanup;
+            break;
+        }
 
-                virDomainHostdevSubsysPCIBackendType backend = dev->source.subsys.u.pci.backend;
-                if (backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO ||
-                        backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT) {
+        case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: {
+            virDomainHostdevSubsysMediatedDevPtr mdevsrc = &dev->source.subsys.u.mdev;
+            switch ((virMediatedDeviceModelType) mdevsrc->model) {
+                case VIR_MDEV_MODEL_TYPE_VFIO_PCI:
+                case VIR_MDEV_MODEL_TYPE_VFIO_AP:
+                case VIR_MDEV_MODEL_TYPE_VFIO_CCW:
                     needsVfio = true;
-                }
+                    break;
+                case VIR_MDEV_MODEL_TYPE_LAST:
+                default:
+                    virReportEnumRangeError(virMediatedDeviceModelType,
+                                            mdevsrc->model);
+                    break;
+            }
+            break;
+        }
 
-                if (pci == NULL)
-                    continue;
+        case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
+            virPCIDevicePtr pci = virPCIDeviceNew(
+                       dev->source.subsys.u.pci.addr.domain,
+                       dev->source.subsys.u.pci.addr.bus,
+                       dev->source.subsys.u.pci.addr.slot,
+                       dev->source.subsys.u.pci.addr.function);
+
+            virDomainHostdevSubsysPCIBackendType backend = dev->source.subsys.u.pci.backend;
+            if (backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO ||
+                    backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT) {
+                needsVfio = true;
+            }
 
-                rc = virPCIDeviceFileIterate(pci, file_iterate_pci_cb, &buf);
-                virPCIDeviceFree(pci);
+            if (pci == NULL)
+                continue;
 
-                break;
-            }
+            rc = virPCIDeviceFileIterate(pci, file_iterate_pci_cb, &buf);
+            virPCIDeviceFree(pci);
 
-            default:
-                rc = 0;
-                break;
-            } /* switch */
+            break;
         }
 
+        default:
+            rc = 0;
+            break;
+        } /* switch */
+    }
+
     for (i = 0; i < ctl->def->nfss; i++) {
-        if (ctl->def->fss[i] &&
-                ctl->def->fss[i]->type == VIR_DOMAIN_FS_TYPE_MOUNT &&
+        if (ctl->def->fss[i]->type == VIR_DOMAIN_FS_TYPE_MOUNT &&
                 (ctl->def->fss[i]->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_PATH ||
                  ctl->def->fss[i]->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_DEFAULT) &&
                 ctl->def->fss[i]->src) {
@@ -1166,16 +1160,14 @@ get_files(vahControl * ctl)
     }
 
     for (i = 0; i < ctl->def->ninputs; i++) {
-        if (ctl->def->inputs[i] &&
-                ctl->def->inputs[i]->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH) {
+        if (ctl->def->inputs[i]->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH) {
             if (vah_add_file(&buf, ctl->def->inputs[i]->source.evdev, "rw") != 0)
                 goto cleanup;
         }
     }
 
     for (i = 0; i < ctl->def->nnets; i++) {
-        if (ctl->def->nets[i] &&
-                ctl->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER &&
+        if (ctl->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER &&
                 ctl->def->nets[i]->data.vhostuser) {
             virDomainChrSourceDefPtr vhu = ctl->def->nets[i]->data.vhostuser;
 
@@ -1186,8 +1178,7 @@ get_files(vahControl * ctl)
     }
 
     for (i = 0; i < ctl->def->nmems; i++) {
-        if (ctl->def->mems[i] &&
-                ctl->def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
+        if (ctl->def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
             if (vah_add_file(&buf, ctl->def->mems[i]->nvdimmPath, "rw") != 0)
                 goto cleanup;
         }
-- 
2.24.0


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

Re: [libvirt] [PATCH v2 3/3] virt-aa-helper: drop pointer checks in get_files
Posted by Cole Robinson 4 years, 4 months ago
On 11/14/19 6:20 AM, Christian Ehrhardt wrote:
> It was mentioned that the pointers in loops like:
>   for (i = 0; i < ctl->def->nserials; i++)
>       if (ctl->def->serials[i] ...
> will always be !NULL or we would have a much more serious problem.
> 
> Simplify the if chains in get_files by dropping these checks.
> 
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> ---
>  src/security/virt-aa-helper.c | 135 ++++++++++++++++------------------
>  1 file changed, 63 insertions(+), 72 deletions(-)
> 

Reviewed-by: Cole Robinson <crobinso@redhat.com>

- Cole

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

Re: [libvirt] [PATCH v2 3/3] virt-aa-helper: drop pointer checks in get_files
Posted by Jamie Strandboge 4 years, 4 months ago
On Thu, 14 Nov 2019, Christian Ehrhardt wrote:

> It was mentioned that the pointers in loops like:
>   for (i = 0; i < ctl->def->nserials; i++)
>       if (ctl->def->serials[i] ...
> will always be !NULL or we would have a much more serious problem.
> 
> Simplify the if chains in get_files by dropping these checks.

I don't really see why a NULL check is a problem so this feels a bit
like busy work and it seems short-circuiting and not doing is exactly
what you would want to do in the event of a 'much more serious problem'.
Is this really required? +0 to apply on principle (I've not reviewed the
changes closely).

> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> ---
>  src/security/virt-aa-helper.c | 135 ++++++++++++++++------------------
>  1 file changed, 63 insertions(+), 72 deletions(-)
> 
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index c6c4bb9bd0..17f49a6259 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -965,8 +965,7 @@ get_files(vahControl * ctl)
>      }
>  
>      for (i = 0; i < ctl->def->nserials; i++)
> -        if (ctl->def->serials[i] &&
> -            (ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
> +        if ((ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
>               ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV ||
>               ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE ||
>               ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX ||
> @@ -980,8 +979,7 @@ get_files(vahControl * ctl)
>                  goto cleanup;
>  
>      for (i = 0; i < ctl->def->nconsoles; i++)
> -        if (ctl->def->consoles[i] &&
> -            (ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
> +        if ((ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
>               ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV ||
>               ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE ||
>               ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX ||
> @@ -993,8 +991,7 @@ get_files(vahControl * ctl)
>                  goto cleanup;
>  
>      for (i = 0; i < ctl->def->nparallels; i++)
> -        if (ctl->def->parallels[i] &&
> -            (ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
> +        if ((ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
>               ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV ||
>               ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE ||
>               ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX ||
> @@ -1008,8 +1005,7 @@ get_files(vahControl * ctl)
>                  goto cleanup;
>  
>      for (i = 0; i < ctl->def->nchannels; i++)
> -        if (ctl->def->channels[i] &&
> -            (ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
> +        if ((ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
>               ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV ||
>               ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE ||
>               ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX ||
> @@ -1082,76 +1078,74 @@ get_files(vahControl * ctl)
>                           "r") != 0)
>              goto cleanup;
>  
> -    for (i = 0; i < ctl->def->nhostdevs; i++)
> -        if (ctl->def->hostdevs[i]) {
> -            virDomainHostdevDefPtr dev = ctl->def->hostdevs[i];
> -            virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb;
> -            switch (dev->source.subsys.type) {
> -            case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: {
> -                virUSBDevicePtr usb =
> -                    virUSBDeviceNew(usbsrc->bus, usbsrc->device, NULL);
> +    for (i = 0; i < ctl->def->nhostdevs; i++) {
> +        virDomainHostdevDefPtr dev = ctl->def->hostdevs[i];
> +        virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb;
> +        switch (dev->source.subsys.type) {
> +        case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: {
> +            virUSBDevicePtr usb =
> +                virUSBDeviceNew(usbsrc->bus, usbsrc->device, NULL);
>  
> -                if (usb == NULL)
> -                    continue;
> -
> -                if (virHostdevFindUSBDevice(dev, true, &usb) < 0)
> -                    continue;
> +            if (usb == NULL)
> +                continue;
>  
> -                rc = virUSBDeviceFileIterate(usb, file_iterate_hostdev_cb, &buf);
> -                virUSBDeviceFree(usb);
> -                if (rc != 0)
> -                    goto cleanup;
> -                break;
> -            }
> +            if (virHostdevFindUSBDevice(dev, true, &usb) < 0)
> +                continue;
>  
> -            case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: {
> -                virDomainHostdevSubsysMediatedDevPtr mdevsrc = &dev->source.subsys.u.mdev;
> -                switch ((virMediatedDeviceModelType) mdevsrc->model) {
> -                    case VIR_MDEV_MODEL_TYPE_VFIO_PCI:
> -                    case VIR_MDEV_MODEL_TYPE_VFIO_AP:
> -                    case VIR_MDEV_MODEL_TYPE_VFIO_CCW:
> -                        needsVfio = true;
> -                        break;
> -                    case VIR_MDEV_MODEL_TYPE_LAST:
> -                    default:
> -                        virReportEnumRangeError(virMediatedDeviceModelType,
> -                                                mdevsrc->model);
> -                        break;
> -                }
> -                break;
> -            }
> -
> -            case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
> -                virPCIDevicePtr pci = virPCIDeviceNew(
> -                           dev->source.subsys.u.pci.addr.domain,
> -                           dev->source.subsys.u.pci.addr.bus,
> -                           dev->source.subsys.u.pci.addr.slot,
> -                           dev->source.subsys.u.pci.addr.function);
> +            rc = virUSBDeviceFileIterate(usb, file_iterate_hostdev_cb, &buf);
> +            virUSBDeviceFree(usb);
> +            if (rc != 0)
> +                goto cleanup;
> +            break;
> +        }
>  
> -                virDomainHostdevSubsysPCIBackendType backend = dev->source.subsys.u.pci.backend;
> -                if (backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO ||
> -                        backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT) {
> +        case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: {
> +            virDomainHostdevSubsysMediatedDevPtr mdevsrc = &dev->source.subsys.u.mdev;
> +            switch ((virMediatedDeviceModelType) mdevsrc->model) {
> +                case VIR_MDEV_MODEL_TYPE_VFIO_PCI:
> +                case VIR_MDEV_MODEL_TYPE_VFIO_AP:
> +                case VIR_MDEV_MODEL_TYPE_VFIO_CCW:
>                      needsVfio = true;
> -                }
> +                    break;
> +                case VIR_MDEV_MODEL_TYPE_LAST:
> +                default:
> +                    virReportEnumRangeError(virMediatedDeviceModelType,
> +                                            mdevsrc->model);
> +                    break;
> +            }
> +            break;
> +        }
>  
> -                if (pci == NULL)
> -                    continue;
> +        case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
> +            virPCIDevicePtr pci = virPCIDeviceNew(
> +                       dev->source.subsys.u.pci.addr.domain,
> +                       dev->source.subsys.u.pci.addr.bus,
> +                       dev->source.subsys.u.pci.addr.slot,
> +                       dev->source.subsys.u.pci.addr.function);
> +
> +            virDomainHostdevSubsysPCIBackendType backend = dev->source.subsys.u.pci.backend;
> +            if (backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO ||
> +                    backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT) {
> +                needsVfio = true;
> +            }
>  
> -                rc = virPCIDeviceFileIterate(pci, file_iterate_pci_cb, &buf);
> -                virPCIDeviceFree(pci);
> +            if (pci == NULL)
> +                continue;
>  
> -                break;
> -            }
> +            rc = virPCIDeviceFileIterate(pci, file_iterate_pci_cb, &buf);
> +            virPCIDeviceFree(pci);
>  
> -            default:
> -                rc = 0;
> -                break;
> -            } /* switch */
> +            break;
>          }
>  
> +        default:
> +            rc = 0;
> +            break;
> +        } /* switch */
> +    }
> +
>      for (i = 0; i < ctl->def->nfss; i++) {
> -        if (ctl->def->fss[i] &&
> -                ctl->def->fss[i]->type == VIR_DOMAIN_FS_TYPE_MOUNT &&
> +        if (ctl->def->fss[i]->type == VIR_DOMAIN_FS_TYPE_MOUNT &&
>                  (ctl->def->fss[i]->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_PATH ||
>                   ctl->def->fss[i]->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_DEFAULT) &&
>                  ctl->def->fss[i]->src) {
> @@ -1166,16 +1160,14 @@ get_files(vahControl * ctl)
>      }
>  
>      for (i = 0; i < ctl->def->ninputs; i++) {
> -        if (ctl->def->inputs[i] &&
> -                ctl->def->inputs[i]->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH) {
> +        if (ctl->def->inputs[i]->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH) {
>              if (vah_add_file(&buf, ctl->def->inputs[i]->source.evdev, "rw") != 0)
>                  goto cleanup;
>          }
>      }
>  
>      for (i = 0; i < ctl->def->nnets; i++) {
> -        if (ctl->def->nets[i] &&
> -                ctl->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER &&
> +        if (ctl->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER &&
>                  ctl->def->nets[i]->data.vhostuser) {
>              virDomainChrSourceDefPtr vhu = ctl->def->nets[i]->data.vhostuser;
>  
> @@ -1186,8 +1178,7 @@ get_files(vahControl * ctl)
>      }
>  
>      for (i = 0; i < ctl->def->nmems; i++) {
> -        if (ctl->def->mems[i] &&
> -                ctl->def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
> +        if (ctl->def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
>              if (vah_add_file(&buf, ctl->def->mems[i]->nvdimmPath, "rw") != 0)
>                  goto cleanup;
>          }
> -- 
> 2.24.0
> 
-- 
Jamie Strandboge             | http://www.canonical.com
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/3] virt-aa-helper: drop pointer checks in get_files
Posted by Cole Robinson 4 years, 4 months ago
On 11/19/19 4:31 PM, Jamie Strandboge wrote:
> On Thu, 14 Nov 2019, Christian Ehrhardt wrote:
> 
>> It was mentioned that the pointers in loops like:
>>   for (i = 0; i < ctl->def->nserials; i++)
>>       if (ctl->def->serials[i] ...
>> will always be !NULL or we would have a much more serious problem.
>>
>> Simplify the if chains in get_files by dropping these checks.
> 
> I don't really see why a NULL check is a problem so this feels a bit
> like busy work and it seems short-circuiting and not doing is exactly
> what you would want to do in the event of a 'much more serious problem'.
> Is this really required? +0 to apply on principle (I've not reviewed the
> changes closely).
> 

If for example def->nserials and def->serials[i] disagree, then there is
a serious bug somewhere. The internal API contract is that it should
never happen, so code shouldn't check for the condition. I'm pretty sure
if the XML parser is creating that situation, the qemu driver would
crash in a dozen different places.

So the patch isn't strictly required, but it is an improvement IMO: it
reduces code, improves readability, and helps simplify review for other
libvirt devs who may be confused by this uncommon idiom (I was). But I
will leave it up to you guys to decide whether to push or not

Thanks,
Cole

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

Re: [libvirt] [PATCH v2 3/3] virt-aa-helper: drop pointer checks in get_files
Posted by Jamie Strandboge 4 years, 4 months ago
On Wed, 20 Nov 2019, Cole Robinson wrote:

> On 11/19/19 4:31 PM, Jamie Strandboge wrote:
> > On Thu, 14 Nov 2019, Christian Ehrhardt wrote:
> > 
> >> It was mentioned that the pointers in loops like:
> >>   for (i = 0; i < ctl->def->nserials; i++)
> >>       if (ctl->def->serials[i] ...
> >> will always be !NULL or we would have a much more serious problem.
> >>
> >> Simplify the if chains in get_files by dropping these checks.
> > 
> > I don't really see why a NULL check is a problem so this feels a bit
> > like busy work and it seems short-circuiting and not doing is exactly
> > what you would want to do in the event of a 'much more serious problem'.
> > Is this really required? +0 to apply on principle (I've not reviewed the
> > changes closely).
> > 
> 
> If for example def->nserials and def->serials[i] disagree, then there is
> a serious bug somewhere. The internal API contract is that it should
> never happen, so code shouldn't check for the condition. I'm pretty sure
> if the XML parser is creating that situation, the qemu driver would
> crash in a dozen different places.
> 
> So the patch isn't strictly required, but it is an improvement IMO: it
> reduces code, improves readability, and helps simplify review for other
> libvirt devs who may be confused by this uncommon idiom (I was). But I
> will leave it up to you guys to decide whether to push or not

To be clear, I am intentionally not blocking on this. If, as upstream,
you'd prefer this to be a certain way for reasons that outweigh my POV,
by all means feel free to do that. The changes are mechanical and IMHO
don't need an apparmor-focused review (though if you'd prefer I go
through the full patch, I can).

-- 
Jamie Strandboge             | http://www.canonical.com
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 3/3] virt-aa-helper: drop pointer checks in get_files
Posted by Cole Robinson 4 years, 4 months ago
On 11/20/19 12:25 PM, Jamie Strandboge wrote:
> On Wed, 20 Nov 2019, Cole Robinson wrote:
> 
>> On 11/19/19 4:31 PM, Jamie Strandboge wrote:
>>> On Thu, 14 Nov 2019, Christian Ehrhardt wrote:
>>>
>>>> It was mentioned that the pointers in loops like:
>>>>   for (i = 0; i < ctl->def->nserials; i++)
>>>>       if (ctl->def->serials[i] ...
>>>> will always be !NULL or we would have a much more serious problem.
>>>>
>>>> Simplify the if chains in get_files by dropping these checks.
>>>
>>> I don't really see why a NULL check is a problem so this feels a bit
>>> like busy work and it seems short-circuiting and not doing is exactly
>>> what you would want to do in the event of a 'much more serious problem'.
>>> Is this really required? +0 to apply on principle (I've not reviewed the
>>> changes closely).
>>>
>>
>> If for example def->nserials and def->serials[i] disagree, then there is
>> a serious bug somewhere. The internal API contract is that it should
>> never happen, so code shouldn't check for the condition. I'm pretty sure
>> if the XML parser is creating that situation, the qemu driver would
>> crash in a dozen different places.
>>
>> So the patch isn't strictly required, but it is an improvement IMO: it
>> reduces code, improves readability, and helps simplify review for other
>> libvirt devs who may be confused by this uncommon idiom (I was). But I
>> will leave it up to you guys to decide whether to push or not
> 
> To be clear, I am intentionally not blocking on this. If, as upstream,
> you'd prefer this to be a certain way for reasons that outweigh my POV,
> by all means feel free to do that. The changes are mechanical and IMHO
> don't need an apparmor-focused review (though if you'd prefer I go
> through the full patch, I can).
> 

This patch moves the code to be more consistent with the rest of the
libvirt code base, so I think it's good to push. I thought Christian was
waiting on you for review before pushing apparmor patches though, I
didn't want to step on any toes :)

Thanks,
Cole

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

Re: [libvirt] [PATCH v2 3/3] virt-aa-helper: drop pointer checks in get_files
Posted by Christian Ehrhardt 4 years, 4 months ago
On Tue, Nov 19, 2019 at 10:31 PM Jamie Strandboge <jamie@canonical.com> wrote:
>
> On Thu, 14 Nov 2019, Christian Ehrhardt wrote:
>
> > It was mentioned that the pointers in loops like:
> >   for (i = 0; i < ctl->def->nserials; i++)
> >       if (ctl->def->serials[i] ...
> > will always be !NULL or we would have a much more serious problem.
> >
> > Simplify the if chains in get_files by dropping these checks.
>
> I don't really see why a NULL check is a problem so this feels a bit
> like busy work and it seems short-circuiting and not doing is exactly
> what you would want to do in the event of a 'much more serious problem'.
> Is this really required? +0 to apply on principle (I've not reviewed the
> changes closely).

Well Cole brought it up and I intentionally wanted to have the
discussion separate to the shmem changes.

As I said in the mail on patch 2/2 in v1 of this I consider it
"defensive programming" and not strictly required to be removed.
I'm happy and fine to keep the checks as-is since they are not on a
performance critical path.
I agree as you say "in the event of a 'much more serious problem'"
this would be exactly what we want.

I'll withdraw this patch from the series then unless somebody really
shows a reason to why we should drop the checks.

> > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> > ---
> >  src/security/virt-aa-helper.c | 135 ++++++++++++++++------------------
> >  1 file changed, 63 insertions(+), 72 deletions(-)
> >
> > diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> > index c6c4bb9bd0..17f49a6259 100644
> > --- a/src/security/virt-aa-helper.c
> > +++ b/src/security/virt-aa-helper.c
> > @@ -965,8 +965,7 @@ get_files(vahControl * ctl)
> >      }
> >
> >      for (i = 0; i < ctl->def->nserials; i++)
> > -        if (ctl->def->serials[i] &&
> > -            (ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
> > +        if ((ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
> >               ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV ||
> >               ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE ||
> >               ctl->def->serials[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX ||
> > @@ -980,8 +979,7 @@ get_files(vahControl * ctl)
> >                  goto cleanup;
> >
> >      for (i = 0; i < ctl->def->nconsoles; i++)
> > -        if (ctl->def->consoles[i] &&
> > -            (ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
> > +        if ((ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
> >               ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV ||
> >               ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE ||
> >               ctl->def->consoles[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX ||
> > @@ -993,8 +991,7 @@ get_files(vahControl * ctl)
> >                  goto cleanup;
> >
> >      for (i = 0; i < ctl->def->nparallels; i++)
> > -        if (ctl->def->parallels[i] &&
> > -            (ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
> > +        if ((ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
> >               ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV ||
> >               ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE ||
> >               ctl->def->parallels[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX ||
> > @@ -1008,8 +1005,7 @@ get_files(vahControl * ctl)
> >                  goto cleanup;
> >
> >      for (i = 0; i < ctl->def->nchannels; i++)
> > -        if (ctl->def->channels[i] &&
> > -            (ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
> > +        if ((ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_PTY ||
> >               ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_DEV ||
> >               ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_FILE ||
> >               ctl->def->channels[i]->source->type == VIR_DOMAIN_CHR_TYPE_UNIX ||
> > @@ -1082,76 +1078,74 @@ get_files(vahControl * ctl)
> >                           "r") != 0)
> >              goto cleanup;
> >
> > -    for (i = 0; i < ctl->def->nhostdevs; i++)
> > -        if (ctl->def->hostdevs[i]) {
> > -            virDomainHostdevDefPtr dev = ctl->def->hostdevs[i];
> > -            virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb;
> > -            switch (dev->source.subsys.type) {
> > -            case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: {
> > -                virUSBDevicePtr usb =
> > -                    virUSBDeviceNew(usbsrc->bus, usbsrc->device, NULL);
> > +    for (i = 0; i < ctl->def->nhostdevs; i++) {
> > +        virDomainHostdevDefPtr dev = ctl->def->hostdevs[i];
> > +        virDomainHostdevSubsysUSBPtr usbsrc = &dev->source.subsys.u.usb;
> > +        switch (dev->source.subsys.type) {
> > +        case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: {
> > +            virUSBDevicePtr usb =
> > +                virUSBDeviceNew(usbsrc->bus, usbsrc->device, NULL);
> >
> > -                if (usb == NULL)
> > -                    continue;
> > -
> > -                if (virHostdevFindUSBDevice(dev, true, &usb) < 0)
> > -                    continue;
> > +            if (usb == NULL)
> > +                continue;
> >
> > -                rc = virUSBDeviceFileIterate(usb, file_iterate_hostdev_cb, &buf);
> > -                virUSBDeviceFree(usb);
> > -                if (rc != 0)
> > -                    goto cleanup;
> > -                break;
> > -            }
> > +            if (virHostdevFindUSBDevice(dev, true, &usb) < 0)
> > +                continue;
> >
> > -            case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: {
> > -                virDomainHostdevSubsysMediatedDevPtr mdevsrc = &dev->source.subsys.u.mdev;
> > -                switch ((virMediatedDeviceModelType) mdevsrc->model) {
> > -                    case VIR_MDEV_MODEL_TYPE_VFIO_PCI:
> > -                    case VIR_MDEV_MODEL_TYPE_VFIO_AP:
> > -                    case VIR_MDEV_MODEL_TYPE_VFIO_CCW:
> > -                        needsVfio = true;
> > -                        break;
> > -                    case VIR_MDEV_MODEL_TYPE_LAST:
> > -                    default:
> > -                        virReportEnumRangeError(virMediatedDeviceModelType,
> > -                                                mdevsrc->model);
> > -                        break;
> > -                }
> > -                break;
> > -            }
> > -
> > -            case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
> > -                virPCIDevicePtr pci = virPCIDeviceNew(
> > -                           dev->source.subsys.u.pci.addr.domain,
> > -                           dev->source.subsys.u.pci.addr.bus,
> > -                           dev->source.subsys.u.pci.addr.slot,
> > -                           dev->source.subsys.u.pci.addr.function);
> > +            rc = virUSBDeviceFileIterate(usb, file_iterate_hostdev_cb, &buf);
> > +            virUSBDeviceFree(usb);
> > +            if (rc != 0)
> > +                goto cleanup;
> > +            break;
> > +        }
> >
> > -                virDomainHostdevSubsysPCIBackendType backend = dev->source.subsys.u.pci.backend;
> > -                if (backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO ||
> > -                        backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT) {
> > +        case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: {
> > +            virDomainHostdevSubsysMediatedDevPtr mdevsrc = &dev->source.subsys.u.mdev;
> > +            switch ((virMediatedDeviceModelType) mdevsrc->model) {
> > +                case VIR_MDEV_MODEL_TYPE_VFIO_PCI:
> > +                case VIR_MDEV_MODEL_TYPE_VFIO_AP:
> > +                case VIR_MDEV_MODEL_TYPE_VFIO_CCW:
> >                      needsVfio = true;
> > -                }
> > +                    break;
> > +                case VIR_MDEV_MODEL_TYPE_LAST:
> > +                default:
> > +                    virReportEnumRangeError(virMediatedDeviceModelType,
> > +                                            mdevsrc->model);
> > +                    break;
> > +            }
> > +            break;
> > +        }
> >
> > -                if (pci == NULL)
> > -                    continue;
> > +        case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: {
> > +            virPCIDevicePtr pci = virPCIDeviceNew(
> > +                       dev->source.subsys.u.pci.addr.domain,
> > +                       dev->source.subsys.u.pci.addr.bus,
> > +                       dev->source.subsys.u.pci.addr.slot,
> > +                       dev->source.subsys.u.pci.addr.function);
> > +
> > +            virDomainHostdevSubsysPCIBackendType backend = dev->source.subsys.u.pci.backend;
> > +            if (backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_VFIO ||
> > +                    backend == VIR_DOMAIN_HOSTDEV_PCI_BACKEND_DEFAULT) {
> > +                needsVfio = true;
> > +            }
> >
> > -                rc = virPCIDeviceFileIterate(pci, file_iterate_pci_cb, &buf);
> > -                virPCIDeviceFree(pci);
> > +            if (pci == NULL)
> > +                continue;
> >
> > -                break;
> > -            }
> > +            rc = virPCIDeviceFileIterate(pci, file_iterate_pci_cb, &buf);
> > +            virPCIDeviceFree(pci);
> >
> > -            default:
> > -                rc = 0;
> > -                break;
> > -            } /* switch */
> > +            break;
> >          }
> >
> > +        default:
> > +            rc = 0;
> > +            break;
> > +        } /* switch */
> > +    }
> > +
> >      for (i = 0; i < ctl->def->nfss; i++) {
> > -        if (ctl->def->fss[i] &&
> > -                ctl->def->fss[i]->type == VIR_DOMAIN_FS_TYPE_MOUNT &&
> > +        if (ctl->def->fss[i]->type == VIR_DOMAIN_FS_TYPE_MOUNT &&
> >                  (ctl->def->fss[i]->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_PATH ||
> >                   ctl->def->fss[i]->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_DEFAULT) &&
> >                  ctl->def->fss[i]->src) {
> > @@ -1166,16 +1160,14 @@ get_files(vahControl * ctl)
> >      }
> >
> >      for (i = 0; i < ctl->def->ninputs; i++) {
> > -        if (ctl->def->inputs[i] &&
> > -                ctl->def->inputs[i]->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH) {
> > +        if (ctl->def->inputs[i]->type == VIR_DOMAIN_INPUT_TYPE_PASSTHROUGH) {
> >              if (vah_add_file(&buf, ctl->def->inputs[i]->source.evdev, "rw") != 0)
> >                  goto cleanup;
> >          }
> >      }
> >
> >      for (i = 0; i < ctl->def->nnets; i++) {
> > -        if (ctl->def->nets[i] &&
> > -                ctl->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER &&
> > +        if (ctl->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER &&
> >                  ctl->def->nets[i]->data.vhostuser) {
> >              virDomainChrSourceDefPtr vhu = ctl->def->nets[i]->data.vhostuser;
> >
> > @@ -1186,8 +1178,7 @@ get_files(vahControl * ctl)
> >      }
> >
> >      for (i = 0; i < ctl->def->nmems; i++) {
> > -        if (ctl->def->mems[i] &&
> > -                ctl->def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
> > +        if (ctl->def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) {
> >              if (vah_add_file(&buf, ctl->def->mems[i]->nvdimmPath, "rw") != 0)
> >                  goto cleanup;
> >          }
> > --
> > 2.24.0
> >
> --
> Jamie Strandboge             | http://www.canonical.com



--
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd


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