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

[Osier Yang]

于 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