Re: [libvirt] [PATCH v3 01/36] network: restrict usage of port management APIs

Okay so I needed to do some studying to understand what's going on in the first part of this series. Just gonna type some notes here: virDomainActualNetDef tracks all the data we need to convert a virNetworkPtr content to a virDomainNetDef <source>. It's only ever filled in for <interface type='network'> when a domain is running. hypervisor drivers fill the data in at VM startup by calling virDomainNetAllocateActualDevice, which hands the DomainNetDefPtr off to bridge_drive.c callbacks, which serialize the net config into virDomainActualNetDef.

One desired end goal of this series is making sure that bridge_driver is not messing with any Domain*Def stuff. The first 10-15 patches here are unwinding some pieces of that before it gets into the real new stuff. On 3/19/19 8:46 AM, Daniel P. Berrangé wrote: > The port allocation APIs are currently called unconditionally for all > types of NIC, but (mostly) only do anything for NICs with type=network. > > The exception is the port allocate API which does some validation even > for NICs with type!=network. Relying on this validation is flawed, > however, since the network driver may not even be installed. IOW virt > drivers must not delegate validation to the network driver for NICs > with type != network.

> > This change allows us to report errors when the virtual network driver > is not registered. > For this and the following patch I don't really follow why we can't put the iface->type == TYPE_NETWORK and the netconn = virGetConnectNetwork() directly into the domain_conf.c bridge_driver callback wrappers like virDomainNetAllocateActualDevice. After the final patch in this series, the open coded netconn lookup and TYPE_NETWORK checks are still there, and the function signature hasn't changed. Am I missing something? > Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; > --- > src/conf/domain_conf.c | 26 ++++++++------ > src/libxl/libxl_domain.c | 6 ++-- > src/libxl/libxl_driver.c | 9 +++-- > src/lxc/lxc_driver.c | 6 ++-- > src/lxc/lxc_process.c | 9 +++-- > src/network/bridge_driver.c | 72 +++++++++++++++++++------------------ > src/qemu/qemu_driver.c | 6 ++-- > src/qemu/qemu_hotplug.c | 17 +++++---- > src/qemu/qemu_process.c | 9 +++-- > 9 files changed, 94 insertions(+), 66 deletions(-) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 504c24b545..b7bb94b90b 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -30187,13 +30187,11 @@ int > virDomainNetAllocateActualDevice(virDomainDefPtr dom, > virDomainNetDefPtr iface) > { > - /* Just silently ignore if network driver isn't present. If something > - * has tried to use a NIC with type=network, other code will already > - * cause an error. This ensures type=bridge doesn't break when > - * network driver is compiled out. > - */ > - if (!netAllocate) > - return 0; > + if (!netAllocate) { > + virReportError(VIR_ERR_NO_SUPPORT, "%s", > + _("Virtual networking driver is not available")); > + return -1; > + } > > return netAllocate(dom, iface); > } > @@ -30223,8 +30221,11 @@ bool > virDomainNetBandwidthChangeAllowed(virDomainNetDefPtr iface, > virNetDevBandwidthPtr newBandwidth) > { > - if (!netBandwidthChangeAllowed) > - return 0; > + if (!netBandwidthChangeAllowed) { > + virReportError(VIR_ERR_NO_SUPPORT, "%s", > + _("Virtual networking driver is not available")); > + return -1; > + } > > return netBandwidthChangeAllowed(iface, newBandwidth); > } > @@ -30233,8 +30234,11 @@ int > virDomainNetBandwidthUpdate(virDomainNetDefPtr iface, > virNetDevBandwidthPtr newBandwidth) > { > - if (!netBandwidthUpdate) > - return 0; > + if (!netBandwidthUpdate) { > + virReportError(VIR_ERR_NO_SUPPORT, "%s", > + _("Virtual networking driver is not available")); > + return -1; > + } > > return netBandwidthUpdate(iface, newBandwidth); > } > diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c > index 287406d323..e2f1b54210 100644 > --- a/src/libxl/libxl_domain.c > +++ b/src/libxl/libxl_domain.c > @@ -903,7 +903,8 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, > > /* cleanup actual device */ > virDomainNetRemoveHostdev(vm->def, net); > - virDomainNetReleaseActualDevice(vm->def, net); > + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) > + virDomainNetReleaseActualDevice(vm->def, net); > } > } > > @@ -1060,7 +1061,8 @@ libxlNetworkPrepareDevices(virDomainDefPtr def) > * network's pool of devices, or resolve bridge device name > * to the one defined in the network definition. > */ > - if (virDomainNetAllocateActualDevice(def, net) < 0) > + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK && > + virDomainNetAllocateActualDevice(def, net) < 0) > return -1; > > actualType = virDomainNetGetActualType(net); > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c > index a9edc8211d..d5cd3fc834 100644 > --- a/src/libxl/libxl_driver.c > +++ b/src/libxl/libxl_driver.c > @@ -3390,7 +3390,8 @@ libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver, > * network's pool of devices, or resolve bridge device name > * to the one defined in the network definition. > */ > - if (virDomainNetAllocateActualDevice(vm->def, net) < 0) > + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK && > + virDomainNetAllocateActualDevice(vm->def, net) < 0) > goto cleanup; > > actualType = virDomainNetGetActualType(net); > @@ -3440,7 +3441,8 @@ libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver, > vm->def->nets[vm->def->nnets++] = net; > } else { > virDomainNetRemoveHostdev(vm->def, net); > - virDomainNetReleaseActualDevice(vm->def, net); > + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) > + virDomainNetReleaseActualDevice(vm->def, net); > } > virObjectUnref(cfg); > return ret; > @@ -3863,7 +3865,8 @@ libxlDomainDetachNetDevice(libxlDriverPrivatePtr driver, > cleanup: > libxl_device_nic_dispose(&nic); > if (!ret) { > - virDomainNetReleaseActualDevice(vm->def, detach); > + if (detach->type == VIR_DOMAIN_NET_TYPE_NETWORK) > + virDomainNetReleaseActualDevice(vm->def, detach); > virDomainNetRemove(vm->def, detachidx); > } > virObjectUnref(cfg); > diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c > index e981f8e901..027c4fd990 100644 > --- a/src/lxc/lxc_driver.c > +++ b/src/lxc/lxc_driver.c > @@ -3834,7 +3834,8 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn, > * network's pool of devices, or resolve bridge device name > * to the one defined in the network definition. > */ > - if (virDomainNetAllocateActualDevice(vm->def, net) < 0) > + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK && > + virDomainNetAllocateActualDevice(vm->def, net) < 0) > return -1; > > actualType = virDomainNetGetActualType(net); > @@ -4388,7 +4389,8 @@ lxcDomainDetachDeviceNetLive(virDomainObjPtr vm, > ret = 0; > cleanup: > if (!ret) { > - virDomainNetReleaseActualDevice(vm->def, detach); > + if (detach->type == VIR_DOMAIN_NET_TYPE_NETWORK) > + virDomainNetReleaseActualDevice(vm->def, detach); > virDomainNetRemove(vm->def, detachidx); > virDomainNetDefFree(detach); > } > diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c > index e0729a24bf..7849aaf5b9 100644 > --- a/src/lxc/lxc_process.c > +++ b/src/lxc/lxc_process.c > @@ -224,7 +224,8 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver, > iface->ifname)); > ignore_value(virNetDevVethDelete(iface->ifname)); > } > - virDomainNetReleaseActualDevice(vm->def, iface); > + if (iface->type == VIR_DOMAIN_NET_TYPE_NETWORK) > + virDomainNetReleaseActualDevice(vm->def, iface); > } > > virDomainConfVMNWFilterTeardown(vm); > @@ -558,7 +559,8 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn, > if (virLXCProcessValidateInterface(net) < 0) > goto cleanup; > > - if (virDomainNetAllocateActualDevice(def, net) < 0) > + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK && > + virDomainNetAllocateActualDevice(def, net) < 0) > goto cleanup; > > type = virDomainNetGetActualType(net); > @@ -637,7 +639,8 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn, > ignore_value(virNetDevOpenvswitchRemovePort( > virDomainNetGetActualBridgeName(iface), > iface->ifname)); > - virDomainNetReleaseActualDevice(def, iface); > + if (iface->type == VIR_DOMAIN_NET_TYPE_NETWORK) > + virDomainNetReleaseActualDevice(def, iface); > } > } > return ret; > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c > index 4d4ab0f375..cf37a16c64 100644 > --- a/src/network/bridge_driver.c > +++ b/src/network/bridge_driver.c > @@ -4371,8 +4371,11 @@ networkAllocateActualDevice(virDomainDefPtr dom, > size_t i; > int ret = -1; > > - if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) > - goto validate; > + if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Expected a interface for a virtual network")); > + goto error; > + } > > virDomainActualNetDefFree(iface->data.network.actual); > iface->data.network.actual = NULL; > @@ -4691,7 +4694,6 @@ networkAllocateActualDevice(virDomainDefPtr dom, > if (virNetDevVPortProfileCheckComplete(virtport, true) < 0) > goto error; > > - validate: > /* make sure that everything now specified for the device is > * actually supported on this type of network. NB: network, > * netdev, and iface->data.network.actual may all be NULL. > @@ -4710,19 +4712,11 @@ networkAllocateActualDevice(virDomainDefPtr dom, > (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE && > virtport && virtport->virtPortType > == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH))) { > - if (netdef) { > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > - _("an interface connecting to network '%s' " > - "is requesting a vlan tag, but that is not " > - "supported for this type of network"), > - netdef->name); > - } else { > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > - _("an interface of type '%s' " > - "is requesting a vlan tag, but that is not " > - "supported for this type of connection"), > - virDomainNetTypeToString(iface->type)); > - } > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("an interface connecting to network '%s' " > + "is requesting a vlan tag, but that is not " > + "supported for this type of network"), > + netdef->name); > goto error; > } > } By dropping the validate label, netdef is always set at this point in the code, so this makes sense. > @@ -4738,22 +4732,20 @@ networkAllocateActualDevice(virDomainDefPtr dom, > } > } > > - if (netdef) { > - netdef->connections++; > + netdef->connections++; > + if (dev) > + dev->connections++; > + /* finally we can call the 'plugged' hook script if any */ > + if (networkRunHook(obj, dom, iface, > + VIR_HOOK_NETWORK_OP_IFACE_PLUGGED, > + VIR_HOOK_SUBOP_BEGIN) < 0) { > + /* adjust for failure */ > + netdef->connections--; > if (dev) > - dev->connections++; > - /* finally we can call the 'plugged' hook script if any */ > - if (networkRunHook(obj, dom, iface, > - VIR_HOOK_NETWORK_OP_IFACE_PLUGGED, > - VIR_HOOK_SUBOP_BEGIN) < 0) { > - /* adjust for failure */ > - netdef->connections--; > - if (dev) > - dev->connections--; > - goto error; > - } > - networkLogAllocation(netdef, actualType, dev, iface, true); > + dev->connections--; > + goto error; > } > + networkLogAllocation(netdef, actualType, dev, iface, true); > This diff is ugly but it's just unindenting an if (netdef) block So by no longer calling this function on != TYPE_NETWORK, we no longer get the bandwidth and vlan option validation for those interfaces. That seems like a minor regression. IMO not enough to hold up applying this but we should remember it for later. I'm pretty sure it can solved with some similar checks added to DomainValidate callbacks Provided there's a reason for not centralizing the TYPE_NETWORK checks: Reviewed-by: Cole Robinson <crobinso(a)redhat.com&gt; And this could go in now IMO Thanks, Cole -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list