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

[Jiri Denemark]

On Sun, Dec 14, 2014 at 20:27:09 -0700, Chun Yan Liu wrote:
> 
> 
> >>> On 12/12/2014 at 05:35 PM, in message <20141212093545.GD136165 at orkuz.home>,
> Jiri Denemark <jdenemar at redhat.com> wrote: 
> > On Fri, Dec 12, 2014 at 10:18:36 +0100, Peter Krempa wrote: 
> > > On 12/12/14 10:04, Chunyan Liu wrote: 
> > > > Add public API virDomainSendSysrq for sending SysRequest key. 
> > > >  
> > > > Signed-off-by: Chunyan Liu <cyliu at suse.com> 
> > > > --- 
> > > >  include/libvirt/libvirt-domain.h |  3 +++ 
> > > >  src/driver-hypervisor.h          |  4 ++++ 
> > > >  src/libvirt-domain.c             | 38  
> > ++++++++++++++++++++++++++++++++++++++ 
> > > >  src/libvirt_public.syms          |  1 + 
> > > >  4 files changed, 46 insertions(+) 
> > > >  
> > >  
> > > [...] 
> > >  
> > > > diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c 
> > > > index cb76d8c..4658fd7 100644 
> > > > --- a/src/libvirt-domain.c 
> > > > +++ b/src/libvirt-domain.c 
> > > > @@ -11192,3 +11192,41 @@ virDomainFSInfoFree(virDomainFSInfoPtr info) 
> > > >          VIR_FREE(info->devAlias[i]); 
> > > >      VIR_FREE(info->devAlias); 
> > > >  } 
> > > > + 
> > > > + 
> > > > +/** 
> > > > + * virDomainSendSysrq: 
> > > > + * @domain:    pointer to domain object, or NULL for Domain0 
> > > > + * @key:    SysRq key, like h, c, ... 
> > > > + * 
> > > > + * Send SysRq key to the guest. 
> > > > + * 
> > > > + * Returns 0 in case of success, -1 in case of failure. 
> > > > + */ 
> > > > +int 
> > > > +virDomainSendSysrq(virDomainPtr domain, const char *key) 
> > >  
> > > The new API should definitely have a 'flags' argument although it may be 
> > > unused for now. 
> >  
> > Moreover, passing a single character by reference sounds pretty strange, 
> > why not just "char key"?
> 
> I tried to define as 'char key', but meet some trouble in remote protocol,
> +struct remote_domain_send_sysrq_args {
> +    remote_nonnull_domain dom;
> +    char key;
> +};
> will report 'char key' as unsupported type.
> 
> > Moreover, wouldn't it be better to provide an 
> > enum of possible values with meaningful names (rather than keys), such 
> > as 
> >  
> > typedef enum { 
> >     VIR_DOMAIN_SYSRQ_REBOOT, 
> >     VIR_DOMAIN_SYSRQ_CRASH, 
> >     VIR_DOMAIN_SYSRQ_OOM_KILL, 
> >     VIR_DOMAIN_SYSRQ_SYNC, 
> >     ... 
> > } virDomainSysrqCommand; 
> >  
> > This way, virDomainSendSysrq(dom, VIR_DOMAIN_SYSRQ_OOM_KILL, 0) would be 
> > pretty self-explaining. The prototype would have to change to 
> 
> Mainly because xen/libxl API both accept letter as parameter directly, so define
> enum will need to map enum to letter before calling xen/libxl API.

Yes, but that mapping is trivial. And the API will be much better with
the enum.

Jirka