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

[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. 
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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org