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

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.