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

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

[Hani Benhabiles]

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;
-- 
1.8.3.2

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

[Michael Tokarev]

Thanks, applied to -trivial.

/mjt

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

[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.

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

[Andreas Färber]

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:

if (ambiguous) {
    error_setg(errp, "Path '%s' is ambiguous.", path);
} else {
    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.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[Hani Benhabiles]

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.

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

[Paolo Bonzini]

Il 25/04/2014 17:51, Andreas Färber ha scritto:
>
> if (ambiguous) {
>     error_setg(errp, "Path '%s' is ambiguous.", path);
> } else {
>     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.

I agree.  I think the only loosening I did was to allow an empty path if 
the required type is not NULL.

Paolo

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

[Paolo Bonzini]

Il 26/04/2014 02:46, Hani Benhabiles ha scritto:
>> 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);
> }
>
>

No, error_set is being phased out.  New errors typically use error_setg 
instead.

Paolo