[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] qom: Implement qom-get HMP command
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v2] qom: Implement qom-get HMP command |
Date: |
Mon, 12 Sep 2016 09:58:58 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
"Daniel P. Berrange" <address@hidden> writes:
> On Fri, Sep 09, 2016 at 06:21:15PM +0200, Markus Armbruster wrote:
>> "Dr. David Alan Gilbert" <address@hidden> writes:
>>
>> > * Daniel P. Berrange (address@hidden) wrote:
>> >> IIUC, you switched because string-output-visitor could not handle
>> >> complex types.
>> >>
>> >> I have previously written a text-output-visitor that could do
>> >> this correctly, since we have this exact same requirement for
>> >> 'qemu-info info' to print out the extra-block specific data
>> >> in human friendly format for the LUKS driver.
>> >>
>> >> https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg01727.html
>> >
>> > How close to going in is it? It looks from the comments that Eric
>> > is thinking about fixing string-output-visitor.
>> > I'm not clear why you'd want a text-output-visitor and a
>> > string-output-vistior.
>>
>> Me neither.
>>
>> In theory, we need two output visitors producing text, one for
>> machine-readable output (JSON), and the one for human-readable output.
>>
>> For the latter, we use the string output visitor.
>>
>> For the former, we first use the (badly named) QMP output visitor to
>> produce a QObject, which is then converted to JSON text by
>> qobject_to_json() or qobject_to_json_pretty().
>>
>> Each of the two output visitors comes with a matching input visitor that
>> reads the same format.
>>
>> To read JSON text, we first parse it into a QObject with
>> json_message_parser_init() & friends, which we then pass to the QMP
>> input visitor.
>>
>> If I read Dan's cover letter correctly, the proposed text output visitor
>> competes with the string output visitor. Why not enhance or replace it?
>
> I guess my original posting was not clear enough about the rational for
> the separate visitor. Looking closely at the output format for the
> existing string output visitor and comparing to what's proposed for my
> text output visitor, you'll see they are completely different output
> formats. They both happen to create strings, but that's pretty much
> where the similarity ends. So while we could squish the functionality
> into one visitor class, it wouldn't really let us share any code - the
> visitor would just end up taking different codepaths for each style.
> Having these two styles mixed will then make it harder to understand
> the logic behind either of them.
Begs the question why we need different output formats, but...
> There is a specific place where code sharing is useful though - the
> formatting of a uint64 in sized notatation ie "1.24 GB", and for
> that I'm creating a new shared helper function in util/cutils since
> we've already got 2 copies of that logic !
>
> Anyway, lets continue this debate on the patch series which actualy
> proposes the text-output-visitor, which I'm re-posting in a few
> minutes with greater explanation of the new format which should
> hopefully clarify why it is justified.
... I agree it should be discussed there, not here.
Re: [Qemu-devel] [PATCH v2] qom: Implement qom-get HMP command, Markus Armbruster, 2016/09/13
- Re: [Qemu-devel] [PATCH v2] qom: Implement qom-get HMP command, Dr. David Alan Gilbert, 2016/09/14
- Re: [Qemu-devel] [PATCH v2] qom: Implement qom-get HMP command, Daniel P. Berrange, 2016/09/14
- Re: [Qemu-devel] [PATCH v2] qom: Implement qom-get HMP command, Markus Armbruster, 2016/09/19
- Re: [Qemu-devel] [PATCH v2] qom: Implement qom-get HMP command, Daniel P. Berrange, 2016/09/19
- Re: [Qemu-devel] [PATCH v2] qom: Implement qom-get HMP command, Daniel P. Berrange, 2016/09/19
- Re: [Qemu-devel] [PATCH v2] qom: Implement qom-get HMP command, Dr. David Alan Gilbert, 2016/09/19
- Re: [Qemu-devel] [PATCH v2] qom: Implement qom-get HMP command, Markus Armbruster, 2016/09/19