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
