[libvirt] [PATCH] virsh: Return false if connection or domain is not available
Instead of goto cleanup lable, it will work fine if go to cleanup lable, but it's somewhat waste. And some functions don't check if the "domain == NULL" before trying to free it with virDomainFree(dom), as a result, there is additional error in the log says: error: invalid domain pointer in virDomainFree Which is useless. --- tools/virsh.c | 60 ++++++++++++++++++++++++++++---------------------------- 1 files changed, 30 insertions(+), 30 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 7d849ec..ffb4ced 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5201,10 +5201,10 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, int ret = -1; if (!vshConnectionUsability(ctl, ctl->conn)) - goto out; + return -1; if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) - goto out; + return -1; if (vshCommandOptString(cmd, "path", &path) < 0) goto out; @@ -10519,10 +10519,10 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd) char *xml; if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false; if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) - goto cleanup; + return false; if (vshCommandOptString(cmd, "type", &type) <= 0) goto cleanup; @@ -10633,10 +10633,10 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) unsigned int flags; if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false; if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) - goto cleanup; + return false; if (vshCommandOptString(cmd, "type", &type) <= 0) goto cleanup; @@ -10922,10 +10922,10 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) char *xml; if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false; if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) - goto cleanup; + return false; if (vshCommandOptString(cmd, "source", &source) <= 0) goto cleanup; @@ -11105,10 +11105,10 @@ cmdDetachDisk(vshControl *ctl, const vshCmd *cmd) unsigned int flags; if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false; if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) - goto cleanup; + return false; if (vshCommandOptString(cmd, "target", &target) <= 0) goto cleanup; @@ -11655,11 +11655,11 @@ cmdEdit (vshControl *ctl, const vshCmd *cmd) int flags = VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_INACTIVE; if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false; dom = vshCommandOptDomain (ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false; /* Get the XML configuration of the domain. */ doc = virDomainGetXMLDesc (dom, flags); @@ -11806,11 +11806,11 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd) char *name = NULL; if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false; dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false; if (vshCommandOptString(cmd, "xmlfile", &from) <= 0) buffer = vshStrdup(ctl, "<domainsnapshot/>"); @@ -11902,11 +11902,11 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) virBuffer buf = VIR_BUFFER_INITIALIZER; if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false; dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false; if (vshCommandOptString(cmd, "name", &name) < 0 || vshCommandOptString(cmd, "description", &desc) < 0) { @@ -11995,11 +11995,11 @@ cmdSnapshotCurrent(vshControl *ctl, const vshCmd *cmd) char *xml = NULL; if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false; dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false; current = virDomainHasCurrentSnapshot(dom, 0); if (current < 0) @@ -12079,11 +12079,11 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) struct tm time_info; if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false; dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false; numsnaps = virDomainSnapshotNum(dom, 0); @@ -12186,11 +12186,11 @@ cmdSnapshotDumpXML(vshControl *ctl, const vshCmd *cmd) char *xml = NULL; if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false; dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false; if (vshCommandOptString(cmd, "snapshotname", &name) <= 0) goto cleanup; @@ -12245,11 +12245,11 @@ cmdSnapshotParent(vshControl *ctl, const vshCmd *cmd) xmlXPathContextPtr ctxt = NULL; if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false; dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false; if (vshCommandOptString(cmd, "snapshotname", &name) <= 0) goto cleanup; @@ -12311,11 +12311,11 @@ cmdDomainSnapshotRevert(vshControl *ctl, const vshCmd *cmd) virDomainSnapshotPtr snapshot = NULL; if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false; dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false; if (vshCommandOptString(cmd, "snapshotname", &name) <= 0) goto cleanup; @@ -12364,11 +12364,11 @@ cmdSnapshotDelete(vshControl *ctl, const vshCmd *cmd) unsigned int flags = 0; if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false; dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false; if (vshCommandOptString(cmd, "snapshotname", &name) <= 0) goto cleanup; @@ -12427,11 +12427,11 @@ cmdQemuMonitorCommand(vshControl *ctl, const vshCmd *cmd) bool pad = false; if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false; dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false; while ((opt = vshCommandOptArgv(cmd, opt))) { if (pad) -- 1.7.6
On 08/22/2011 04:55 AM, Osier Yang wrote:
Instead of goto cleanup lable, it will work fine if go to cleanup lable, but it's somewhat waste. And some functions don't check if the "domain == NULL" before trying to free it with virDomainFree(dom), as a result, there is additional error in the log says:
error: invalid domain pointer in virDomainFree
Which is useless. --- tools/virsh.c | 60 ++++++++++++++++++++++++++++---------------------------- 1 files changed, 30 insertions(+), 30 deletions(-)
I had to look at the context of each function touched.
diff --git a/tools/virsh.c b/tools/virsh.c index 7d849ec..ffb4ced 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5201,10 +5201,10 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, int ret = -1;
if (!vshConnectionUsability(ctl, ctl->conn)) - goto out; + return -1;
if (!(dom = vshCommandOptDomain(ctl, cmd,&name))) - goto out; + return -1;
Good, since out: was doing unconditional virDomainFree.
if (vshCommandOptString(cmd, "path",&path)< 0) goto out; @@ -10519,10 +10519,10 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd) char *xml;
if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false;
if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) - goto cleanup; + return false;
Doesn't help, doesn't hurt, since cleanup: was properly doing conditional virDomainFree. I'd almost recommend getting rid of the conditional in cleanup: if we keep this hunk.
if (vshCommandOptString(cmd, "type",&type)<= 0) goto cleanup; @@ -10633,10 +10633,10 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) unsigned int flags;
> if (!vshConnectionUsability(ctl, ctl->conn))
- goto cleanup; + return false;
if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) - goto cleanup; + return false;
Same as cmdAttachInterface - doesn't help, doesn't hurt. If we keep this hunk, then the cleanup hunk should be changed to be unconditional.
if (vshCommandOptString(cmd, "type",&type)<= 0) goto cleanup; @@ -10922,10 +10922,10 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) char *xml;
if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false;
if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) - goto cleanup; + return false;
Same story - doesn't help, doesn't hurt.
if (vshCommandOptString(cmd, "source",&source)<= 0) goto cleanup; @@ -11105,10 +11105,10 @@ cmdDetachDisk(vshControl *ctl, const vshCmd *cmd) unsigned int flags;
if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false;
if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) - goto cleanup; + return false;
Same story - doesn't help, doesn't hurt.
if (vshCommandOptString(cmd, "target",&target)<= 0) goto cleanup; @@ -11655,11 +11655,11 @@ cmdEdit (vshControl *ctl, const vshCmd *cmd) int flags = VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_INACTIVE;
if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false;
dom = vshCommandOptDomain (ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false;
Same story - doesn't help, doesn't hurt.
/* Get the XML configuration of the domain. */ doc = virDomainGetXMLDesc (dom, flags); @@ -11806,11 +11806,11 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd) char *name = NULL;
if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false;
dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false;
Same story - doesn't help, doesn't hurt.
if (vshCommandOptString(cmd, "xmlfile",&from)<= 0) buffer = vshStrdup(ctl, "<domainsnapshot/>"); @@ -11902,11 +11902,11 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) virBuffer buf = VIR_BUFFER_INITIALIZER;
if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false;
dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false;
Same story - doesn't help, doesn't hurt.
if (vshCommandOptString(cmd, "name",&name)< 0 || vshCommandOptString(cmd, "description",&desc)< 0) { @@ -11995,11 +11995,11 @@ cmdSnapshotCurrent(vshControl *ctl, const vshCmd *cmd) char *xml = NULL;
if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false;
dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false;
Same story - doesn't help, doesn't hurt.
current = virDomainHasCurrentSnapshot(dom, 0); if (current< 0) @@ -12079,11 +12079,11 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) struct tm time_info;
if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false;
dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false;
Same story - doesn't help, doesn't hurt.
numsnaps = virDomainSnapshotNum(dom, 0);
@@ -12186,11 +12186,11 @@ cmdSnapshotDumpXML(vshControl *ctl, const vshCmd *cmd) char *xml = NULL;
if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false;
dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false;
Same story - doesn't help, doesn't hurt.
if (vshCommandOptString(cmd, "snapshotname",&name)<= 0) goto cleanup; @@ -12245,11 +12245,11 @@ cmdSnapshotParent(vshControl *ctl, const vshCmd *cmd) xmlXPathContextPtr ctxt = NULL;
if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false;
dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false;
Same story - doesn't help, doesn't hurt.
if (vshCommandOptString(cmd, "snapshotname",&name)<= 0) goto cleanup; @@ -12311,11 +12311,11 @@ cmdDomainSnapshotRevert(vshControl *ctl, const vshCmd *cmd) virDomainSnapshotPtr snapshot = NULL;
if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false;
dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false;
Same story - doesn't help, doesn't hurt.
if (vshCommandOptString(cmd, "snapshotname",&name)<= 0) goto cleanup; @@ -12364,11 +12364,11 @@ cmdSnapshotDelete(vshControl *ctl, const vshCmd *cmd) unsigned int flags = 0;
if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false;
dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false;
Same story - doesn't help, doesn't hurt.
if (vshCommandOptString(cmd, "snapshotname",&name)<= 0) goto cleanup; @@ -12427,11 +12427,11 @@ cmdQemuMonitorCommand(vshControl *ctl, const vshCmd *cmd) bool pad = false;
if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false;
dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false;
Same story - doesn't help, doesn't hurt. End result - your patch has one real bug fix (first hunk), and lots of remaining changes for consistency. I'd rather see a v2 of this, in one of two styles: Style 1: Just one patch: fix blockJobImpl() to rename label from out: to cleanup: to match conventions, and within the cleanup label, make the virDomainFree() conditional. Style 2: First patch: fix blockJobImpl() to get rid of the bug. Second patch: fix all remaining calls to use a consistent style of returning early and not jumping to the cleanup label unless there is something to cleanup; and in the process, make the cleanup label call virDomainFree() unconditionally since no one will jump to the label with a NULL dom. My preference is 70-30 towards the first style, based on a maintainability argument - it is easier to write code that _always_ goes to the cleanup label, so that if you ever introduce new code at the beginning of the function that also causes an early exit, the existing code isn't violating assumptions by doing a return instead of a goto. We've had several bugs in the past where adding new code at the beginning of a function caused a regression due to a later return statement, but I can't think of any regressions caused by adding early code in a function where we consistently used goto cleanup. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
于 2011年08月22日 21:51, Eric Blake 写道:
On 08/22/2011 04:55 AM, Osier Yang wrote:
Instead of goto cleanup lable, it will work fine if go to cleanup lable, but it's somewhat waste. And some functions don't check if the "domain == NULL" before trying to free it with virDomainFree(dom), as a result, there is additional error in the log says:
error: invalid domain pointer in virDomainFree
Which is useless. --- tools/virsh.c | 60 ++++++++++++++++++++++++++++---------------------------- 1 files changed, 30 insertions(+), 30 deletions(-)
I had to look at the context of each function touched.
diff --git a/tools/virsh.c b/tools/virsh.c index 7d849ec..ffb4ced 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5201,10 +5201,10 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, int ret = -1;
if (!vshConnectionUsability(ctl, ctl->conn)) - goto out; + return -1;
if (!(dom = vshCommandOptDomain(ctl, cmd,&name))) - goto out; + return -1;
Good, since out: was doing unconditional virDomainFree.
if (vshCommandOptString(cmd, "path",&path)< 0) goto out; @@ -10519,10 +10519,10 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd) char *xml;
if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false;
if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) - goto cleanup; + return false;
Doesn't help, doesn't hurt, since cleanup: was properly doing conditional virDomainFree. I'd almost recommend getting rid of the conditional in cleanup: if we keep this hunk.
if (vshCommandOptString(cmd, "type",&type)<= 0) goto cleanup; @@ -10633,10 +10633,10 @@ cmdDetachInterface(vshControl *ctl, const vshCmd *cmd) unsigned int flags;
if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false;
if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) - goto cleanup; + return false;
Same as cmdAttachInterface - doesn't help, doesn't hurt. If we keep this hunk, then the cleanup hunk should be changed to be unconditional.
if (vshCommandOptString(cmd, "type",&type)<= 0) goto cleanup; @@ -10922,10 +10922,10 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) char *xml;
if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false;
if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) - goto cleanup; + return false;
Same story - doesn't help, doesn't hurt.
if (vshCommandOptString(cmd, "source",&source)<= 0) goto cleanup; @@ -11105,10 +11105,10 @@ cmdDetachDisk(vshControl *ctl, const vshCmd *cmd) unsigned int flags;
if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false;
if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) - goto cleanup; + return false;
Same story - doesn't help, doesn't hurt.
if (vshCommandOptString(cmd, "target",&target)<= 0) goto cleanup; @@ -11655,11 +11655,11 @@ cmdEdit (vshControl *ctl, const vshCmd *cmd) int flags = VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_INACTIVE;
if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false;
dom = vshCommandOptDomain (ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false;
Same story - doesn't help, doesn't hurt.
/* Get the XML configuration of the domain. */ doc = virDomainGetXMLDesc (dom, flags); @@ -11806,11 +11806,11 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd) char *name = NULL;
if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false;
dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false;
Same story - doesn't help, doesn't hurt.
if (vshCommandOptString(cmd, "xmlfile",&from)<= 0) buffer = vshStrdup(ctl, "<domainsnapshot/>"); @@ -11902,11 +11902,11 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) virBuffer buf = VIR_BUFFER_INITIALIZER;
if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false;
dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false;
Same story - doesn't help, doesn't hurt.
if (vshCommandOptString(cmd, "name",&name)< 0 || vshCommandOptString(cmd, "description",&desc)< 0) { @@ -11995,11 +11995,11 @@ cmdSnapshotCurrent(vshControl *ctl, const vshCmd *cmd) char *xml = NULL;
if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false;
dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false;
Same story - doesn't help, doesn't hurt.
current = virDomainHasCurrentSnapshot(dom, 0); if (current< 0) @@ -12079,11 +12079,11 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) struct tm time_info;
if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false;
dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false;
Same story - doesn't help, doesn't hurt.
numsnaps = virDomainSnapshotNum(dom, 0);
@@ -12186,11 +12186,11 @@ cmdSnapshotDumpXML(vshControl *ctl, const vshCmd *cmd) char *xml = NULL;
if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false;
dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false;
Same story - doesn't help, doesn't hurt.
if (vshCommandOptString(cmd, "snapshotname",&name)<= 0) goto cleanup; @@ -12245,11 +12245,11 @@ cmdSnapshotParent(vshControl *ctl, const vshCmd *cmd) xmlXPathContextPtr ctxt = NULL;
if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false;
dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false;
Same story - doesn't help, doesn't hurt.
if (vshCommandOptString(cmd, "snapshotname",&name)<= 0) goto cleanup; @@ -12311,11 +12311,11 @@ cmdDomainSnapshotRevert(vshControl *ctl, const vshCmd *cmd) virDomainSnapshotPtr snapshot = NULL;
if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false;
dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false;
Same story - doesn't help, doesn't hurt.
if (vshCommandOptString(cmd, "snapshotname",&name)<= 0) goto cleanup; @@ -12364,11 +12364,11 @@ cmdSnapshotDelete(vshControl *ctl, const vshCmd *cmd) unsigned int flags = 0;
if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false;
dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false;
Same story - doesn't help, doesn't hurt.
if (vshCommandOptString(cmd, "snapshotname",&name)<= 0) goto cleanup; @@ -12427,11 +12427,11 @@ cmdQemuMonitorCommand(vshControl *ctl, const vshCmd *cmd) bool pad = false;
if (!vshConnectionUsability(ctl, ctl->conn)) - goto cleanup; + return false;
dom = vshCommandOptDomain(ctl, cmd, NULL); if (dom == NULL) - goto cleanup; + return false;
Same story - doesn't help, doesn't hurt.
End result - your patch has one real bug fix (first hunk), and lots of remaining changes for consistency.
I'd rather see a v2 of this, in one of two styles:
Style 1: Just one patch: fix blockJobImpl() to rename label from out: to cleanup: to match conventions, and within the cleanup label, make the virDomainFree() conditional.
Style 2: First patch: fix blockJobImpl() to get rid of the bug. Second patch: fix all remaining calls to use a consistent style of returning early and not jumping to the cleanup label unless there is something to cleanup; and in the process, make the cleanup label call virDomainFree() unconditionally since no one will jump to the label with a NULL dom.
My preference is 70-30 towards the first style, based on a maintainability argument - it is easier to write code that _always_ goes to the cleanup label, so that if you ever introduce new code at the beginning of the function that also causes an early exit, the existing code isn't violating assumptions by doing a return instead of a goto.
<snip>
We've had several bugs in the past where adding new code at the beginning of a function caused a regression due to a later return statement, but I can't think of any regressions caused by adding early code in a function where we consistently used goto cleanup. </snip>
I don't consider this, make sense, sent a v2 with style 1. Thanks Osier
Without these patch, there will be error like below if domain is NULL. error: invalid domain pointer in virDomainFree Which is useless. --- tools/virsh.c | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 7d849ec..5f7bbcb 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -5201,16 +5201,16 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, int ret = -1; if (!vshConnectionUsability(ctl, ctl->conn)) - goto out; + goto cleanup; if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) - goto out; + goto cleanup; if (vshCommandOptString(cmd, "path", &path) < 0) - goto out; + goto cleanup; if (vshCommandOptUL(cmd, "bandwidth", &bandwidth) < 0) - goto out; + goto cleanup; if (mode == VSH_CMD_BLOCK_JOB_ABORT) ret = virDomainBlockJobAbort(dom, path, 0); @@ -5221,8 +5221,8 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, else if (mode == VSH_CMD_BLOCK_JOB_PULL) ret = virDomainBlockPull(dom, path, bandwidth, 0); -out: - virDomainFree(dom); +cleanup: + if (dom) virDomainFree(dom); return ret; } -- 1.7.6
On 08/22/2011 09:54 PM, Osier Yang wrote:
Without these patch, there will be error like below if domain is NULL.
error: invalid domain pointer in virDomainFree
Which is useless. --- tools/virsh.c | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-)
-out: - virDomainFree(dom); +cleanup: + if (dom) virDomainFree(dom);
Split this into two lines: if (dom) virDomainFree(dom); ACK with that style fix. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
于 2011年08月23日 21:34, Eric Blake 写道:
Without these patch, there will be error like below if domain is NULL.
error: invalid domain pointer in virDomainFree
Which is useless.
Pushed with the style changed. Osier
participants (2)
-
Eric Blake -
Osier Yang