[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PULL 04/12] hmp: Simplify qom-set
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [PULL 04/12] hmp: Simplify qom-set |
Date: |
Wed, 3 Jun 2020 14:33:54 +0100 |
User-agent: |
Mutt/1.13.4 (2020-02-15) |
* David Hildenbrand (david@redhat.com) wrote:
> On 03.06.20 14:26, David Hildenbrand wrote:
> > On 03.06.20 14:24, Dr. David Alan Gilbert wrote:
> >> * 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.
> >
> > So, I have to use a calculator from now on to set a property that I can
> > set on the QEMU cmdline just fine without it? :(
> >
> > This feels like a step backwards, @Markus any way to keep supporting sizes?
>
> Or what about adding qom-set-json instead for complex types instead of
> changing the behavior if an existing interface?
Yes, this isn't nice - we could add a flag to qom-set to take nice
numerics.
Dave
> --
> Thanks,
>
> David / dhildenb
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
- Re: [PULL 04/12] hmp: Simplify qom-set, (continued)
- Re: [PULL 04/12] hmp: Simplify qom-set, Dr. David Alan Gilbert, 2020/06/02
- Re: [PULL 04/12] hmp: Simplify qom-set, David Hildenbrand, 2020/06/03
- Re: [PULL 04/12] hmp: Simplify qom-set, Dr. David Alan Gilbert, 2020/06/03
- Re: [PULL 04/12] hmp: Simplify qom-set, David Hildenbrand, 2020/06/03
- Re: [PULL 04/12] hmp: Simplify qom-set, David Hildenbrand, 2020/06/03
- Re: [PULL 04/12] hmp: Simplify qom-set, Dr. David Alan Gilbert, 2020/06/03
- Re: [PULL 04/12] hmp: Simplify qom-set, David Hildenbrand, 2020/06/03
- Re: [PULL 04/12] hmp: Simplify qom-set, Dr. David Alan Gilbert, 2020/06/03
- Re: [PULL 04/12] hmp: Simplify qom-set, David Hildenbrand, 2020/06/03
- Re: [PULL 04/12] hmp: Simplify qom-set, David Hildenbrand, 2020/06/03
- Re: [PULL 04/12] hmp: Simplify qom-set,
Dr. David Alan Gilbert <=
[PULL 01/12] migration/rdma: fix potential nullptr access in rdma_start_incoming_migration, Dr. David Alan Gilbert (git), 2020/06/01
[PULL 05/12] virtiofsd: remove symlink fallbacks, Dr. David Alan Gilbert (git), 2020/06/01
[PULL 08/12] migration/colo.c: Use cpu_synchronize_all_states(), Dr. David Alan Gilbert (git), 2020/06/01
[PULL 06/12] migration/vmstate: Remove unnecessary MemoryRegion forward declaration, Dr. David Alan Gilbert (git), 2020/06/01
[PULL 09/12] migration/colo.c: Flush ram cache only after receiving device state, Dr. David Alan Gilbert (git), 2020/06/01
[PULL 10/12] migration/colo.c: Relaunch failover even if there was an error, Dr. David Alan Gilbert (git), 2020/06/01
[PULL 07/12] migration/colo.c: Use event instead of semaphore, Dr. David Alan Gilbert (git), 2020/06/01
[PULL 11/12] migration/colo.c: Move colo_notify_compares_event to the right place, Dr. David Alan Gilbert (git), 2020/06/01
[PULL 12/12] migration/migration.c: Fix hang in ram_save_host_page, Dr. David Alan Gilbert (git), 2020/06/01
Re: [PULL 00/12] migration/virtiofs/hmp queue, Peter Maydell, 2020/06/02