Re: [libvirt] [PATCH] virErrorPreserveLast/virErrorRestore conversions
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(a)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