[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] virsh: Return false if connection or domain is not available



于 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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]