Re: [libvirt] [PATCH V3 1/5] Add public API virDomainSendSysrq
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>, "Chun Yan Liu"
<cyliu(a)suse.com> wrote:
>
>>>> On 12/17/2014 at 06:52 PM, in message
<20141217105227.GQ136165(a)orkuz.home>,
> Jiri Denemark <jdenemar(a)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(a)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).
The string for key would be passed to the virDomainSendSysrq which would
then "convert" or map that string into an enum. Check out the
VIR_ENUM_{IMPL|DECL} usage and how they generate "TypeToString" and
"TypeFromString" API's to perform the string <-> enum mapping.
So there's "some thoughts" for you - hopefully it gives you some ideas.
John