| Message ID | 1473157086-12062-1-git-send-email-dgilbert@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 06.09.2016 um 12:18 schrieb Dr. David Alan Gilbert (git): > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > This started off as Andreas Färber's implementation from > March 2015, but after feedback from Paolo morphed into > using the json output which handles structs reasonably. > > Use with qom-list to find the members of an object. > > (qemu) qom-get /backend/console[0]/device/vga.rom[0] size > 65536 > (qemu) qom-get /machine smm > "auto" > (qemu) qom-get /machine rtc-time > { > "tm_year": 116, > "tm_sec": 0, > "tm_hour": 9, > "tm_min": 46, > "tm_mon": 8, > "tm_mday": 6 > } > (qemu) qom-get /machine frob > Property '.frob' not found > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Acked-by: Andreas Färber <afaerber@suse.de> Thanks for working on this, Andreas
On Tue, Sep 06, 2016 at 11:18:06AM +0100, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > This started off as Andreas Färber's implementation from > March 2015, but after feedback from Paolo morphed into > using the json output which handles structs reasonably. > > Use with qom-list to find the members of an object. > > (qemu) qom-get /backend/console[0]/device/vga.rom[0] size > 65536 > (qemu) qom-get /machine smm > "auto" > (qemu) qom-get /machine rtc-time > { > "tm_year": 116, > "tm_sec": 0, > "tm_hour": 9, > "tm_min": 46, > "tm_mon": 8, > "tm_mday": 6 > } I'm not really a fan of us exposing JSON in the HMP, as it rather seems to defeat the purpose of using the HMP. > -- > v2 > switched from using string-output-visitor to qobject_to_json_pretty, > drop string-output-visitor patch. 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 With your example that ought to lead to output looking like (qemu) qom-get /machine rtc-time tm_year: 116 tm_sec: 0 tm_hour: 9 tm_min: 46 tm_mon: 8 tm_mday: 6 which i think is more suitable for the HMP. Regards, Daniel
* Daniel P. Berrange (berrange@redhat.com) wrote: > On Tue, Sep 06, 2016 at 11:18:06AM +0100, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > This started off as Andreas Färber's implementation from > > March 2015, but after feedback from Paolo morphed into > > using the json output which handles structs reasonably. > > > > Use with qom-list to find the members of an object. > > > > (qemu) qom-get /backend/console[0]/device/vga.rom[0] size > > 65536 > > (qemu) qom-get /machine smm > > "auto" > > (qemu) qom-get /machine rtc-time > > { > > "tm_year": 116, > > "tm_sec": 0, > > "tm_hour": 9, > > "tm_min": 46, > > "tm_mon": 8, > > "tm_mday": 6 > > } > > I'm not really a fan of us exposing JSON in the HMP, as it rather > seems to defeat the purpose of using the HMP. Well as long as it's clear and easily readable by humans I'm OK about it; and I'm less fussed about it on the output side; JSON is much more painful to use as human typed input. > > -- > > v2 > > switched from using string-output-visitor to qobject_to_json_pretty, > > drop string-output-visitor patch. > > 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. > With your example that ought to lead to output looking like > > (qemu) qom-get /machine rtc-time > tm_year: 116 > tm_sec: 0 > tm_hour: 9 > tm_min: 46 > tm_mon: 8 > tm_mday: 6 > > which i think is more suitable for the HMP. Yes, it's a little prettier; I'm not fussed either way - but I just want to make sure that we do end up with a qom-get; it's something I'd have found useful in the past and I know others would have, and the code for it was originally written by Andreas ~2.5 years ago - so if text-output-visitor is imminent then sure, but if it's not then I suggest we stick with this. Dave > > Regards, > Daniel > -- > |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| > |: http://libvirt.org -o- http://virt-manager.org :| > |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| > |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > * Daniel P. Berrange (berrange@redhat.com) wrote: >> On Tue, Sep 06, 2016 at 11:18:06AM +0100, Dr. David Alan Gilbert (git) wrote: >> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> >> > >> > This started off as Andreas Färber's implementation from >> > March 2015, but after feedback from Paolo morphed into >> > using the json output which handles structs reasonably. >> > >> > Use with qom-list to find the members of an object. >> > >> > (qemu) qom-get /backend/console[0]/device/vga.rom[0] size >> > 65536 >> > (qemu) qom-get /machine smm >> > "auto" >> > (qemu) qom-get /machine rtc-time >> > { >> > "tm_year": 116, >> > "tm_sec": 0, >> > "tm_hour": 9, >> > "tm_min": 46, >> > "tm_mon": 8, >> > "tm_mday": 6 >> > } >> >> I'm not really a fan of us exposing JSON in the HMP, as it rather >> seems to defeat the purpose of using the HMP. > > Well as long as it's clear and easily readable by humans I'm OK about it; > and I'm less fussed about it on the output side; JSON is much more painful > to use as human typed input. > >> > -- >> > v2 >> > switched from using string-output-visitor to qobject_to_json_pretty, >> > drop string-output-visitor patch. >> >> 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? >> With your example that ought to lead to output looking like >> >> (qemu) qom-get /machine rtc-time >> tm_year: 116 >> tm_sec: 0 >> tm_hour: 9 >> tm_min: 46 >> tm_mon: 8 >> tm_mday: 6 >> >> which i think is more suitable for the HMP. > > Yes, it's a little prettier; I'm not fussed either way - but I just want to > make sure that we do end up with a qom-get; it's something I'd have found > useful in the past and I know others would have, and the code for it was > originally written by Andreas ~2.5 years ago - so if text-output-visitor is > imminent then sure, but if it's not then I suggest we stick with this. We can take qom-get as is and improve its output later.
On Fri, Sep 09, 2016 at 06:21:15PM +0200, Markus Armbruster wrote: > "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > > > * Daniel P. Berrange (berrange@redhat.com) 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. 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. Regards, Daniel
"Daniel P. Berrange" <berrange@redhat.com> writes: > On Fri, Sep 09, 2016 at 06:21:15PM +0200, Markus Armbruster wrote: >> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: >> >> > * Daniel P. Berrange (berrange@redhat.com) 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.
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > This started off as Andreas Färber's implementation from > March 2015, but after feedback from Paolo morphed into > using the json output which handles structs reasonably. > > Use with qom-list to find the members of an object. > > (qemu) qom-get /backend/console[0]/device/vga.rom[0] size > 65536 > (qemu) qom-get /machine smm > "auto" > (qemu) qom-get /machine rtc-time > { > "tm_year": 116, > "tm_sec": 0, > "tm_hour": 9, > "tm_min": 46, > "tm_mon": 8, > "tm_mday": 6 > } > (qemu) qom-get /machine frob > Property '.frob' not found > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Ignorant question: how does qom-set deal with structs? I tried the obvious (qemu) qom-set /machine rtc-time abc Insufficient permission to perform this operation
* Markus Armbruster (armbru@redhat.com) wrote: > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes: > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > This started off as Andreas Färber's implementation from > > March 2015, but after feedback from Paolo morphed into > > using the json output which handles structs reasonably. > > > > Use with qom-list to find the members of an object. > > > > (qemu) qom-get /backend/console[0]/device/vga.rom[0] size > > 65536 > > (qemu) qom-get /machine smm > > "auto" > > (qemu) qom-get /machine rtc-time > > { > > "tm_year": 116, > > "tm_sec": 0, > > "tm_hour": 9, > > "tm_min": 46, > > "tm_mon": 8, > > "tm_mday": 6 > > } > > (qemu) qom-get /machine frob > > Property '.frob' not found > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > Ignorant question: how does qom-set deal with structs? > > I tried the obvious > > (qemu) qom-set /machine rtc-time abc > Insufficient permission to perform this operation I don't think it does. Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Wed, Sep 14, 2016 at 11:30:06AM +0100, 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> > > > > > > This started off as Andreas Färber's implementation from > > > March 2015, but after feedback from Paolo morphed into > > > using the json output which handles structs reasonably. > > > > > > Use with qom-list to find the members of an object. > > > > > > (qemu) qom-get /backend/console[0]/device/vga.rom[0] size > > > 65536 > > > (qemu) qom-get /machine smm > > > "auto" > > > (qemu) qom-get /machine rtc-time > > > { > > > "tm_year": 116, > > > "tm_sec": 0, > > > "tm_hour": 9, > > > "tm_min": 46, > > > "tm_mon": 8, > > > "tm_mday": 6 > > > } > > > (qemu) qom-get /machine frob > > > Property '.frob' not found > > > > > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > > Ignorant question: how does qom-set deal with structs? > > > > I tried the obvious > > > > (qemu) qom-set /machine rtc-time abc > > Insufficient permission to perform this operation > > I don't think it does. Indeed it can't - qom_set ends up calling object_property_parse which uses string-input-visitor to parse the value, which can only handle scalars as the magic special case list-of-ints. To deal with compound properties would really require us to use a qdict_crumple + qmp_input_visitor combination, similar to how I've made -object and object_add be able to deal with compound properties. Regards, Daniel
"Daniel P. Berrange" <berrange@redhat.com> writes: > On Wed, Sep 14, 2016 at 11:30:06AM +0100, 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> >> > > >> > > This started off as Andreas Färber's implementation from >> > > March 2015, but after feedback from Paolo morphed into >> > > using the json output which handles structs reasonably. >> > > >> > > Use with qom-list to find the members of an object. >> > > >> > > (qemu) qom-get /backend/console[0]/device/vga.rom[0] size >> > > 65536 >> > > (qemu) qom-get /machine smm >> > > "auto" >> > > (qemu) qom-get /machine rtc-time >> > > { >> > > "tm_year": 116, >> > > "tm_sec": 0, >> > > "tm_hour": 9, >> > > "tm_min": 46, >> > > "tm_mon": 8, >> > > "tm_mday": 6 >> > > } >> > > (qemu) qom-get /machine frob >> > > Property '.frob' not found >> > > >> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >> > >> > Ignorant question: how does qom-set deal with structs? >> > >> > I tried the obvious >> > >> > (qemu) qom-set /machine rtc-time abc >> > Insufficient permission to perform this operation >> >> I don't think it does. > > Indeed it can't - qom_set ends up calling object_property_parse which > uses string-input-visitor to parse the value, which can only handle > scalars as the magic special case list-of-ints. > > To deal with compound properties would really require us to use a > qdict_crumple + qmp_input_visitor combination, similar to how I've > made -object and object_add be able to deal with compound properties. HMP I/O formats are not ABI. We can use visitors in whatever way we want, as long as we keep -get and -set consistent. The sane way to do that is using the same kind of visitor for both, in its input and output form, respectively. Right now, qom-set uses the string input visitor. As long as it does that, qom-get should use the string output visitor. Sadly, this pair of visitors is quite limited ("does not implement support for visiting QAPI structs, alternates, null, or arbitrary QTypes"). We can extend it to cover more, or we can switch to another, less limited pair of visitors. Can we agree on what to do so we can have qom-get sooner rather than later? It doesn't have to be perfect, we can iterate.
On Mon, Sep 19, 2016 at 11:18:05AM +0200, Markus Armbruster wrote: > "Daniel P. Berrange" <berrange@redhat.com> writes: > > > On Wed, Sep 14, 2016 at 11:30:06AM +0100, 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> > >> > > > >> > > This started off as Andreas Färber's implementation from > >> > > March 2015, but after feedback from Paolo morphed into > >> > > using the json output which handles structs reasonably. > >> > > > >> > > Use with qom-list to find the members of an object. > >> > > > >> > > (qemu) qom-get /backend/console[0]/device/vga.rom[0] size > >> > > 65536 > >> > > (qemu) qom-get /machine smm > >> > > "auto" > >> > > (qemu) qom-get /machine rtc-time > >> > > { > >> > > "tm_year": 116, > >> > > "tm_sec": 0, > >> > > "tm_hour": 9, > >> > > "tm_min": 46, > >> > > "tm_mon": 8, > >> > > "tm_mday": 6 > >> > > } > >> > > (qemu) qom-get /machine frob > >> > > Property '.frob' not found > >> > > > >> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > >> > > >> > Ignorant question: how does qom-set deal with structs? > >> > > >> > I tried the obvious > >> > > >> > (qemu) qom-set /machine rtc-time abc > >> > Insufficient permission to perform this operation > >> > >> I don't think it does. > > > > Indeed it can't - qom_set ends up calling object_property_parse which > > uses string-input-visitor to parse the value, which can only handle > > scalars as the magic special case list-of-ints. > > > > To deal with compound properties would really require us to use a > > qdict_crumple + qmp_input_visitor combination, similar to how I've > > made -object and object_add be able to deal with compound properties. > > HMP I/O formats are not ABI. We can use visitors in whatever way we > want, as long as we keep -get and -set consistent. The sane way to do > that is using the same kind of visitor for both, in its input and output > form, respectively. > > Right now, qom-set uses the string input visitor. As long as it does > that, qom-get should use the string output visitor. Sadly, this pair of > visitors is quite limited ("does not implement support for visiting QAPI > structs, alternates, null, or arbitrary QTypes"). We can extend it to > cover more, or we can switch to another, less limited pair of visitors. > > Can we agree on what to do so we can have qom-get sooner rather than > later? It doesn't have to be perfect, we can iterate. I think that -object sets the precedent that the rest should ultimately follow. It currently uses the opts visitor syntax, but is being switched over to the combination of qdict_crumple + qobject input visitor, which is basically the same as opts visitor syntax for scalars and with dotted notation for compound types. The HMP object_add command will use the exact same syntax as -object CLI arg. Given this, I think 'qom-set' really ought to be updated to use qdict_crumple + qobject input visitor too, so it can deal with compound types. In fact I'd view the lack of conversion of qom-set as a mistake in my patch series - I should have converted that too, while adding support for compound properties to -object and object_add. This ultimately means that qom-get probably ought to use qobject output visitor, followed by a qdict flatten operation to turn the nested dicts/lists, into a flat dict with dotted syntax. Regards, Daniel
On Mon, Sep 19, 2016 at 10:54:49AM +0100, Daniel P. Berrange wrote: > On Mon, Sep 19, 2016 at 11:18:05AM +0200, Markus Armbruster wrote: > > "Daniel P. Berrange" <berrange@redhat.com> writes: > > > > > On Wed, Sep 14, 2016 at 11:30:06AM +0100, 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> > > >> > > > > >> > > This started off as Andreas Färber's implementation from > > >> > > March 2015, but after feedback from Paolo morphed into > > >> > > using the json output which handles structs reasonably. > > >> > > > > >> > > Use with qom-list to find the members of an object. > > >> > > > > >> > > (qemu) qom-get /backend/console[0]/device/vga.rom[0] size > > >> > > 65536 > > >> > > (qemu) qom-get /machine smm > > >> > > "auto" > > >> > > (qemu) qom-get /machine rtc-time > > >> > > { > > >> > > "tm_year": 116, > > >> > > "tm_sec": 0, > > >> > > "tm_hour": 9, > > >> > > "tm_min": 46, > > >> > > "tm_mon": 8, > > >> > > "tm_mday": 6 > > >> > > } > > >> > > (qemu) qom-get /machine frob > > >> > > Property '.frob' not found > > >> > > > > >> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > >> > > > >> > Ignorant question: how does qom-set deal with structs? > > >> > > > >> > I tried the obvious > > >> > > > >> > (qemu) qom-set /machine rtc-time abc > > >> > Insufficient permission to perform this operation > > >> > > >> I don't think it does. > > > > > > Indeed it can't - qom_set ends up calling object_property_parse which > > > uses string-input-visitor to parse the value, which can only handle > > > scalars as the magic special case list-of-ints. > > > > > > To deal with compound properties would really require us to use a > > > qdict_crumple + qmp_input_visitor combination, similar to how I've > > > made -object and object_add be able to deal with compound properties. > > > > HMP I/O formats are not ABI. We can use visitors in whatever way we > > want, as long as we keep -get and -set consistent. The sane way to do > > that is using the same kind of visitor for both, in its input and output > > form, respectively. > > > > Right now, qom-set uses the string input visitor. As long as it does > > that, qom-get should use the string output visitor. Sadly, this pair of > > visitors is quite limited ("does not implement support for visiting QAPI > > structs, alternates, null, or arbitrary QTypes"). We can extend it to > > cover more, or we can switch to another, less limited pair of visitors. > > > > Can we agree on what to do so we can have qom-get sooner rather than > > later? It doesn't have to be perfect, we can iterate. > > I think that -object sets the precedent that the rest should ultimately > follow. It currently uses the opts visitor syntax, but is being switched > over to the combination of qdict_crumple + qobject input visitor, which > is basically the same as opts visitor syntax for scalars and with dotted > notation for compound types. > > The HMP object_add command will use the exact same syntax as -object > CLI arg. Given this, I think 'qom-set' really ought to be updated to use > qdict_crumple + qobject input visitor too, so it can deal with compound > types. In fact I'd view the lack of conversion of qom-set as a mistake > in my patch series - I should have converted that too, while adding > support for compound properties to -object and object_add. > > This ultimately means that qom-get probably ought to use qobject output > visitor, followed by a qdict flatten operation to turn the nested > dicts/lists, into a flat dict with dotted syntax. Further, QOM has two methods which are intended to be mirror imges of each other - object_property_parse and object_property_print. The current qom-set impl uses object_property_parse, so we ought to make 'qom-get' use object_property_print. We would still need to enhance object_property_print() to deal with compound types, but that's doesn't have to be a blocker - Dave's qom-get patch can just use object_property_print() as it exists today and we can enhance the impl separately. Regards, Daniel
* Daniel P. Berrange (berrange@redhat.com) wrote: > On Mon, Sep 19, 2016 at 10:54:49AM +0100, Daniel P. Berrange wrote: > > On Mon, Sep 19, 2016 at 11:18:05AM +0200, Markus Armbruster wrote: > > > "Daniel P. Berrange" <berrange@redhat.com> writes: > > > > > > > On Wed, Sep 14, 2016 at 11:30:06AM +0100, 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> > > > >> > > > > > >> > > This started off as Andreas Färber's implementation from > > > >> > > March 2015, but after feedback from Paolo morphed into > > > >> > > using the json output which handles structs reasonably. > > > >> > > > > > >> > > Use with qom-list to find the members of an object. > > > >> > > > > > >> > > (qemu) qom-get /backend/console[0]/device/vga.rom[0] size > > > >> > > 65536 > > > >> > > (qemu) qom-get /machine smm > > > >> > > "auto" > > > >> > > (qemu) qom-get /machine rtc-time > > > >> > > { > > > >> > > "tm_year": 116, > > > >> > > "tm_sec": 0, > > > >> > > "tm_hour": 9, > > > >> > > "tm_min": 46, > > > >> > > "tm_mon": 8, > > > >> > > "tm_mday": 6 > > > >> > > } > > > >> > > (qemu) qom-get /machine frob > > > >> > > Property '.frob' not found > > > >> > > > > > >> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > >> > > > > >> > Ignorant question: how does qom-set deal with structs? > > > >> > > > > >> > I tried the obvious > > > >> > > > > >> > (qemu) qom-set /machine rtc-time abc > > > >> > Insufficient permission to perform this operation > > > >> > > > >> I don't think it does. > > > > > > > > Indeed it can't - qom_set ends up calling object_property_parse which > > > > uses string-input-visitor to parse the value, which can only handle > > > > scalars as the magic special case list-of-ints. > > > > > > > > To deal with compound properties would really require us to use a > > > > qdict_crumple + qmp_input_visitor combination, similar to how I've > > > > made -object and object_add be able to deal with compound properties. > > > > > > HMP I/O formats are not ABI. We can use visitors in whatever way we > > > want, as long as we keep -get and -set consistent. The sane way to do > > > that is using the same kind of visitor for both, in its input and output > > > form, respectively. > > > > > > Right now, qom-set uses the string input visitor. As long as it does > > > that, qom-get should use the string output visitor. Sadly, this pair of > > > visitors is quite limited ("does not implement support for visiting QAPI > > > structs, alternates, null, or arbitrary QTypes"). We can extend it to > > > cover more, or we can switch to another, less limited pair of visitors. > > > > > > Can we agree on what to do so we can have qom-get sooner rather than > > > later? It doesn't have to be perfect, we can iterate. > > > > I think that -object sets the precedent that the rest should ultimately > > follow. It currently uses the opts visitor syntax, but is being switched > > over to the combination of qdict_crumple + qobject input visitor, which > > is basically the same as opts visitor syntax for scalars and with dotted > > notation for compound types. > > > > The HMP object_add command will use the exact same syntax as -object > > CLI arg. Given this, I think 'qom-set' really ought to be updated to use > > qdict_crumple + qobject input visitor too, so it can deal with compound > > types. In fact I'd view the lack of conversion of qom-set as a mistake > > in my patch series - I should have converted that too, while adding > > support for compound properties to -object and object_add. > > > > This ultimately means that qom-get probably ought to use qobject output > > visitor, followed by a qdict flatten operation to turn the nested > > dicts/lists, into a flat dict with dotted syntax. > > Further, QOM has two methods which are intended to be mirror imges > of each other - object_property_parse and object_property_print. > The current qom-set impl uses object_property_parse, so we ought > to make 'qom-get' use object_property_print. > > We would still need to enhance object_property_print() to deal with > compound types, but that's doesn't have to be a blocker - Dave's > qom-get patch can just use object_property_print() as it exists today > and we can enhance the impl separately. That's what the v1 version of Andreas/my patch did isn't it? Dave > Regards, > Daniel > -- > |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| > |: http://libvirt.org -o- http://virt-manager.org :| > |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| > |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > * Daniel P. Berrange (berrange@redhat.com) wrote: >> On Mon, Sep 19, 2016 at 10:54:49AM +0100, Daniel P. Berrange wrote: >> > On Mon, Sep 19, 2016 at 11:18:05AM +0200, Markus Armbruster wrote: >> > > "Daniel P. Berrange" <berrange@redhat.com> writes: >> > > >> > > > On Wed, Sep 14, 2016 at 11:30:06AM +0100, 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> >> > > >> > > >> > > >> > > This started off as Andreas Färber's implementation from >> > > >> > > March 2015, but after feedback from Paolo morphed into >> > > >> > > using the json output which handles structs reasonably. >> > > >> > > >> > > >> > > Use with qom-list to find the members of an object. >> > > >> > > >> > > >> > > (qemu) qom-get /backend/console[0]/device/vga.rom[0] size >> > > >> > > 65536 >> > > >> > > (qemu) qom-get /machine smm >> > > >> > > "auto" >> > > >> > > (qemu) qom-get /machine rtc-time >> > > >> > > { >> > > >> > > "tm_year": 116, >> > > >> > > "tm_sec": 0, >> > > >> > > "tm_hour": 9, >> > > >> > > "tm_min": 46, >> > > >> > > "tm_mon": 8, >> > > >> > > "tm_mday": 6 >> > > >> > > } >> > > >> > > (qemu) qom-get /machine frob >> > > >> > > Property '.frob' not found >> > > >> > > >> > > >> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> >> > > >> > >> > > >> > Ignorant question: how does qom-set deal with structs? >> > > >> > >> > > >> > I tried the obvious >> > > >> > >> > > >> > (qemu) qom-set /machine rtc-time abc >> > > >> > Insufficient permission to perform this operation >> > > >> >> > > >> I don't think it does. >> > > > >> > > > Indeed it can't - qom_set ends up calling object_property_parse which >> > > > uses string-input-visitor to parse the value, which can only handle >> > > > scalars as the magic special case list-of-ints. >> > > > >> > > > To deal with compound properties would really require us to use a >> > > > qdict_crumple + qmp_input_visitor combination, similar to how I've >> > > > made -object and object_add be able to deal with compound properties. >> > > >> > > HMP I/O formats are not ABI. We can use visitors in whatever way we >> > > want, as long as we keep -get and -set consistent. The sane way to do >> > > that is using the same kind of visitor for both, in its input and output >> > > form, respectively. >> > > >> > > Right now, qom-set uses the string input visitor. As long as it does >> > > that, qom-get should use the string output visitor. Sadly, this pair of >> > > visitors is quite limited ("does not implement support for visiting QAPI >> > > structs, alternates, null, or arbitrary QTypes"). We can extend it to >> > > cover more, or we can switch to another, less limited pair of visitors. >> > > >> > > Can we agree on what to do so we can have qom-get sooner rather than >> > > later? It doesn't have to be perfect, we can iterate. >> > >> > I think that -object sets the precedent that the rest should ultimately >> > follow. It currently uses the opts visitor syntax, but is being switched >> > over to the combination of qdict_crumple + qobject input visitor, which >> > is basically the same as opts visitor syntax for scalars and with dotted >> > notation for compound types. >> > >> > The HMP object_add command will use the exact same syntax as -object >> > CLI arg. Given this, I think 'qom-set' really ought to be updated to use >> > qdict_crumple + qobject input visitor too, so it can deal with compound >> > types. In fact I'd view the lack of conversion of qom-set as a mistake >> > in my patch series - I should have converted that too, while adding >> > support for compound properties to -object and object_add. >> > >> > This ultimately means that qom-get probably ought to use qobject output >> > visitor, followed by a qdict flatten operation to turn the nested >> > dicts/lists, into a flat dict with dotted syntax. >> >> Further, QOM has two methods which are intended to be mirror imges >> of each other - object_property_parse and object_property_print. >> The current qom-set impl uses object_property_parse, so we ought >> to make 'qom-get' use object_property_print. >> >> We would still need to enhance object_property_print() to deal with >> compound types, but that's doesn't have to be a blocker - Dave's >> qom-get patch can just use object_property_print() as it exists today >> and we can enhance the impl separately. > > That's what the v1 version of Andreas/my patch did isn't it? Looks like it :)
diff --git a/hmp-commands.hx b/hmp-commands.hx index 848efee..73f0372 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -1736,6 +1736,19 @@ Print QOM properties of object at location @var{path} ETEXI { + .name = "qom-get", + .args_type = "path:s,property:s", + .params = "path property", + .help = "print QOM property", + .mhandler.cmd = hmp_qom_get, + }, + +STEXI +@item qom-get @var{path} @var{property} +Print QOM property @var{property} of object at location @var{path} +ETEXI + + { .name = "qom-set", .args_type = "path:s,property:s,value:s", .params = "path property value", diff --git a/hmp.c b/hmp.c index cc2056e..88c659b 100644 --- a/hmp.c +++ b/hmp.c @@ -22,11 +22,13 @@ #include "qemu/option.h" #include "qemu/timer.h" #include "qmp-commands.h" +#include "qom/qom-qobject.h" #include "qemu/sockets.h" #include "monitor/monitor.h" #include "monitor/qdev.h" #include "qapi/opts-visitor.h" #include "qapi/qmp/qerror.h" +#include "qapi/qmp/qjson.h" #include "qapi/string-output-visitor.h" #include "qapi/util.h" #include "qapi-visit.h" @@ -2064,6 +2066,30 @@ void hmp_qom_list(Monitor *mon, const QDict *qdict) hmp_handle_error(mon, &err); } +void hmp_qom_get(Monitor *mon, const QDict *qdict) +{ + const char *path = qdict_get_str(qdict, "path"); + const char *property = qdict_get_str(qdict, "property"); + Error *err = NULL; + Object *obj; + QObject *sub; + + obj = object_resolve_path(path, NULL); + if (obj == NULL) { + error_set(&err, ERROR_CLASS_DEVICE_NOT_FOUND, + "Device '%s' not found", path); + hmp_handle_error(mon, &err); + return; + } + sub = object_property_get_qobject(obj, property, &err); + if (err == NULL) { + QString *str = qobject_to_json_pretty(sub); + monitor_printf(mon, "%s\n", qstring_get_str(str)); + QDECREF(str); + } + hmp_handle_error(mon, &err); +} + void hmp_qom_set(Monitor *mon, const QDict *qdict) { const char *path = qdict_get_str(qdict, "path"); diff --git a/hmp.h b/hmp.h index 0876ec0..882f339 100644 --- a/hmp.h +++ b/hmp.h @@ -103,6 +103,7 @@ void hmp_object_del(Monitor *mon, const QDict *qdict); void hmp_info_memdev(Monitor *mon, const QDict *qdict); void hmp_info_memory_devices(Monitor *mon, const QDict *qdict); void hmp_qom_list(Monitor *mon, const QDict *qdict); +void hmp_qom_get(Monitor *mon, const QDict *qdict); void hmp_qom_set(Monitor *mon, const QDict *qdict); void object_add_completion(ReadLineState *rs, int nb_args, const char *str); void object_del_completion(ReadLineState *rs, int nb_args, const char *str);