On Tue, Nov 19, 2019 at 10:31 PM Jamie Strandboge <jamie@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
