Re: [Qemu-devel] [PATCH v4] qom, qmp, hmp, qapi: create qom-type-prop-list for class properties

[Valentin Rakush]

Hi Eric,

On Mon, Jan 25, 2016 at 8:43 PM, Eric Blake <address@hidden> wrote:

On 01/22/2016 01:20 PM, Valentin Rakush wrote:
> 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.

Well, HMP is not set in stone. We can rename the HMP command to qom_list
and qom_set, if we want consistency so that _all_ HMP commands prefer _.

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

Any script using HMP is already broken, because HMP is not designed for
scriptability.  Scripts should be using QMP, and QMP has stricter rules
about not arbitrarily changing names.

The HMP and QMP has same command qom-list. When I searched for this command 

then I found that it is used in scripts (QMP only) and did not look further. My fault. 

Thank you for explanations.

 

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

I'm not asking you to change the QMP name, just the HMP name.

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

We aren't aiming for equivalence between QMP and HMP.  It's fine for the
HMP command to be higher-level, have more smarts, and have more
consistency with other HMP commands.

>
> For this reasons I would leave qom-type-prop-list as it is right now.

Since HMP is not scriptable, I am not going to hold up the patch on
bikeshedding how the HMP command is spelled.  I just wanted to point out
the difference in conventions, and that you are adding an exception to
the conventions.

The qom-list breaks this conventions and this is why I did mistake when 

implemented qom-type-prop-list. 

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

Another idea is to add ONLY a QMP command, and omit the functionality
from HMP altogether.  If someone finds the HMP interface important, they
can submit a later patch to add it on top of the QMP command.

There were no requirements to add HMP command, but I was implementing 

qom-type-prop-list based on the qom-list (as an example). The qom-list command

has QMP and HMP commands which spells similar and this is a reason why 

I implemented HMP version of the qom-type-prop-list.

Because there were no such requirement to add HMP qom-type-prop-list command 

I think it would be better to remove HMP version of qom-type-prop-list. I will do this in

next version of this patch.

Thank you.

--
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

--

Best Regards,
Valentin Rakush.