Re: [Qemu-trivial] [PATCH] qmp: Remove unused variable.

[Hani Benhabiles <kroosec@gmail.com>]

On Fri, Apr 25, 2014 at 05:51:33PM +0200, Andreas Färber wrote:
> Am 25.04.2014 17:40, schrieb Luiz Capitulino:
> > On Tue, 22 Apr 2014 22:44:03 +0100
> > Hani Benhabiles <kroosec@gmail.com> wrote:
> > 
> >> Signed-off-by: Hani Benhabiles <hani@linux.com>
> >> ---
> >>  qmp.c | 3 +--
> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/qmp.c b/qmp.c
> >> index 87a28f7..44a6e17 100644
> >> --- a/qmp.c
> >> +++ b/qmp.c
> >> @@ -194,11 +194,10 @@ void qmp_system_wakeup(Error **errp)
> >>  ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp)
> >>  {
> >>      Object *obj;
> >> -    bool ambiguous = false;
> >>      ObjectPropertyInfoList *props = NULL;
> >>      ObjectProperty *prop;
> >>  
> >> -    obj = object_resolve_path(path, &ambiguous);
> >> +    obj = object_resolve_path(path, NULL);
> >>      if (obj == NULL) {
> >>          error_set(errp, QERR_DEVICE_NOT_FOUND, path);
> >>          return NULL;
> > 
> > I'm under the impression that this check in object_resolve_partial_path():
> > 
> >         if (ambiguous && *ambiguous) {
> >             return NULL;
> >         }
> > 
> > Uses 'ambiguous' internally. In that case, this change could have a side effect.
> > 
> > But I'm not sure, I think it would be good to get a reviewed-by from
> > a QOM expert.
> 
> Your understanding matches mine. I would propose to instead use the
> variable in error reporting:
> 

Luiz, Andreas, thanks for spotting this.

On second thought, I agree that this has the side effect of listing the last
found object with that path value, though the preivous behaviour too is
suboptimal as it reports that no devices are found at that path.

> if (ambiguous) {
>     error_setg(errp, "Path '%s' is ambiguous.", path);
> } else {
>     error_set(errp, QERR_DEVICE_NOT_FOUND, path);
> }
> 

Probably, you meant something like this:

if (ambiguous) {
    error_set(errp, QERR_AMBIGUOUS_PATH, path);
} else if (!obj) {
    error_set(errp, QERR_DEVICE_NOT_FOUND, path);
}


> My reasoning is that an ambiguous path might deliver unpredictable
> results (whichever it encounters first), across QEMU versions at least.
> 
> But I'd like to hear Paolo's opinion, too, since he was involved in
> loosening requirements on paths.
> 

Is this documented somewhere ? Searching the mailing-list archives doesn't show
something concrete. Paolo ?

Quick grepping shows only one out of 9 other object_resolve_path() calls
checking for path ambiguity.