[PATCH 0/4] Replace QERR_PERMISSION_DENIED by better error messages

[PATCH 0/4] Replace QERR_PERMISSION_DENIED by better error messages

[Markus Armbruster]

Markus Armbruster (4):
  qom: Improve error messages when property has no getter or setter
  backends: Improve error messages when property can no longer be set
  qtest: Improve error messages when property can not be set right now
  qerror: QERR_PERMISSION_DENIED is no longer used, drop

 include/qapi/qmp/qerror.h       | 3 ---
 backends/cryptodev-vhost-user.c | 2 +-
 backends/rng-egd.c              | 2 +-
 backends/rng-random.c           | 2 +-
 backends/vhost-user.c           | 2 +-
 qom/object.c                    | 6 ++++--
 softmmu/qtest.c                 | 4 ++--
 7 files changed, 10 insertions(+), 11 deletions(-)

-- 
2.37.2

[PATCH 1/4] qom: Improve error messages when property has no getter or setter

[Markus Armbruster]

When you try to set a property that has no setter, the error message
blames "insufficient permission":

    $ qemu-system-x86_64 -S -display none -nodefaults -monitor stdio
    QEMU 7.1.50 monitor - type 'help' for more information
    (qemu) qom-set /machine type q35
    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 'pc-i440fx-7.2-machine.type' is not writable

Do the same for getting a property that has no getter.

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

diff --git a/qom/object.c b/qom/object.c
index d34608558e..e5cef30f6d 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1383,7 +1383,8 @@ bool object_property_get(Object *obj, const char *name, Visitor *v,
     }
 
     if (!prop->get) {
-        error_setg(errp, QERR_PERMISSION_DENIED);
+        error_setg(errp, "Property '%s.%s' is not readable",
+                   object_get_typename(obj), name);
         return false;
     }
     prop->get(obj, v, name, prop->opaque, &err);
@@ -1402,7 +1403,8 @@ bool object_property_set(Object *obj, const char *name, Visitor *v,
     }
 
     if (!prop->set) {
-        error_setg(errp, QERR_PERMISSION_DENIED);
+        error_setg(errp, "Property '%s.%s' is not writable",
+                   object_get_typename(obj), name);
         return false;
     }
     prop->set(obj, v, name, prop->opaque, errp);
-- 
2.37.2

Re: [PATCH 1/4] qom: Improve error messages when property has no getter or setter

[David Hildenbrand]

On 12.10.22 17:37, Markus Armbruster wrote:
> When you try to set a property that has no setter, the error message
> blames "insufficient permission":
> 
>      $ qemu-system-x86_64 -S -display none -nodefaults -monitor stdio
>      QEMU 7.1.50 monitor - type 'help' for more information
>      (qemu) qom-set /machine type q35
>      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 'pc-i440fx-7.2-machine.type' is not writable
> 
> Do the same for getting a property that has no getter.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   qom/object.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/qom/object.c b/qom/object.c
> index d34608558e..e5cef30f6d 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1383,7 +1383,8 @@ bool object_property_get(Object *obj, const char *name, Visitor *v,
>       }
>   
>       if (!prop->get) {
> -        error_setg(errp, QERR_PERMISSION_DENIED);
> +        error_setg(errp, "Property '%s.%s' is not readable",
> +                   object_get_typename(obj), name);
>           return false;
>       }
>       prop->get(obj, v, name, prop->opaque, &err);
> @@ -1402,7 +1403,8 @@ bool object_property_set(Object *obj, const char *name, Visitor *v,
>       }
>   
>       if (!prop->set) {
> -        error_setg(errp, QERR_PERMISSION_DENIED);
> +        error_setg(errp, "Property '%s.%s' is not writable",
> +                   object_get_typename(obj), name);
>           return false;
>       }
>       prop->set(obj, v, name, prop->opaque, errp);

Much better!

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

[PATCH 2/4] backends: Improve error messages when property can no longer be set

[Markus Armbruster]

When you try to set virtio-rng property "filename" after the backend
has been completed with user_creatable_complete(), the error message
blames "insufficient permission":

    $ qemu-system-x86_64 -S -display none -nodefaults -monitor stdio -object rng-random,id=rng0 -device virtio-rng,id=vrng0,rng=rng0
    QEMU 7.1.50 monitor - type 'help' for more information
    (qemu) qom-set /objects/rng0 filename /dev/random
    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 'filename' can no longer be set

Same for cryptodev-vhost-user property "chardev", rng-egd property
"chardev", and vhost-user-backend property "chardev".

