Re: [libvirt] [PATCH 5/5] network: centralize check for active network during interface attach

On 04/10/2014 09:19 AM, Laine Stump wrote: > The check for a network being active during interface attach was being > done individually in several places (by both the lxc driver and the > qemu driver), but those places were too specific, leading to it *not* > being checked when allocating a connection/device from a macvtap or > hostdev network. > > This patch puts a single check in networkAllocateActualDevice(), which > is always called before the any network interface is attached to any > type of domain. It also removes all the other now-redundant checks > from the lxc and qemu drivers. > > [Note that prior to the previous 4 patches in this series, it would > have been very cumbersome to apply this current patch, as macvtap and > hostdev networks would be automatically set inactive at each libvirtd > restart (unless they were marked as "autostart"). Therefore, those 4 > patches should be seen as prerequisites to this patch for any > backporting. This comment is a placeholder that I intend to replace > with the git commit id's for those 4 patches as soon as I have pushed > them.] > > This fixes: > > https://bugzilla.redhat.com/show_bug.cgi?id=880483 > --- > src/lxc/lxc_driver.c | 21 +++------------------ > src/lxc/lxc_process.c | 18 ++---------------- > src/network/bridge_driver.c | 10 +++++++++- > src/qemu/qemu_command.c | 18 ++---------------- > src/qemu/qemu_hotplug.c | 11 +---------- > 5 files changed, 17 insertions(+), 61 deletions(-) > This appears to be fine with the caveat mentioned/noted. I'm also assuming that by moving the check to higher in the call/check stack that it's not "too generic" not right? Other 'actualType''s would need that object to be active since networkAllocateActualDevice() will fail otherwise.

ACK for what's here as long as you're comfortable with the active check for all actualType's now... John > diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c > index 942e139..f84b8c3 100644 > --- a/src/lxc/lxc_driver.c > +++ b/src/lxc/lxc_driver.c > @@ -4155,27 +4155,12 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn, > virNetworkPtr network; > char *brname = NULL; > bool fail = false; > - int active; > virErrorPtr errobj; > > - if (!(network = virNetworkLookupByName(conn, > - net->data.network.name))) > + if (!(network = virNetworkLookupByName(conn, net->data.network.name))) > goto cleanup; > - > - active = virNetworkIsActive(network); > - if (active != 1) { > - fail = true; > - if (active == 0) > - virReportError(VIR_ERR_INTERNAL_ERROR, > - _("Network '%s' is not active."), > - net->data.network.name); > - } > - > - if (!fail) { > - brname = virNetworkGetBridgeName(network); > - if (brname == NULL) > - fail = true; > - } > + if (!(brname = virNetworkGetBridgeName(network))) > + fail = true; > > /* Make sure any above failure is preserved */ > errobj = virSaveLastError(); > diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c > index 091102b..0aef13a 100644 > --- a/src/lxc/lxc_process.c > +++ b/src/lxc/lxc_process.c > @@ -387,27 +387,13 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn, > virNetworkPtr network; > char *brname = NULL; > bool fail = false; > - int active; > virErrorPtr errobj; > > if (!(network = virNetworkLookupByName(conn, > def->nets[i]->data.network.name))) > goto cleanup; > - > - active = virNetworkIsActive(network); > - if (active != 1) { > - fail = true; > - if (active == 0) > - virReportError(VIR_ERR_INTERNAL_ERROR, > - _("Network '%s' is not active."), > - def->nets[i]->data.network.name); > - } > - > - if (!fail) { > - brname = virNetworkGetBridgeName(network); > - if (brname == NULL) > - fail = true; > - } > + if (!(brname = virNetworkGetBridgeName(network))) > + fail = true; > > /* Make sure any above failure is preserved */ > errobj = virSaveLastError(); > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c > index 7beab79..132af4f 100644 > --- a/src/network/bridge_driver.c > +++ b/src/network/bridge_driver.c > @@ -3118,7 +3118,8 @@ static int networkDestroy(virNetworkPtr net) > > if (!virNetworkObjIsActive(network)) { > virReportError(VIR_ERR_OPERATION_INVALID, > - "%s", _("network is not active")); > + _("network '%s' is not active"), > + network->def->name); > goto cleanup; > } > > @@ -3462,6 +3463,13 @@ networkAllocateActualDevice(virDomainDefPtr dom, > } > netdef = network->def; > > + if (!virNetworkObjIsActive(network)) { > + virReportError(VIR_ERR_OPERATION_INVALID, > + _("network '%s' is not active"), > + netdef->name); > + goto error; > + } > + > if (VIR_ALLOC(iface->data.network.actual) < 0) > goto error; > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 379c094..baf1ad1 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -297,7 +297,6 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, > virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); > > if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK) { > - int active; > bool fail = false; > virErrorPtr errobj; > virNetworkPtr network = virNetworkLookupByName(conn, > @@ -305,21 +304,8 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, > if (!network) > return ret; > > - active = virNetworkIsActive(network); > - if (active != 1) { > - fail = true; > - > - if (active == 0) > - virReportError(VIR_ERR_INTERNAL_ERROR, > - _("Network '%s' is not active."), > - net->data.network.name); > - } > - > - if (!fail) { > - brname = virNetworkGetBridgeName(network); > - if (brname == NULL) > - fail = true; > - } > + if (!(brname = virNetworkGetBridgeName(network))) > + fail = true; > > /* Make sure any above failure is preserved */ > errobj = virSaveLastError(); > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index 233b183..ccfb358 100644 > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c > @@ -1710,7 +1710,6 @@ qemuDomainNetGetBridgeName(virConnectPtr conn, virDomainNetDefPtr net) > if (VIR_STRDUP(brname, tmpbr) < 0) > goto cleanup; > } else if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK) { > - int active; > virErrorPtr errobj; > virNetworkPtr network; > > @@ -1720,15 +1719,7 @@ qemuDomainNetGetBridgeName(virConnectPtr conn, virDomainNetDefPtr net) > net->data.network.name); > goto cleanup; > } > - > - active = virNetworkIsActive(network); > - if (active == 1) { > - brname = virNetworkGetBridgeName(network); > - } else if (active == 0) { > - virReportError(VIR_ERR_INTERNAL_ERROR, > - _("Network '%s' is not active."), > - net->data.network.name); > - } > + brname = virNetworkGetBridgeName(network); > > /* Make sure any above failure is preserved */ > errobj = virSaveLastError(); >