On Fri, Feb 01, 2019 at 11:42:57AM +0000, Dr. David Alan Gilbert wrote: > * Markus Armbruster (arm...@redhat.com) wrote: > > Eric Blake <ebl...@redhat.com> writes: > > > > > On 1/31/19 2:08 PM, Yuval Shaia wrote: > > >> On Thu, Jan 31, 2019 at 07:17:16AM -0600, Eric Blake wrote: > > >>> On 1/31/19 7:08 AM, Yuval Shaia wrote: > > >>>> Signed-off-by: Yuval Shaia <yuval.sh...@oracle.com> > > >>>> --- > > >>>> hmp-commands-info.hx | 14 ++++++++++++++ > > >>>> monitor.c | 6 ++++++ > > >>>> 2 files changed, 20 insertions(+) > > >> > > >> Hi Eric, > > >> > > >>> > > >>> Commit message should state WHY this is being added as an HMP-only > > >>> command, and does not have a QMP counterpart. It may be okay if the > > >>> interface is only designed to be useful to developers, but having that > > >>> justification in the git log is important. > > >> > > >> Thanks for your review. > > >> > > >> See, i need this interface mainly for development/debug purposes, to help > > >> troubleshot problems and to give insights to what device "is doing". > > >> > > >> Trace points are great but not effective in high load. > > >> QMP as i see it, and correct me if i'm wrong, is used to report > > >> management > > >> events etc and also here, is not effective in high load. > > > > If QMP is not effective, HMP won't be effective, either. But I guess > > you mean something else, namely QMP *events* aren't effective, but > > *polling* is. > > > > That's an argument for polling, not an argument for not supporting QMP. > > > > >> I choose this interface as it is interactive, i.e. whenever i need the > > >> info > > >> i trigger 'info pvrdmastats' command from the monitor console. > > >> > > >> During my research i notice that some devices (or families) have nice > > >> user > > >> interface via virsh (blkstat, ifstat, memstat etc). Is it the preferred > > >> way > > >> for non-devel/debug purposes? > > > > Libvirt interfaces like these are built on top of *QMP* interfaces. If > > a libvirt interface would be useful, that's another argument for > > supporting QMP. > > > > > Using existing HMP-only debug interfaces as the design you copied is > > > indeed acceptable justification for making yours HMP-only as well. So > > > now you just need to copy the rationale from this email into your commit > > > message, so it doesn't get lost. > > > > Yes. If we conclude HMP-only is okay, then the rationale for it goes > > into your commit message. > > > > If we conclude we want HMP and QMP, I'll be happy to assist you with > > adapting your patch. > > > > HMP commands without a QMP equivalent are okay if their functionality > > makes no sense in QMP, or is of use only for human users. > > > > Example for "makes no sense in QMP": setting the current CPU, because a > > QMP monitor doesn't have a current CPU. > > > > Examples for "is of use only for human users": HMP command "help", the > > integrated pocket calculator. > > > > Debugging commands are kind of borderline. Debugging is commonly a > > human activity, where HMP is just fine. However, humans create tools to > > assist with their activities, and then QMP is useful. While I wouldn't > > encourage HMP-only for the debugging use case, I wouldn't veto it. > > > > "Device statistics" sounds like it should have debugging uses. But > > statistics often have non-debugging uses as well. What use cases can > > you imagine for this command? > > I think the question here partially comes down to how 'stable' the set > of statistics is and how useful they are to non-developers. > The 'stable' part is a question because when you expose something via > QMP it's part of the ABI so people don't like it changing. > If they're things that are generic and likely to stay the same then they > should probably go via QMP (e.g. bandwidth, requests/second). > If they're things that are about the internal state of your > implementation and just for debug, then they're fine to be HMP only. > You can add them to the qmp as well, even if they're not stable by > prefixing the name with an x-. > > Dave
Thanks for giving one more point of view, yes, this interface is not "stable", I exposed the stuff i need now so it is highly reasonable that in the future it will be enhanced and new stuff would be added. Those things represent internal state of the device. > > > >> If this is the correct method for this purpose then let me know and i'll > > >> update the git log message accordingly. > -- > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