Signed-off-by: Markus Armbruster <armbru@redhat.com>

# This is the commit message #2:

# fixup! backends: Improve error messages when property can no longer be set
---
 backends/cryptodev-vhost-user.c | 2 +-
 backends/rng-egd.c              | 2 +-
 backends/rng-random.c           | 2 +-
 backends/vhost-user.c           | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/backends/cryptodev-vhost-user.c b/backends/cryptodev-vhost-user.c
index 5443a59153..f9c5867e38 100644
--- a/backends/cryptodev-vhost-user.c
+++ b/backends/cryptodev-vhost-user.c
@@ -339,7 +339,7 @@ static void cryptodev_vhost_user_set_chardev(Object *obj,
                       CRYPTODEV_BACKEND_VHOST_USER(obj);
 
     if (s->opened) {
-        error_setg(errp, QERR_PERMISSION_DENIED);
+        error_setg(errp, "Property 'chardev' can no longer be set");
     } else {
         g_free(s->chr_name);
         s->chr_name = g_strdup(value);
diff --git a/backends/rng-egd.c b/backends/rng-egd.c
index 4de142b9dc..684c3cf3d6 100644
--- a/backends/rng-egd.c
+++ b/backends/rng-egd.c
@@ -116,7 +116,7 @@ static void rng_egd_set_chardev(Object *obj, const char *value, Error **errp)
     RngEgd *s = RNG_EGD(b);
 
     if (b->opened) {
-        error_setg(errp, QERR_PERMISSION_DENIED);
+        error_setg(errp, "Property 'chardev' can no longer be set");
     } else {
         g_free(s->chr_name);
         s->chr_name = g_strdup(value);
diff --git a/backends/rng-random.c b/backends/rng-random.c
index 7add272edd..80eb5be138 100644
--- a/backends/rng-random.c
+++ b/backends/rng-random.c
@@ -96,7 +96,7 @@ static void rng_random_set_filename(Object *obj, const char *filename,
     RngRandom *s = RNG_RANDOM(obj);
 
     if (b->opened) {
-        error_setg(errp, QERR_PERMISSION_DENIED);
+        error_setg(errp, "Property 'filename' can no longer be set");
         return;
     }
 
diff --git a/backends/vhost-user.c b/backends/vhost-user.c
index 10b39992d2..5dedb2d987 100644
--- a/backends/vhost-user.c
+++ b/backends/vhost-user.c
@@ -141,7 +141,7 @@ static void set_chardev(Object *obj, const char *value, Error **errp)
     Chardev *chr;
 
     if (b->completed) {
-        error_setg(errp, QERR_PERMISSION_DENIED);
+        error_setg(errp, "Property 'chardev' can no longer be set");
         return;
     }
 
-- 
2.37.2

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

[Markus Armbruster]

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

[Thomas Huth]

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

[David Hildenbrand]

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

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

[Markus Armbruster]

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

[Markus Armbruster]

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.

[PATCH 4/4] qerror: QERR_PERMISSION_DENIED is no longer used, drop

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/qmp/qerror.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index 596fce0c54..87ca83b155 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -50,9 +50,6 @@
 #define QERR_MISSING_PARAMETER \
     "Parameter '%s' is missing"
 
-#define QERR_PERMISSION_DENIED \
-    "Insufficient permission to perform this operation"
-
 #define QERR_PROPERTY_VALUE_BAD \
     "Property '%s.%s' doesn't take value '%s'"
 
-- 
2.37.2

Re: [PATCH 0/4] Replace QERR_PERMISSION_DENIED by better error messages

[Michael S. Tsirkin]

On Wed, Oct 12, 2022 at 05:37:57PM +0200, Markus Armbruster wrote:
> Markus Armbruster (4):
>   qom: Improve error messages when property has no getter or setter
>   backends: Improve error messages when property can no longer be set
>   qtest: Improve error messages when property can not be set right now
>   qerror: QERR_PERMISSION_DENIED is no longer used, drop

vhost user things

Acked-by: Michael S. Tsirkin <mst@redhat.com>

>  include/qapi/qmp/qerror.h       | 3 ---
>  backends/cryptodev-vhost-user.c | 2 +-
>  backends/rng-egd.c              | 2 +-
>  backends/rng-random.c           | 2 +-
>  backends/vhost-user.c           | 2 +-
>  qom/object.c                    | 6 ++++--
>  softmmu/qtest.c                 | 4 ++--
>  7 files changed, 10 insertions(+), 11 deletions(-)
> 
> -- 
> 2.37.2