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

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

Posted by Gaurav Agrawal 4 years, 2 months ago

From: GAURAV AGRAWAL <agrawalgaurav@gnome.org>

Signed-off-by: Gaurav Agrawal <agrawalgaurav@gnome.org>
---
 src/network/bridge_driver_linux.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

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);
-- 
2.24.1

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

Posted by Laine Stump 4 years, 2 months ago

On 2/24/20 1:58 PM, Gaurav Agrawal wrote:
> From: GAURAV AGRAWAL <agrawalgaurav@gnome.org>
>
> Signed-off-by: Gaurav Agrawal <agrawalgaurav@gnome.org>
> ---
>   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.

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)

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

Posted by Ján Tomko 4 years, 2 months ago

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@gnome.org>
>>
>>Signed-off-by: Gaurav Agrawal <agrawalgaurav@gnome.org>
>>---
>>  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)
>

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

Posted by Laine Stump 4 years, 2 months ago

On 2/25/20 7:37 AM, Ján Tomko wrote:
> On Mon, Feb 24, 2020 at 10:09:05PM -0500, Laine Stump wrote:
>> On 2/24/20 1:58 PM, Gaurav Agrawal wrote:
>>
>
> 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.

I could have sworn that when I looked last night there were a handful of 
virSaveLastError() calls, and that I looked at one that was paired with 
virSetError(). But when I look now I see that all the virSaveLastError() 
calls remaining are strange "save this for later" type things rather 
than "save this for a second while we clean up the mess".

It looks like jferlan pushed a bunch of patches in Dec 2018 to do all 
the valid replacements, but the item was never removed from the 
bite-sized tasks list (he might have fixed them without even knowing 
about the item on the list). I just went to the wiki to remove it and 
see that Jano has already taken care of it!

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

Yeah, I realized that was probably the case in the middle of the night 
last night, but wasn't at the keyboard to chastise myself. I knew I 
should have just gone to bed instead of sitting down for one last pass...

But anyway the upside is that Guarav got git send-email configured 
properly to send future patches (while we're on the topic of workflow - 
when you send a modified/updated version of a patch, be sure to note 
that in the subject, e.g. with "--subject-prefix="PATCHv2").

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

Posted by Gaurav Agrawal 4 years, 2 months ago

Hi Laine,

I am sure that I did --subject-prefix , not sure why it did not landed.

Now am wondering about this situation do I still need a PATCH-3 or it's
handled ?

Thanks for giving your one last pass!

Best Regards
Gaurav

On Tue, Feb 25, 2020, 22:46 Laine Stump <laine@redhat.com> wrote:

> On 2/25/20 7:37 AM, Ján Tomko wrote:
> > On Mon, Feb 24, 2020 at 10:09:05PM -0500, Laine Stump wrote:
> >> On 2/24/20 1:58 PM, Gaurav Agrawal wrote:
> >>
> >
> > 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.
>
>
> I could have sworn that when I looked last night there were a handful of
> virSaveLastError() calls, and that I looked at one that was paired with
> virSetError(). But when I look now I see that all the virSaveLastError()
> calls remaining are strange "save this for later" type things rather
> than "save this for a second while we clean up the mess".
>
>
> It looks like jferlan pushed a bunch of patches in Dec 2018 to do all
> the valid replacements, but the item was never removed from the
> bite-sized tasks list (he might have fixed them without even knowing
> about the item on the list). I just went to the wiki to remove it and
> see that Jano has already taken care of it!
>
>
> >
> >
> >>
> >> 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.
>
>
> Yeah, I realized that was probably the case in the middle of the night
> last night, but wasn't at the keyboard to chastise myself. I knew I
> should have just gone to bed instead of sitting down for one last pass...
>
>
> But anyway the upside is that Guarav got git send-email configured
> properly to send future patches (while we're on the topic of workflow -
> when you send a modified/updated version of a patch, be sure to note
> that in the subject, e.g. with "--subject-prefix="PATCHv2").
>
>
>
>

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

Posted by Laine Stump 4 years, 2 months ago

On 2/25/20 1:10 PM, Gaurav Agrawal wrote:
> Hi Laine,
> 
> I am sure that I did --subject-prefix , not sure why it did not landed.
> 
> Now am wondering about this situation do I still need a PATCH-3 or it's 
> handled ?

Nah, Jano pointed out what I thought of late last night - this is 
different from the traditional use cases that the task-list wanted to 
have removed - it saves the error to re-use it multiple times in the 
future, so  virErrorRestore() would actually do the *wrong* thing here. 
And since virErrorPreserveLast() is intended to be used in a pair with 
virErrorRestore(), there is actually nothing to do here.

And even beyond that, when I looked at the list of remaining cases of 
virSaveLastError() for more than the 10 seconds I looked last night, I 
see (as Jano pointed out) that all the cases that should be converted, 
already *were* converted.

The only problem is that the person who did the converted didn't know 
that was a task on the bite-sized tasks list, so they didn't remove it.

Jano has now updated the task list to remove stale entries. So maybe you 
can find something else on that list to fix.

Sorry for sending you through all this. On the upside, your .gitconfig 
is now in order :-)


> 
> Thanks for giving your one last pass!
> 
> Best Regards
> Gaurav
> 
> On Tue, Feb 25, 2020, 22:46 Laine Stump <laine@redhat.com 
> <mailto:laine@redhat.com>> wrote:
> 
>     On 2/25/20 7:37 AM, Ján Tomko wrote:
>      > On Mon, Feb 24, 2020 at 10:09:05PM -0500, Laine Stump wrote:
>      >> On 2/24/20 1:58 PM, Gaurav Agrawal wrote:
>      >>
>      >
>      > 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.
> 
> 
>     I could have sworn that when I looked last night there were a
>     handful of
>     virSaveLastError() calls, and that I looked at one that was paired with
>     virSetError(). But when I look now I see that all the
>     virSaveLastError()
>     calls remaining are strange "save this for later" type things rather
>     than "save this for a second while we clean up the mess".
> 
> 
>     It looks like jferlan pushed a bunch of patches in Dec 2018 to do all
>     the valid replacements, but the item was never removed from the
>     bite-sized tasks list (he might have fixed them without even knowing
>     about the item on the list). I just went to the wiki to remove it and
>     see that Jano has already taken care of it!
> 
> 
>      >
>      >
>      >>
>      >> 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.
> 
> 
>     Yeah, I realized that was probably the case in the middle of the night
>     last night, but wasn't at the keyboard to chastise myself. I knew I
>     should have just gone to bed instead of sitting down for one last
>     pass...
> 
> 
>     But anyway the upside is that Guarav got git send-email configured
>     properly to send future patches (while we're on the topic of workflow -
>     when you send a modified/updated version of a patch, be sure to note
>     that in the subject, e.g. with "--subject-prefix="PATCHv2").
> 
> 
>