[libvirt] [PATCH] virErrorPreserveLast/virErrorRestore conversions

Cole Robinson crobinso at redhat.com
Mon Apr 15 23:55:53 UTC 2019


Thanks for the patch! The code changes look good (with some minor
comments below). The commit message needs work though. Some good
guidelines are: https://chris.beams.io/posts/git-commit/

The subject should mention roughly what file it's touching. git log
tools/virsh-domain.c will give some examples. Since you are specifically
just converting one file, I'd reference it in the subject.

The bits in the content about responding to previous review comments
don't belong in the message that ends up in git. If you want them in the
mail, but not in git, you can put them after the --- break, before the
diffstat. Also since this is v2 of the patch you should have v2 in the
subject. 'git format-patch -v2' will do that. Here's a suggested commit
message (keep in mind I'm not that great at them either):

  virsh-domain: Convert to virErrorRestore

  Replace all virSaveLastError usage in virsh-domain.c to use
  virErrorPreserveLast and virErrorRestore


On 4/6/19 3:45 AM, Syed Humaid wrote:
> As per the suggestions regarding the previous patch for virErrorPreserveLast
> conversions, I have converted all instances to virErrorPreserveLast and
> virErrorRestore.
> 
> Signed-off-by: Syed Humaid <syedhumaidbinharoon at gmail.com>
> 
> 

Generally the signoff is the last line of the commit but there's an
extra newline here. I think git strips on commit it but it looks weird
in the mail

---
>  src/libvirt-domain.c | 26 +++++++++++---------------
>  1 file changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index be5b1f6740..8eebb60a2e 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
> @@ -2929,10 +2929,8 @@ virDomainMigrateVersion2(virDomainPtr domain,
>          VIR_ERROR(_("finish step ignored that migration was cancelled"));
>  
>   done:
> -    if (orig_err) {
> -        virSetError(orig_err);
> -        virFreeError(orig_err);
> -    }
> +    virErrorRestore(&orig_err);
> +

Generally you shouldn't mix whitespace changes in with other patch
changes. Since there wasn't a newline here before, don't add one now.
Applies to the other changes too.

So fix those up and send a v3 and I think it's good to go :)

- Cole




More information about the libvir-list mailing list