Re: [libvirt] [PATCH V3 1/5] Add public API virDomainSendSysrq

>> On 12/21/2014 at 10:34 PM, in message <5496DA81.40708(a)redhat.com&gt;, Peter Krempa

On 12/19/14 13:03, John Ferlan wrote: > > > On 12/19/2014 12:31 AM, Chun Yan Liu wrote: >> >> >>>>> On 12/18/2014 at 01:00 PM, in message >> <5492D0080200006600086404(a)soto.provo.novell.com&gt;, "Chun Yan Liu" >> <cyliu(a)suse.com&gt; wrote: >> >>> >>>>>> On 12/17/2014 at 06:52 PM, in message >>>>>> <20141217105227.GQ136165(a)orkuz.home&gt;, >>> Jiri Denemark <jdenemar(a)redhat.com&gt; wrote: >>>> On Wed, Dec 17, 2014 at 16:48:52 +0800, Chunyan Liu wrote: >>>>> Add public API virDomainSendSysrq for sending SysRequest key. >>>>> >>>>> Signed-off-by: Chunyan Liu <cyliu(a)suse.com&gt; >>>>> --- >>>>> changes: >>>>> * add 'flags' to the new API >>>>> * change parameter from 'const char *key' to 'char key' >>>>> * change version number from 1.2.11 to 1.2.12 >>>>> >>>>> include/libvirt/libvirt-domain.h | 3 +++ >>>>> src/driver-hypervisor.h | 4 ++++ >>>>> src/libvirt-domain.c | 39 >>>> +++++++++++++++++++++++++++++++++++++++ >>>>> src/libvirt_public.syms | 5 +++++ >>>>> 4 files changed, 51 insertions(+) >>>>> >>>>> diff --git a/include/libvirt/libvirt-domain.h >>>> b/include/libvirt/libvirt-domain.h >>>>> index baef32d..5f72850 100644 >>>>> --- a/include/libvirt/libvirt-domain.h >>>>> +++ b/include/libvirt/libvirt-domain.h >>>>> @@ -3526,6 +3526,9 @@ int virDomainGetFSInfo(virDomainPtr dom, >>>>> virDomainFSInfoPtr **info, >>>>> unsigned int flags); >>>>> >>>>> +/* virDomainSendSysrq */ >>>>> +int virDomainSendSysrq(virDomainPtr dom, char key, unsigned int >>>>> flags); >>>>> + >>>> >>>> I think quite a few reviewers (Daniel, Eric, and I) agreed on using an >>>> enum instead of char so that the API is more general. >>> >>> Sorry, I missed this part. I'll update. One left question: >>> How about 'virsh sysrq' parameters? What would we expect users to pass? >> >> Any thoughts on that? >> libxl_send_sysrq > > Without a virsh.pod in v3 to go with virsh-domain.c, I'm not sure what > you had in mind for syntax previously - although it looks like: > > virsh sysrq domain [key] > > Where if not provided key would be NULL, which doesn't look good for how > the code reads now. The description for key in virDomainSendSysrq is > still not sufficient to help me either: > > + * @key: SysRq key, like h, c, ... > > What does 'h', 'c', ... mean? What are the options? What do they map to > functionality wise? I assume it's hypervisor dependent, but that's all > stuff you need to describe somewhere. I don't want to guess or go > searching for the answer through numerous search engine hits. > > Looking at the enum Jirka proposed: > > typedef enum { > VIR_DOMAIN_SYSRQ_REBOOT, > VIR_DOMAIN_SYSRQ_CRASH, > VIR_DOMAIN_SYSRQ_OOM_KILL, > VIR_DOMAIN_SYSRQ_SYNC, > ... > } virDomainSysrqCommand; > > It seems "REBOOT" would/could be the default. So if key wasn't provided > the incoming key would be "0" (zero)... If you didn't want a default, > then you'd have to force a style to be chosen. You're defining the API > so you show us how you want to handle that. Eventually, each hypervisor > would map that enum into a character. That is, you'll end up with a way > to map the enum to a letter for the types of sysrq's each hypervisor > could support. If a hypervisor doesn't support a specific type of sysrq, > then decide how to handle. > > Anyway given the above enum list, I would think the virsh would be: > > virsh sysrq domain reboot > virsh sysrq domain crash > virsh sysrq domain kill > virsh sysrq domain sync > ... > > And key goes from optional to required unless you want to allow 'virsh > sysrq domain' to mean reboot by default (e.g., if not provided the > default is to reboot). > This still can be implemented using the existing API for sending general keystrokes to the guest. I still don't see a reason to add a new API as a special case of an existing one.

Peter