Re: [PATCH] network: bridge_driver: Use new helpers for storing libvirt errors

[Ján Tomko <jtomko@xxxxxxxxxx>]

On Mon, Feb 24, 2020 at 10:09:05PM -0500, Laine Stump wrote:
> On 2/24/20 1:58 PM, Gaurav Agrawal wrote:
> > From: GAURAV AGRAWAL <agrawalgaurav@xxxxxxxxx>
> >
> > Signed-off-by: Gaurav Agrawal <agrawalgaurav@xxxxxxxxx>
> > ---
> >  src/network/bridge_driver_linux.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
>
>
> Yay! It applied properly, so you have all the pieces of patch 
> submission properly in line! :-)
>
>
> (Welcome to the neighborhood, BTW.)
>
>
> > diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c
> > index 7bbde5c6a9..fde33b5d38 100644
> > --- a/src/network/bridge_driver_linux.c
> > +++ b/src/network/bridge_driver_linux.c
> > @@ -22,6 +22,7 @@
> >  #include <config.h>
> >  #include "viralloc.h"
> > +#include "virerror.h"
> >  #include "virfile.h"
> >  #include "viriptables.h"
> >  #include "virstring.h"
> > @@ -53,7 +54,7 @@ static void networkSetupPrivateChains(void)
> >      if (rc < 0) {
> >          VIR_DEBUG("Failed to create global IPv4 chains: %s",
> >                    virGetLastErrorMessage());
> > -        errInitV4 = virSaveLastError();
> > +        virErrorPreserveLast(&errInitV4);
> >          virResetLastError();
> >      } else {
> >          virFreeError(errInitV4);
> > @@ -70,7 +71,7 @@ static void networkSetupPrivateChains(void)
> >      if (rc < 0) {
> >          VIR_DEBUG("Failed to create global IPv6 chains: %s",
> >                    virGetLastErrorMessage());
> > -        errInitV6 = virSaveLastError();
> > +        virErrorPreserveLast(&errInitV6);
> >          virResetLastError();
> >      } else {
> >          virFreeError(errInitV6);
>
>
> For anyone who didn't notice, this patch is for one of the "bite sized 
> tasks" here:
>
> https://wiki.libvirt.org/page/BiteSizedTasks#More_conversions_to_virErrorPreserveLast.2FvirErrorRestore
>
>
> This is a very strange case of virSaveLastError() / virSetError() to 
> pick - usually they are in pairs within the same function, but in this 
> case when the original error occurs during the driver init, it is 
> squanched away in the static errInitV[46] with virSaveLastError(), and 
> later, in a completely different context, whenever something tries to 
> add a firewall rule of the given type, *then* it sets the error (with 
> virSetError()) to what was earlier encountered during init.

Yes, networkSetupPrivateChains is only called once (via virOnce, as
suggested by the comment on the top of the function) on initialization
and if either IPv4 or IPv6 chains could not be created, it sets the
dirver-global error, which is then called on any subsequent attempt
to use it.

So this is not really a case that needs to be converted.
In fact, glancing at git gre virSetError it seems we already got rid
of all the ones worth converting.

> Your patch has replaced the virSaveLastError() of the earlier part 
> with virErrorPreserveLast(), but hasn't replaced the virSetError() of 
> the later part (which is down in networkAddFirewallRules()) with 
> virErrorRestore().

virErrorRestore resets the error, which is not what we want here -
any subsequent calls should report the same error we caught when
initializing.

Jano

> I can make those two minor changes and push if you like, or you're 
> free to send again with --subject-prefix="PATCHv2". (Or, maybe an even 
> better idea - you could expand the patch to replace all the other uses 
> of virSaveLastError()/virSetError()). The choice is yours :-)
>
>
> (Oh, and BTW, no need to Cc: crobinso - he'll see it anyway)

Attachment: signature.asc
Description: PGP signature