[PATCH 3/4] qtest: Improve error messages when property can not be set right now

Markus Armbruster posted 4 patches 3 years, 3 months ago
Maintainers: "Gonglei (Arei)" <arei.gonglei@huawei.com>, Laurent Vivier <lvivier@redhat.com>, Amit Shah <amit@kernel.org>, "Michael S. Tsirkin" <mst@redhat.com>, Markus Armbruster <armbru@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, Thomas Huth <thuth@redhat.com>
[PATCH 3/4] qtest: Improve error messages when property can not be set right now
Posted by Markus Armbruster 3 years, 3 months ago
When you try to set qtest property "log" while the qtest object is
active, the error message blames "insufficient permission":

    $ qemu-system-x86_64 -S -display none -nodefaults -monitor stdio -chardev socket,id=chrqt0,path=qtest.socket,server=on,wait=off -object qtest,id=qt0,chardev=chrqt0,log=/dev/null
    QEMU 7.1.50 monitor - type 'help' for more information
    (qemu) qom-set /objects/qt0 log qtest.log
    Error: Insufficient permission to perform this operation

This implies it could work with "sufficient permission".  It can't.
Change the error message to:

    Error: Property 'log' can not be set now

Same for property "chardev".

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 softmmu/qtest.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/softmmu/qtest.c b/softmmu/qtest.c
index f8acef2628..afea7693d0 100644
--- a/softmmu/qtest.c
+++ b/softmmu/qtest.c
@@ -977,7 +977,7 @@ static void qtest_set_log(Object *obj, const char *value, Error **errp)
     QTest *q = QTEST(obj);
 
     if (qtest == q) {
-        error_setg(errp, QERR_PERMISSION_DENIED);
+        error_setg(errp, "Property 'log' can not be set now");
     } else {
         g_free(q->log);
         q->log = g_strdup(value);
@@ -997,7 +997,7 @@ static void qtest_set_chardev(Object *obj, const char *value, Error **errp)
     Chardev *chr;
 
     if (qtest == q) {
-        error_setg(errp, QERR_PERMISSION_DENIED);
+        error_setg(errp, "Property 'chardev' can not be set now");
         return;
     }
 
-- 
2.37.2
Re: [PATCH 3/4] qtest: Improve error messages when property can not be set right now
Posted by Thomas Huth 3 years, 3 months ago
On 12/10/2022 17.38, Markus Armbruster wrote:
> When you try to set qtest property "log" while the qtest object is
> active, the error message blames "insufficient permission":
> 
>      $ qemu-system-x86_64 -S -display none -nodefaults -monitor stdio -chardev socket,id=chrqt0,path=qtest.socket,server=on,wait=off -object qtest,id=qt0,chardev=chrqt0,log=/dev/null
>      QEMU 7.1.50 monitor - type 'help' for more information
>      (qemu) qom-set /objects/qt0 log qtest.log
>      Error: Insufficient permission to perform this operation
> 
> This implies it could work with "sufficient permission".  It can't.
> Change the error message to:
> 
>      Error: Property 'log' can not be set now

Can it be set later? ... if not, that error message is almost as confusing 
as the original one. Maybe it's better to tell the users *when* they can set 
the property?

  Thomas
Re: [PATCH 3/4] qtest: Improve error messages when property can not be set right now
Posted by Markus Armbruster 3 years, 3 months ago
Thomas Huth <thuth@redhat.com> writes:

> On 12/10/2022 17.38, Markus Armbruster wrote:
>> When you try to set qtest property "log" while the qtest object is
>> active, the error message blames "insufficient permission":
>> 
>>      $ qemu-system-x86_64 -S -display none -nodefaults -monitor stdio -chardev socket,id=chrqt0,path=qtest.socket,server=on,wait=off -object qtest,id=qt0,chardev=chrqt0,log=/dev/null
>>      QEMU 7.1.50 monitor - type 'help' for more information
>>      (qemu) qom-set /objects/qt0 log qtest.log
>>      Error: Insufficient permission to perform this operation
>> 
>> This implies it could work with "sufficient permission".  It can't.
>> Change the error message to:
>> 
>>      Error: Property 'log' can not be set now
>
> Can it be set later? ... if not, that error message is almost as confusing 
> as the original one. Maybe it's better to tell the users *when* they can set 
> the property?

The property cannot be set while the object is "active", i.e. global
@qtest points to it.

Right now, @qtest points to the object from completion with
user_creatable_complete() to unparent.

Completion fails when @qtest already points to another object, i.e. only
one object can be complete at any time.

Since Paolo took the trouble to code an unparent method, I assume
unparent can happen.  I can't tell offhand when.

Help!
Re: [PATCH 3/4] qtest: Improve error messages when property can not be set right now
Posted by Markus Armbruster 3 years, 3 months ago
Markus Armbruster <armbru@redhat.com> writes:

> Thomas Huth <thuth@redhat.com> writes:
>
>> On 12/10/2022 17.38, Markus Armbruster wrote:
>>> When you try to set qtest property "log" while the qtest object is
>>> active, the error message blames "insufficient permission":
>>> 
>>>      $ qemu-system-x86_64 -S -display none -nodefaults -monitor stdio -chardev socket,id=chrqt0,path=qtest.socket,server=on,wait=off -object qtest,id=qt0,chardev=chrqt0,log=/dev/null
>>>      QEMU 7.1.50 monitor - type 'help' for more information
>>>      (qemu) qom-set /objects/qt0 log qtest.log
>>>      Error: Insufficient permission to perform this operation
>>> 
>>> This implies it could work with "sufficient permission".  It can't.
>>> Change the error message to:
>>> 
>>>      Error: Property 'log' can not be set now
>>
>> Can it be set later? ... if not, that error message is almost as confusing 
>> as the original one. Maybe it's better to tell the users *when* they can set 
>> the property?
>
> The property cannot be set while the object is "active", i.e. global
> @qtest points to it.
>
> Right now, @qtest points to the object from completion with
> user_creatable_complete() to unparent.
>
> Completion fails when @qtest already points to another object, i.e. only
> one object can be complete at any time.
>
> Since Paolo took the trouble to code an unparent method, I assume
> unparent can happen.  I can't tell offhand when.
>
> Help!

Since users are unlikely to hit this error, the new message feels good
enough to me.  Happy to take rewordings, or to redo the patch so that it
preserves the old message while getting rid of QERR_PERMISSION_DENIED.
Re: [PATCH 3/4] qtest: Improve error messages when property can not be set right now
Posted by David Hildenbrand 3 years, 3 months ago
On 12.10.22 20:05, Thomas Huth wrote:
> On 12/10/2022 17.38, Markus Armbruster wrote:
>> When you try to set qtest property "log" while the qtest object is
>> active, the error message blames "insufficient permission":
>>
>>       $ qemu-system-x86_64 -S -display none -nodefaults -monitor stdio -chardev socket,id=chrqt0,path=qtest.socket,server=on,wait=off -object qtest,id=qt0,chardev=chrqt0,log=/dev/null
>>       QEMU 7.1.50 monitor - type 'help' for more information
>>       (qemu) qom-set /objects/qt0 log qtest.log
>>       Error: Insufficient permission to perform this operation
>>
>> This implies it could work with "sufficient permission".  It can't.
>> Change the error message to:
>>
>>       Error: Property 'log' can not be set now
> 
> Can it be set later? ... if not, that error message is almost as confusing
> as the original one. Maybe it's better to tell the users *when* they can set
> the property?

I assume it's mostly about "This property cannot be set." and "This 
property can no longer be set." ?

-- 
Thanks,

David / dhildenb