qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v4] qom, qmp, hmp, qapi: create qom-type-prop-li


From: Valentin Rakush
Subject: Re: [Qemu-devel] [PATCH v4] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties
Date: Fri, 22 Jan 2016 23:20:45 +0300

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 <address@hidden> 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 <address@hidden>
> Cc: Luiz Capitulino <address@hidden>
> Cc: Eric Blake <address@hidden>
> Cc: Markus Armbruster <address@hidden>
> Cc: Andreas Färber <address@hidden>
> Cc: Daniel P. Berrange <address@hidden>
> ---

> +++ 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.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]