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@xxxxxxxxx> > > 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
