[libvirt] [PATCH] virErrorPreserveLast conversions

[Daniel P. Berrangé]

On Fri, Apr 05, 2019 at 10:42:25AM +0200, Michal Privoznik wrote:
> On 4/3/19 8:34 PM, Syed Humaid wrote:
> > From: Humaid <syedhumaidbinharoon at gmail.com>
> > 
> > Converted few instances of virSaveLastError() to virErrorPreserveLast() as per the newer internal APIs for saving and restoring error reports.
> > 
> 
> Please split this long line.
> 
> > Signed-off-by: Syed Humaid <syedhumaidbinharoon at gmail.com>
> > ---
> >   src/libvirt-domain.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> > index be5b1f6740..fba4d98994 100644
> > --- a/src/libvirt-domain.c
> > +++ b/src/libvirt-domain.c
> > @@ -2894,7 +2894,7 @@ virDomainMigrateVersion2(virDomainPtr domain,
> >                          _("domainMigratePrepare2 did not set uri"));
> >           cancelled = 1;
> >           /* Make sure Finish doesn't overwrite the error */
> > -        orig_err = virSaveLastError();
> > +        virErrorPreserveLast(&orig_err);
> >           goto finish;
> >       }
> >       if (uri_out)
> > @@ -2909,7 +2909,7 @@ virDomainMigrateVersion2(virDomainPtr domain,
> >       /* Perform failed. Make sure Finish doesn't overwrite the error */
> >       if (ret < 0)
> > -        orig_err = virSaveLastError();
> > +        virErrorPreserveLast(&orig_err);
> >       /* If Perform returns < 0, then we need to cancel the VM
> >        * startup on the destination
> > @@ -3100,7 +3100,7 @@ virDomainMigrateVersion3Full(virDomainPtr domain,
> >           virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> >                          _("domainMigratePrepare3 did not set uri"));
> >           cancelled = 1;
> > -        orig_err = virSaveLastError();
> > +        virErrorPreserveLast(&orig_err);
> >           goto finish;
> >       }
> > 
> 
> I like this, but:
> 
> 1) it should be done for the rest of the code too [*]
> 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.

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.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|