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

[Osier Yang <jyang@xxxxxxxxxx>]

于 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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list