virErrorPreserveLast conversions

[libvirt] [PATCH] virErrorPreserveLast conversions

Posted by Syed Humaid 5 years ago

From: Humaid <syedhumaidbinharoon@gmail.com>

Converted few instances of virSaveLastError() to virErrorPreserveLast() as per the newer internal APIs for saving and restoring error reports.

Signed-off-by: Syed Humaid <syedhumaidbinharoon@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;
     }
 
-- 
2.20.1

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

Re: [libvirt] [PATCH] virErrorPreserveLast conversions

Posted by Michal Privoznik 5 years ago

On 4/3/19 8:34 PM, Syed Humaid wrote:
> From: Humaid <syedhumaidbinharoon@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@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.

Michal

* - Unfortunately, we can't just ditch virSaveLastError() because it's a 
public API :-(

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

Re: [libvirt] [PATCH] virErrorPreserveLast conversions

Posted by Daniel P. Berrangé 5 years ago

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

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