Re: [libvirt] [PATCH 2/2] network: better log message when network is inactive during reconnect

On 04/26/2017 02:39 PM, John Ferlan wrote: > > > On 04/25/2017 12:34 PM, Laine Stump wrote: >> If the network isn't active during networkNotifyActualDevice(), we >> would log an error message stating that the bridge device didn't >> exist. This patch adds a check to see if the network is active, making >> the logs more useful in the case that it isn't. >> >> Partially resolves: https://bugzilla.redhat.com/1442700 >> --- >> src/network/bridge_driver.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c >> index d2d8557..e06f81b 100644 >> --- a/src/network/bridge_driver.c >> +++ b/src/network/bridge_driver.c >> @@ -4676,6 +4676,13 @@ networkNotifyActualDevice(virDomainDefPtr dom, >> } >> netdef = network->def; >> >> + if (!virNetworkObjIsActive(network)) { >> + virReportError(VIR_ERR_OPERATION_INVALID, >> + _("network '%s' is not active"), >> + netdef->name); >> + goto error; >> + } >> + > > /me wonders whether this should just a goto cleanup - IOW: if the > network isn't active, so what, why error. Once someone attempts to start > it, they'll get errors I assume... > > Not that goto error or cleanup matters since commit id '4fee4e0' changed > the goto cleanup to goto error and added: > > + > +error: > + goto cleanup; That's missing the point of that commit. "cleanup:" is there only as a place for error: to goto the cleanup code that is common to both the "success:" exit and the "error:" exit (and the three labels are all there so that that this function is structured similarly to networkAllocateActualDevice() - I wanted them to be as close to the same as possible). In the body of the function you either declare the allocate/notification a success (with "goto success") in which case the connect count for the network goes up, or you declare it a failure (with "goto error") in which case the connect count for the network remains unchanged, but you never directly goto the noncommittal "cleanup:".

> > I guess I don't have the answer readily available in my head as to how > much of the subsequent code would be called at network start time? None of this is called when the network is started - we have no way for the networkStart() function to learn which interfaces in which domains are supposed to be connected to a particular network. That needs more infrastructure change than I wanted to put in right now (we need some sort of event or callback that will allow the network driver to notify all active hypervisor drivers whenever a network is started (or destroyed?), giving the hypervisor driver a chance to call networkNotifyActualDevice() for any affected domain interface). So have I explained myself well enough?

> > John > >> /* if we're restarting libvirtd after an upgrade from a version >> * that didn't save bridge name in actualNetDef for >> * actualType==network, we need to copy it in so that it will be >> >