8:51 a.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org