Hi Eric, hi Daniel,
Re dashes in the command name
AFAICC, the QOM related command in HMP use dash "-". For example, qom-list
and qom-set. If we will change dash "-" to underscore "_" then
qom_type_prop_list will be consistent with old HMP commands (created before
year 2013), but will _not_ be consistent with QOM commands created after
2013. Which is not nice and may be misleading.
If we want to have consistent naming of all HMP commands, then we should
rename all QOM commands to replace "-" to "_". But in this case we can
brake compatibility in possible scripts that already use these commands.
For example, scripts/qmp/qom-fuse
I would leave name qom-type-prop-list with dashes, so it will be consistent
with other two QOM commands and would refactor all QOM commands names if
possible.
Re subcommand of the info command
Also from hmp-command.hx I can see that info command shows various
information about the _system_state_ The qom-type-prop-list shows
properties of the class type. They can be changed during runtime, but I am
not sure if they can be referred as a system state. From another side
command like "info class x86_64-cpu" could take less typing, but this will
be inconsistent with QMP version of the command.
For this reasons I would leave qom-type-prop-list as it is right now.
Daniel have reviewed this patch already but found my error in the
parameters of the HMP command. I have fixed this error and tested command
with monitor. But would it be ok to add QMP and HMP tests to this patch?
Or may be submit tests with another patch, because this one is already
reviewed? I do not see much QMP/HMP tests so I am hesitating if this is a
good idea.
Thank you,
Valentin
On Fri, Jan 22, 2016 at 10:02 PM, Eric Blake <ebl...@redhat.com> wrote:
> On 01/22/2016 05:15 AM, Valentin Rakush wrote:
> > This patch adds support for qom-type-prop-list command to list object
> > class properties. A later patch will use this functionality to
> > implement x86_64-cpu properties.
> >
> > Signed-off-by: Valentin Rakush <valentin.rak...@gmail.com>
> > Cc: Luiz Capitulino <lcapitul...@redhat.com>
> > Cc: Eric Blake <ebl...@redhat.com>
> > Cc: Markus Armbruster <arm...@redhat.com>
> > Cc: Andreas Färber <afaer...@suse.de>
> > Cc: Daniel P. Berrange <berra...@redhat.com>
> > ---
>
> > +++ b/hmp-commands.hx
> > @@ -1734,6 +1734,19 @@ Print QOM properties of object at location
> @var{path}
> > ETEXI
> >
> > {
> > + .name = "qom-type-prop-list",
>
> To be consistent with most existing HMP commands, this should use
> qmp_type_prop_list (only the QMP version should use '-').
>
> For that matter, should this really be a new top-level command, or
> should it be a subcommand of the 'info' command?
>
> The QMP command looks fine, though.
>
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
>
--
Best Regards,
Valentin Rakush.
