[libvirt] [PATCH v2 3/3] virt-aa-helper: drop pointer checks in get_files
Jamie Strandboge
jamie at canonical.com
Tue Nov 19 21:31:48 UTC 2019
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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20191119/398e6a77/attachment-0001.sig>
More information about the libvir-list
mailing list