Re: [PATCH v2] hmp: Make json format optional for qom-set

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Markus Armbruster (armbru@redhat.com) wrote:
> David Hildenbrand <david@redhat.com> writes:
> 
> > Commit 7d2ef6dcc1cf ("hmp: Simplify qom-set") switched to the json
> > parser, making it possible to specify complex types. However, with this
> > change it is no longer possible to specify proper sizes (e.g., 2G, 128M),
> > turning the interface harder to use for properties that consume sizes.
> >
> > Let's switch back to the previous handling and allow to specify passing
> > json via the "-j" parameter.
> 
> Two issues:
> 
> 1. Makes qom-get and qom-set inconsistent
> 
>    qom-get formats as JSON, always.
> 
>    qom-set parses the string visitor's undocumented ad hoc language by
>    default.  You can make it parse JSON by passing -j.
> 
>    Not a show stopper, but sure ugly.  I feel documentation should point
>    it out.

I can imagine one way around this owuld be to remove the flag and make
it happen in the failure case; i.e.:

    obj = qobject_from_json(value, &err);
    if (err == NULL) {
        qmp_qom_set(path, property, obj, &err);
    } else {
        somehow check if it parses with the integer parser and if it
        does use object_property_parse
    }

unfortunately that else path is a bit messy, because you need to pick a
parser in this case and then if that fails probably present the json
error message not it's error.

> 2. Rearms the string visitor death trap
> 
>    If you try to qom-set a property whose ->set() uses something the
>    string input visitor doesn't support, QEMU crashes.  I'm not aware of
>    such a ->set(), but this is a death trap all the same.  Mind, I
>    didn't actually *look* for such a ->set().  Details:
> 
>     Subject: Re: [RFC PATCH] qom: Implement qom-get HMP command
>     Date: Sat, 02 May 2020 08:02:43 +0200 (6 weeks, 2 days, 4 minutes ago)
>     Message-ID: <87a72q6fi4.fsf@dusky.pond.sub.org>
>     https://lists.nongnu.org/archive/html/qemu-devel/2020-05/msg00178.html
> 
>    Since we've had this death trap in the code for a number of years, I
>    can't call its restoration a show stopper.  It does feel like an
>    unadvisable risk, though.

That just needs fixing in qom somewhere; it shouldn't assert - people
are too free with asserts.

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK