If there's an error when setting up QoS on a bridge the control
jumps over to 'err5' label. Here, the virNetDevBandwidthClear()
is called to clear out any partially set QoS. This function can
also report an error which would overwrite the actual error that
caused us jumping here. 🤦 Use virErrorPreserveLast() to preserve
the original error.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/network/bridge_driver.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index ce4f4890f1..77206b4584 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -2493,6 +2493,7 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
     return 0;
 
  err5:
+    virErrorPreserveLast(&save_err);
     if (def->bandwidth)
        virNetDevBandwidthClear(def->bridge);
 
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, Apr 23, 2019 at 16:21:49 +0200, Michal Privoznik wrote:
> If there's an error when setting up QoS on a bridge the control
> jumps over to 'err5' label. Here, the virNetDevBandwidthClear()
> is called to clear out any partially set QoS. This function can
> also report an error which would overwrite the actual error that
> caused us jumping here. 🤦 Use virErrorPreserveLast() to preserve

NACK
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Now the proper review:

On Tue, Apr 23, 2019 at 16:21:49 +0200, Michal Privoznik wrote:
> If there's an error when setting up QoS on a bridge the control
> jumps over to 'err5' label. Here, the virNetDevBandwidthClear()
> is called to clear out any partially set QoS. This function can
> also report an error which would overwrite the actual error that
> caused us jumping here. 🤦 Use virErrorPreserveLast() to preserve

Drop the emoji.

> the original error.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/network/bridge_driver.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index ce4f4890f1..77206b4584 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -2493,6 +2493,7 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
>      return 0;
>  
>   err5:
> +    virErrorPreserveLast(&save_err);


You [1] said:

"2) there are several places where we call virSetError() + virFreeError() even
though we've caleld virErrorPreserveLast() earlier. Now, it's not incorrect
(strictly speaking), but it would look much nicer if we called
virErrorRestore() instead. But this is something for a separate patch."

And then Daniel added [2]:

"IMHO it should be part of this patch.  virErrorPreserveLast is intended
to be matched with virErrorRestore, so any conversion that adds the
former, should add the latter in the same method."

networkStartNetworkVirtual uses virSaveLastError() in other cases and
virSetError/virFreeError on the cleanup path. Either convert it before
adding the above call or use the old way.

[1] https://www.redhat.com/archives/libvir-list/2019-April/msg00471.html
[2] https://www.redhat.com/archives/libvir-list/2019-April/msg00473.html

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, Apr 23, 2019 at 17:14:27 +0200, Peter Krempa wrote:
> Now the proper review:
> 
> On Tue, Apr 23, 2019 at 16:21:49 +0200, Michal Privoznik wrote:
> > If there's an error when setting up QoS on a bridge the control
> > jumps over to 'err5' label. Here, the virNetDevBandwidthClear()
> > is called to clear out any partially set QoS. This function can
> > also report an error which would overwrite the actual error that
> > caused us jumping here. 🤦 Use virErrorPreserveLast() to preserve
> 
> Drop the emoji.
> 
> > the original error.
> > 
> > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > ---
> >  src/network/bridge_driver.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> > index ce4f4890f1..77206b4584 100644
> > --- a/src/network/bridge_driver.c
> > +++ b/src/network/bridge_driver.c
> > @@ -2493,6 +2493,7 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
> >      return 0;
> >  
> >   err5:
> > +    virErrorPreserveLast(&save_err);
> 
> 
> You [1] said:
> 
> "2) there are several places where we call virSetError() + virFreeError() even
> though we've caleld virErrorPreserveLast() earlier. Now, it's not incorrect
> (strictly speaking), but it would look much nicer if we called
> virErrorRestore() instead. But this is something for a separate patch."
> 
> And then Daniel added [2]:
> 
> "IMHO it should be part of this patch.  virErrorPreserveLast is intended
> to be matched with virErrorRestore, so any conversion that adds the
> former, should add the latter in the same method."
> 
> networkStartNetworkVirtual uses virSaveLastError() in other cases and
> virSetError/virFreeError on the cleanup path. Either convert it before
> adding the above call or use the old way.

Sorry. I didn't notice the other patch which also fixes this function
which was already pushed.

ACK
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list