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

On 2/24/20 1:58 PM, Gaurav Agrawal wrote: >From: GAURAV AGRAWAL <agrawalgaurav(a)gnome.org&gt; > >Signed-off-by: Gaurav Agrawal <agrawalgaurav(a)gnome.org&gt; >--- > 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_virError... 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.

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().

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)