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

[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 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.

Peter

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20141221/d84b2d87/attachment-0001.sig>