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

[Chun Yan Liu]

>>> On 12/21/2014 at 10:34 PM, in message <5496DA81.40708 at redhat.com>, Peter Krempa
<pkrempa at redhat.com> wrote: 
> 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 at soto.provo.novell.com>, "Chun Yan Liu" 
> >> <cyliu at suse.com> wrote: 
> >> 
> >>> 
> >>>>>> On 12/17/2014 at 06:52 PM, in message 
> >>>>>> <20141217105227.GQ136165 at orkuz.home>, 
> >>> Jiri Denemark <jdenemar at redhat.com> 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 at suse.com> 
> >>>>> --- 
> >>>>> 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. 

First version is implemented by using .domainSendKey but objected. See:
https://www.redhat.com/archives/libvir-list/2014-December/msg00480.html

Thanks.

>  
> Peter 
>  
>