Hi Eric, On Mon, Jan 25, 2016 at 8:43 PM, Eric Blake <[email protected]> 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.
