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
>