Re: [PATCH] networkStartNetworkVirtual: Don't overwrite error in 'err5'

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
> > ---
> >  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

Attachment: signature.asc
Description: PGP signature

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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux