Re: [PULL 04/12] hmp: Simplify qom-set

[Dr. David Alan Gilbert]

* David Hildenbrand (david@redhat.com) wrote:
> On 03.06.20 13:43, Dr. David Alan Gilbert wrote:
> > * David Hildenbrand (david@redhat.com) wrote:
> >> On 03.06.20 12:43, Dr. David Alan Gilbert wrote:
> >>> * David Hildenbrand (david@redhat.com) wrote:
> >>>> On 02.06.20 11:26, Dr. David Alan Gilbert wrote:
> >>>>> * Markus Armbruster (armbru@redhat.com) wrote:
> >>>>>> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> >>>>>>
> >>>>>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >>>>>>>
> >>>>>>> Simplify qom_set by making it use qmp_qom_set and the JSON parser.
> >>>>>>>
> >>>>>>> (qemu) qom-get /machine smm
> >>>>>>> "auto"
> >>>>>>> (qemu) qom-set /machine smm "auto"
> >>>>>>>
> >>>>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >>>>>>> Message-Id: <20200520151108.160598-3-dgilbert@redhat.com>
> >>>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >>>>>>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> >>>>>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >>>>>>>   With 's'->'S' type change suggested by Paolo and Markus
> >>>>>>
> >>>>>> This is actually more than just simplification, it's disarming a bear
> >>>>>> trap: the string visitor is restricted to a subset of the QAPI types,
> >>>>>> and when you qom-set a property with a type it can't handle, QEMU
> >>>>>> aborts.  I mentioned this in the discussion of possible ways out of the
> >>>>>> qom-get impasse, but missed reraising it in patch review.
> >>>>>>
> >>>>>> A suitably amended commit would be nice, but respinning the PR just for
> >>>>>> that may not be worthwhile.
> >>>>>
> >>>>> A bit late; still as long as we're removing bear traps not adding them.
> >>>>
> >>>> This breaks qom-set for my (virtio-mem) use case:
> >>>>
> >>>> echo "qom-set vm0 requested-size 300M" | sudo nc -U /var/tmp/mon_src
> >>>> QEMU 5.0.50 monitor - type 'help' for more information
> >>>> (qemu) qom-set vm0 requested-size 300M
> >>>> Error: Expecting at most one JSON value
> >>>
> >>> Does qom-set vm0 requested-size 300e6 do the same thing?
> >>
> >> The property is defined to be of type "size".
> >>
> >> (qemu) qom-set vm0 requested-size 300e6
> >> Error: Parameter 'requested-size' expects uint64
> >>
> >> (not sure how "size" and "uint64" are mapped here)
> > 
> > I think the problem here is that the JSON parser is converting anything
> > with an 'e' as a float; JSON itself doesn't have the distinction
> > between int and float.
> > 
> 
> (and just to clarify - I assume you are aware - 300e6 != 300M. So the
> interface becomes way harder to use in case one wants to specify
> properly aligned sizes - 300M vs 314572800)

Oops, yes, good point.

I think on balance it's probably best that this keeps supporting JSON;
although tbh I'm not convinced there are any complex types that can be
set.
I'm not seeing a prettier answer.

Dave

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