2:48 a.m.
xend/libxl support sending sysrq key to guest kernel but not support
sending any key sequence as virDomainSendKey is expected to do. To
add equivalant sysrq functionality in libvirt for xen/libxl, add a new
virDomainSendSysrq API and add related codes to virsh, remote driver,
xen/libxl driver.
Changes to V2:
* change parameter from 'const char *key' to 'char key'.
* add 'flags' parameter to virDomainSendSysrq API.
* update codes to fit for above changes.
V2 is here:
http://www.mail-archive.com/libvir-list@redhat.com/msg106106.html
Chunyan Liu (5):
Add public API virDomainSendSysrq
implement remote protocol for domainSendSysrq
virsh: add 'sysrq' command
libxl: implement .domainSendSysrq method
xen: add .domainSendSysrq method
include/libvirt/libvirt-domain.h | 3 +++
src/driver-hypervisor.h | 4 +++
src/libvirt-domain.c | 39 +++++++++++++++++++++++++++++
src/libvirt_public.syms | 5 ++++
src/libxl/libxl_driver.c | 25 +++++++++++++++++++
src/remote/remote_driver.c | 1 +
src/remote/remote_protocol.x | 14 ++++++++++-
src/remote_protocol-structs | 6 +++++
src/rpc/gendispatch.pl | 12 +++++++++
src/xen/xen_driver.c | 21 ++++++++++++++++
src/xen/xend_internal.c | 21 ++++++++++++++++
src/xen/xend_internal.h | 1 +
tools/virsh-domain.c | 54 ++++++++++++++++++++++++++++++++++++++++
13 files changed, 205 insertions(+), 1 deletion(-)
--
1.8.4.5
2:48 a.m.
New subject: [libvirt] [PATCH V3 1/5] Add public API virDomainSendSysrq
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);
+
int virDomainGetTime(virDomainPtr dom,
long long *seconds,
unsigned int *nseconds,
diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
index 9f26b13..d260d29 100644
--- a/src/driver-hypervisor.h
+++ b/src/driver-hypervisor.h
@@ -1170,6 +1170,9 @@ typedef int
unsigned int cellCount,
unsigned int flags);
+typedef int
+(*virDrvDomainSendSysrq)(virDomainPtr dom, char key, unsigned int flags);
+
typedef struct _virHypervisorDriver virHypervisorDriver;
typedef virHypervisorDriver *virHypervisorDriverPtr;
@@ -1396,6 +1399,7 @@ struct _virHypervisorDriver {
virDrvConnectGetAllDomainStats connectGetAllDomainStats;
virDrvNodeAllocPages nodeAllocPages;
virDrvDomainGetFSInfo domainGetFSInfo;
+ virDrvDomainSendSysrq domainSendSysrq;
};
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index cb76d8c..d58ec87 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -11192,3 +11192,42 @@ 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, ...
+ * @flags: extra flags; not used yet, so callers should always pass 0
+ *
+ * Send SysRq key to the guest.
+ *
+ * Returns 0 in case of success, -1 in case of failure.
+ */
+int
+virDomainSendSysrq(virDomainPtr domain, char key, unsigned int flags)
+{
+ virConnectPtr conn;
+ VIR_DOMAIN_DEBUG(domain, "key=%c, flags=%x", key, flags);
+
+ virResetLastError();
+
+ virCheckDomainReturn(domain, -1);
+ conn = domain->conn;
+
+ virCheckReadOnlyGoto(conn->flags, error);
+
+ if (conn->driver->domainSendSysrq) {
+ int ret;
+ ret = conn->driver->domainSendSysrq(domain, key, flags);
+ if (ret < 0)
+ goto error;
+ return ret;
+ }
+
+ virReportUnsupportedError();
+
+ error:
+ virDispatchError(domain->conn);
+ return -1;
+}
diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
index e4c2df1..5d4999a 100644
--- a/src/libvirt_public.syms
+++ b/src/libvirt_public.syms
@@ -690,4 +690,9 @@ LIBVIRT_1.2.11 {
virDomainGetFSInfo;
} LIBVIRT_1.2.9;
+LIBVIRT_1.2.12 {
+ global:
+ virDomainSendSysrq;
+} LIBVIRT_1.2.11;
+
# .... define new API here using predicted next version number ....
--
1.8.4.5
4:52 a.m.
New subject: [libvirt] [PATCH V3 1/5] Add public API virDomainSendSysrq
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.
Jirka
11 p.m.
New subject: [libvirt] [PATCH V3 1/5] Add public API virDomainSendSysrq
>> 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?
Jirka
11:31 p.m.
New subject: [libvirt] [PATCH V3 1/5] Add public API virDomainSendSysrq
>> 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?
>
> Jirka
>
>
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
6:03 a.m.
New subject: [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
8:34 a.m.
New subject: [libvirt] [PATCH V3 1/5] Add public API virDomainSendSysrq
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>, "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).
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
9:18 p.m.
New subject: [libvirt] [PATCH V3 1/5] Add public API virDomainSendSysrq
>> On 12/21/2014 at 10:34 PM, in message
<5496DA81.40708(a)redhat.com>, Peter Krempa
<pkrempa(a)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(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).
>
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
9:15 p.m.
New subject: [libvirt] [PATCH V3 1/5] Add public API virDomainSendSysrq
>> On 12/19/2014 at 08:03 PM, in message
<54941429.8000802(a)redhat.com>, John
Ferlan <jferlan(a)redhat.com> 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>, "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]
Thanks for reply. The syntax I'm used previously is:
#virsh sysrq domain key
key is required. It's just a letter, like 'h', 'c', etc. About which
options can we
have, on can refer to the results on guest through sysrq help. (that is, issue
'virsh sysrq domain h' and look at guest kernel message. I think on each guest,
there must be 'h' option, it will print help message.)
Where if not provided key would be NULL, which doesn't look good for how
the code reads now.
As said above, key is required, it couldn't be NULL, otherwise, it will report error.
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.
I can add more description on how one could get those options, but the way
I think is through 'sysrq help' and check guest message.
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
...
OK. That's what I'm concerned and why I hesitated to change API parameter
from 'char key' to 'enum'. Personally I don't think this is a better
user
interface and has risk to miss some functionality, since we don't know
which options those hypervisors can support.
I still prefer:
#virsh sysrq domain key_letter
One can first issue 'virsh sysrq domain h', and check guest kernel message
for all sysrq options. Then send option as he need.
And as a result, I still think I don't see benefit of changing the API parameter
from 'char key' to 'enum'.
How do you think?
Chunyan
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
6:17 a.m.
New subject: [libvirt] [PATCH V3 1/5] Add public API virDomainSendSysrq
On 12/21/2014 10:15 PM, Chun Yan Liu wrote:
>>> On 12/19/2014 at 08:03 PM, in message <54941429.8000802(a)redhat.com>,
John
Ferlan <jferlan(a)redhat.com> 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>, "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]
Thanks for reply. The syntax I'm used previously is:
#virsh sysrq domain key
key is required. It's just a letter, like 'h', 'c', etc. About which
options can we
have, on can refer to the results on guest through sysrq help. (that is, issue
'virsh sysrq domain h' and look at guest kernel message. I think on each guest,
there must be 'h' option, it will print help message.)
h, c, etc. doesn't tell me enough about what to expect from the
perspective of this "naive user"... Passing 'h' via virsh to a driver
to return some help string that gets displayed where? Was there a
mechanism I missed to return and display that output? Do you have sample
output to show on a system with these changes applied?
>
> Where if not provided key would be NULL, which doesn't look good for how
> the code reads now.
As said above, key is required, it couldn't be NULL, otherwise, it will report
error.
While the check in virsh because VSH_OFLAG_REQ is set for key is good,
what if someone calls the API directly? You have no check there for
"key" being non null - it just gets passed along.
> 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.
I can add more description on how one could get those options, but the way
I think is through 'sysrq help' and check guest message.
>
> 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
> ...
OK. That's what I'm concerned and why I hesitated to change API parameter
from 'char key' to 'enum'. Personally I don't think this is a better
user
interface and has risk to miss some functionality, since we don't know
which options those hypervisors can support.
If some other option is to be supported on some other hypervisor or some
new option is created, then whomever makes the change to support the
option adds the new option marking it as 'hypervisor X' only or requires
specific libvirt version.
Although I do recognize the flexibility of being able to just provide a
mechanism to pass any character. It's also possibly dangerous and
difficult to document. For example, if hypervisor X says key 'h' means
'help', by hypervisor Y says key 'h' means 'halt', then what do
you do?
That's why you have a name to key mapping so that each hypervisor can
implement the required functionality that it supports. Thus 'virsh
sysrq domain halt' passes 'h' on hypervisor Y, while perhaps on
hypervisor X it passes 'p' (pause) for the "equivalent functionality".
I still prefer:
#virsh sysrq domain key_letter
One can first issue 'virsh sysrq domain h', and check guest kernel message
for all sysrq options. Then send option as he need.
And as a result, I still think I don't see benefit of changing the API parameter
from 'char key' to 'enum'.
How do you think?
I think I just don't have enough information yet. You asked in general
for some ideas - I've tried to provide some ideas. Hope they help.
John
8:55 p.m.
New subject: [libvirt] [PATCH V3 1/5] Add public API virDomainSendSysrq
>> On 12/22/2014 at 08:17 PM, in message
<54980BF2.1060206(a)redhat.com>, John
Ferlan <jferlan(a)redhat.com> wrote:
On 12/21/2014 10:15 PM, Chun Yan Liu wrote:
>
>
>>>> On 12/19/2014 at 08:03 PM, in message
<54941429.8000802(a)redhat.com>, John
> Ferlan <jferlan(a)redhat.com> 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>, "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]
>
> Thanks for reply. The syntax I'm used previously is:
> #virsh sysrq domain key
> key is required. It's just a letter, like 'h', 'c', etc. About
which
options can we
> have, on can refer to the results on guest through sysrq help. (that is,
issue
> 'virsh sysrq domain h' and look at guest kernel message. I think on each
guest,
> there must be 'h' option, it will print help message.)
h, c, etc. doesn't tell me enough about what to expect from the
perspective of this "naive user"... Passing 'h' via virsh to a driver
to return some help string that gets displayed where?
Guest kernel message if guest is Linux. xen/libxl just passes the key
blindly to guest kernel, so to pass 'h' to guest kernel, it just like one issue:
#echo 'h' > /proc/sysrq-trigger
in a Linux guest, you will see in /var/log/messages:
SysRq : HELP : loglevel(0-9) reboot(b) crash(c) terminate-all-tasks(e)
memory-full-oom-kill(f) kill-all-tasks(i) thaw-filesystems(j)
sak(k) show-backtrace-all-active-cpus(l) show-memory-usage(m)
nice-all-RT-tasks(n) poweroff(o) show-registers(p) show-all-timers(q)
unraw(r) sync(s) show-task-states(t) unmount(u) force-fb(V)
show-blocked-tasks(w) dump-ftrace-buffer(z)
Was there a
mechanism I missed to return and display that output? Do you have sample
output to show on a system with these changes applied?
I don't know how if any other hypervisor behaves differently, for xen/libxl,
they just send sysrq key to guest blindly if I understand correctly. So, which letter
is corresponding to which option is all the same with guest sysrq key definition,
in other words, it depends on guest sysrq key definition.
>
>>
>> Where if not provided key would be NULL, which doesn't look good for how
>> the code reads now.
>
> As said above, key is required, it couldn't be NULL, otherwise, it will
report error.
>
While the check in virsh because VSH_OFLAG_REQ is set for key is good,
what if someone calls the API directly? You have no check there for
"key" being non null - it just gets passed along.
>> 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.
>
> I can add more description on how one could get those options, but the way
> I think is through 'sysrq help' and check guest message.
>
>>
>> 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
>> ...
>
> OK. That's what I'm concerned and why I hesitated to change API parameter
> from 'char key' to 'enum'. Personally I don't think this is a
better user
> interface and has risk to miss some functionality, since we don't know
> which options those hypervisors can support.
>
If some other option is to be supported on some other hypervisor or some
new option is created, then whomever makes the change to support the
option adds the new option marking it as 'hypervisor X' only or requires
specific libvirt version.
Although I do recognize the flexibility of being able to just provide a
mechanism to pass any character. It's also possibly dangerous and
difficult to document. For example, if hypervisor X says key 'h' means
'help', by hypervisor Y says key 'h' means 'halt', then what do
you do?
That's why you have a name to key mapping so that each hypervisor can
implement the required functionality that it supports. Thus 'virsh
sysrq domain halt' passes 'h' on hypervisor Y, while perhaps on
hypervisor X it passes 'p' (pause) for the "equivalent functionality".
OK. I'll try to implement this way.
> I still prefer:
> #virsh sysrq domain key_letter
> One can first issue 'virsh sysrq domain h', and check guest kernel message
> for all sysrq options. Then send option as he need.
> And as a result, I still think I don't see benefit of changing the API
parameter
> from 'char key' to 'enum'.
>
> How do you think?
I think I just don't have enough information yet. You asked in general
for some ideas - I've tried to provide some ideas. Hope they help.
Thanks for all your suggestions before holiday time. Really appreciated.
Merry Christmas!
- Chunyan
John
9:42 p.m.
New subject: [libvirt] [PATCH V3 1/5] Add public API virDomainSendSysrq
On 12/22/2014 09:55 PM, Chun Yan Liu wrote:
>>> On 12/22/2014 at 08:17 PM, in message <54980BF2.1060206(a)redhat.com>,
John
Ferlan <jferlan(a)redhat.com> wrote:
>
> On 12/21/2014 10:15 PM, Chun Yan Liu wrote:
>>
>>
>>>>> On 12/19/2014 at 08:03 PM, in message
<54941429.8000802(a)redhat.com>, John
>> Ferlan <jferlan(a)redhat.com> 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>, "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]
>>
>> Thanks for reply. The syntax I'm used previously is:
>> #virsh sysrq domain key
>> key is required. It's just a letter, like 'h', 'c', etc.
About which
> options can we
>> have, on can refer to the results on guest through sysrq help. (that is,
> issue
>> 'virsh sysrq domain h' and look at guest kernel message. I think on each
> guest,
>> there must be 'h' option, it will print help message.)
>
> h, c, etc. doesn't tell me enough about what to expect from the
> perspective of this "naive user"... Passing 'h' via virsh to a
driver
> to return some help string that gets displayed where?
Guest kernel message if guest is Linux. xen/libxl just passes the key
blindly to guest kernel, so to pass 'h' to guest kernel, it just like one issue:
#echo 'h' > /proc/sysrq-trigger
in a Linux guest, you will see in /var/log/messages:
SysRq : HELP : loglevel(0-9) reboot(b) crash(c) terminate-all-tasks(e)
memory-full-oom-kill(f) kill-all-tasks(i) thaw-filesystems(j)
sak(k) show-backtrace-all-active-cpus(l) show-memory-usage(m)
nice-all-RT-tasks(n) poweroff(o) show-registers(p) show-all-timers(q)
unraw(r) sync(s) show-task-states(t) unmount(u) force-fb(V)
show-blocked-tasks(w) dump-ftrace-buffer(z)
FWIW: My point on this was - by using 'virsh sysrq domain h' you don't
provide a mechanism to display this output.
Seems just from the descriptions some of those letters might return some
useful information... Some aren't one way commands. Perhaps it would be
useful to capture output and be able to return it...
John
> Was there a
> mechanism I missed to return and display that output? Do you have sample
> output to show on a system with these changes applied?
I don't know how if any other hypervisor behaves differently, for xen/libxl,
they just send sysrq key to guest blindly if I understand correctly. So, which letter
is corresponding to which option is all the same with guest sysrq key definition,
in other words, it depends on guest sysrq key definition.
>
>>
>>>
>>> Where if not provided key would be NULL, which doesn't look good for how
>>> the code reads now.
>>
>> As said above, key is required, it couldn't be NULL, otherwise, it will
> report error.
>>
>
> While the check in virsh because VSH_OFLAG_REQ is set for key is good,
> what if someone calls the API directly? You have no check there for
> "key" being non null - it just gets passed along.
>
>>> 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.
>>
>> I can add more description on how one could get those options, but the way
>> I think is through 'sysrq help' and check guest message.
>>
>>>
>>> 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
>>> ...
>>
>> OK. That's what I'm concerned and why I hesitated to change API parameter
>> from 'char key' to 'enum'. Personally I don't think this is a
better user
>> interface and has risk to miss some functionality, since we don't know
>> which options those hypervisors can support.
>>
>
> If some other option is to be supported on some other hypervisor or some
> new option is created, then whomever makes the change to support the
> option adds the new option marking it as 'hypervisor X' only or requires
> specific libvirt version.
>
> Although I do recognize the flexibility of being able to just provide a
> mechanism to pass any character. It's also possibly dangerous and
> difficult to document. For example, if hypervisor X says key 'h' means
> 'help', by hypervisor Y says key 'h' means 'halt', then what
do you do?
> That's why you have a name to key mapping so that each hypervisor can
> implement the required functionality that it supports. Thus 'virsh
> sysrq domain halt' passes 'h' on hypervisor Y, while perhaps on
> hypervisor X it passes 'p' (pause) for the "equivalent
functionality".
>
OK. I'll try to implement this way.
>
>> I still prefer:
>> #virsh sysrq domain key_letter
>> One can first issue 'virsh sysrq domain h', and check guest kernel
message
>> for all sysrq options. Then send option as he need.
>> And as a result, I still think I don't see benefit of changing the API
> parameter
>> from 'char key' to 'enum'.
>>
>> How do you think?
>
> I think I just don't have enough information yet. You asked in general
> for some ideas - I've tried to provide some ideas. Hope they help.
Thanks for all your suggestions before holiday time. Really appreciated.
Merry Christmas!
- Chunyan
>
> John
>
>
1:32 a.m.
New subject: [libvirt] [PATCH V3 1/5] Add public API virDomainSendSysrq
>> On 12/23/2014 at 11:42 AM, in message
<5498E4BA.1000403(a)redhat.com>, John
Ferlan <jferlan(a)redhat.com> wrote:
On 12/22/2014 09:55 PM, Chun Yan Liu wrote:
>
>
>>>> On 12/22/2014 at 08:17 PM, in message
<54980BF2.1060206(a)redhat.com>, John
> Ferlan <jferlan(a)redhat.com> wrote:
>
>>
>> On 12/21/2014 10:15 PM, Chun Yan Liu wrote:
>>>
>>>
>>>>>> On 12/19/2014 at 08:03 PM, in message
<54941429.8000802(a)redhat.com>, John
>>> Ferlan <jferlan(a)redhat.com> 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>, "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]
>>>
>>> Thanks for reply. The syntax I'm used previously is:
>>> #virsh sysrq domain key
>>> key is required. It's just a letter, like 'h', 'c', etc.
About which
>> options can we
>>> have, on can refer to the results on guest through sysrq help. (that is,
>> issue
>>> 'virsh sysrq domain h' and look at guest kernel message. I think on
each
>> guest,
>>> there must be 'h' option, it will print help message.)
>>
>> h, c, etc. doesn't tell me enough about what to expect from the
>> perspective of this "naive user"... Passing 'h' via virsh to
a driver
>> to return some help string that gets displayed where?
>
> Guest kernel message if guest is Linux. xen/libxl just passes the key
> blindly to guest kernel, so to pass 'h' to guest kernel, it just like one
issue:
> #echo 'h' > /proc/sysrq-trigger
> in a Linux guest, you will see in /var/log/messages:
>
> SysRq : HELP : loglevel(0-9) reboot(b) crash(c) terminate-all-tasks(e)
> memory-full-oom-kill(f) kill-all-tasks(i) thaw-filesystems(j)
> sak(k) show-backtrace-all-active-cpus(l) show-memory-usage(m)
> nice-all-RT-tasks(n) poweroff(o) show-registers(p) show-all-timers(q)
> unraw(r) sync(s) show-task-states(t) unmount(u) force-fb(V)
> show-blocked-tasks(w) dump-ftrace-buffer(z)
>
FWIW: My point on this was - by using 'virsh sysrq domain h' you don't
provide a mechanism to display this output.
Seems just from the descriptions some of those letters might return some
useful information... Some aren't one way commands. Perhaps it would be
useful to capture output and be able to return it...
Right. But might be hard.
And I think of a problem when mapping enum to letter:
to different guests (e.g. Linux vs Windows), the letter to enum mapping might
be different, even hypervisor may not precisely know that. At least for xen
hypervisor, I think it only blindly sends the key to guest, but has no idea of
different key-letter meaning to different guests.
- Chunyan
John
>> Was there a
>> mechanism I missed to return and display that output? Do you have sample
>> output to show on a system with these changes applied?
>
> I don't know how if any other hypervisor behaves differently, for
xen/libxl,
> they just send sysrq key to guest blindly if I understand correctly. So,
which letter
> is corresponding to which option is all the same with guest sysrq key
definition,
> in other words, it depends on guest sysrq key definition.
>
>>
>>>
>>>>
>>>> Where if not provided key would be NULL, which doesn't look good for
how
>>>> the code reads now.
>>>
>>> As said above, key is required, it couldn't be NULL, otherwise, it will
>> report error.
>>>
>>
>> While the check in virsh because VSH_OFLAG_REQ is set for key is good,
>> what if someone calls the API directly? You have no check there for
>> "key" being non null - it just gets passed along.
>>
>>>> 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.
>>>
>>> I can add more description on how one could get those options, but the way
>>> I think is through 'sysrq help' and check guest message.
>>>
>>>>
>>>> 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
>>>> ...
>>>
>>> OK. That's what I'm concerned and why I hesitated to change API
parameter
>>> from 'char key' to 'enum'. Personally I don't think this
is a better user
>>> interface and has risk to miss some functionality, since we don't know
>>> which options those hypervisors can support.
>>>
>>
>> If some other option is to be supported on some other hypervisor or some
>> new option is created, then whomever makes the change to support the
>> option adds the new option marking it as 'hypervisor X' only or requires
>> specific libvirt version.
>>
>> Although I do recognize the flexibility of being able to just provide a
>> mechanism to pass any character. It's also possibly dangerous and
>> difficult to document. For example, if hypervisor X says key 'h' means
>> 'help', by hypervisor Y says key 'h' means 'halt', then
what do you do?
>> That's why you have a name to key mapping so that each hypervisor can
>> implement the required functionality that it supports. Thus 'virsh
>> sysrq domain halt' passes 'h' on hypervisor Y, while perhaps on
>> hypervisor X it passes 'p' (pause) for the "equivalent
functionality".
>>
>
> OK. I'll try to implement this way.
>
>>
>>> I still prefer:
>>> #virsh sysrq domain key_letter
>>> One can first issue 'virsh sysrq domain h', and check guest kernel
message
>>> for all sysrq options. Then send option as he need.
>>> And as a result, I still think I don't see benefit of changing the API
>> parameter
>>> from 'char key' to 'enum'.
>>>
>>> How do you think?
>>
>> I think I just don't have enough information yet. You asked in general
>> for some ideas - I've tried to provide some ideas. Hope they help.
>
> Thanks for all your suggestions before holiday time. Really appreciated.
> Merry Christmas!
>
> - Chunyan
>>
>> John
>>
>>
>
10:55 p.m.
New subject: [libvirt] [PATCH V3 1/5] Add public API virDomainSendSysrq
>> On 12/23/2014 at 03:32 PM, in message <54991AA7.243 : 102
: 21807>, Chun Yan
Liu wrote:
>>> On 12/23/2014 at 11:42 AM, in message <5498E4BA.1000403(a)redhat.com>,
John
Ferlan <jferlan(a)redhat.com> wrote:
>
> On 12/22/2014 09:55 PM, Chun Yan Liu wrote:
> >
> >
> >>>> On 12/22/2014 at 08:17 PM, in message
<54980BF2.1060206(a)redhat.com>, John
> > Ferlan <jferlan(a)redhat.com> wrote:
> >
> >>
> >> On 12/21/2014 10:15 PM, Chun Yan Liu wrote:
> >>>
> >>>
> >>>>>> On 12/19/2014 at 08:03 PM, in message
<54941429.8000802(a)redhat.com>, John
> >>> Ferlan <jferlan(a)redhat.com> 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>,
"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]
> >>>
> >>> Thanks for reply. The syntax I'm used previously is:
> >>> #virsh sysrq domain key
> >>> key is required. It's just a letter, like 'h', 'c',
etc. About which
> >> options can we
> >>> have, on can refer to the results on guest through sysrq help. (that is,
> >> issue
> >>> 'virsh sysrq domain h' and look at guest kernel message. I think
on each
> >> guest,
> >>> there must be 'h' option, it will print help message.)
> >>
> >> h, c, etc. doesn't tell me enough about what to expect from the
> >> perspective of this "naive user"... Passing 'h' via virsh
to a driver
> >> to return some help string that gets displayed where?
> >
> > Guest kernel message if guest is Linux. xen/libxl just passes the key
> > blindly to guest kernel, so to pass 'h' to guest kernel, it just like
one
> issue:
> > #echo 'h' > /proc/sysrq-trigger
> > in a Linux guest, you will see in /var/log/messages:
> >
> > SysRq : HELP : loglevel(0-9) reboot(b) crash(c) terminate-all-tasks(e)
> > memory-full-oom-kill(f) kill-all-tasks(i) thaw-filesystems(j)
> > sak(k) show-backtrace-all-active-cpus(l) show-memory-usage(m)
> > nice-all-RT-tasks(n) poweroff(o) show-registers(p) show-all-timers(q)
> > unraw(r) sync(s) show-task-states(t) unmount(u) force-fb(V)
> > show-blocked-tasks(w) dump-ftrace-buffer(z)
> >
>
> FWIW: My point on this was - by using 'virsh sysrq domain h' you don't
> provide a mechanism to display this output.
>
> Seems just from the descriptions some of those letters might return some
> useful information... Some aren't one way commands. Perhaps it would be
> useful to capture output and be able to return it...
Right. But might be hard.
And I think of a problem when mapping enum to letter:
to different guests (e.g. Linux vs Windows), the letter to enum mapping
might
be different, even hypervisor may not precisely know that. At least for xen
hypervisor, I think it only blindly sends the key to guest, but has no idea
of
different key-letter meaning to different guests.
- Chunyan
Hi, everyone,
Happy New Year!
Pick up this thread again since I just could not follow your suggestions about
using 'enum' instead of 'char' as virDomainSendSysrq parameter and virsh
sysrq
domain reboot|crash|... syntax. Summarize here so that you could remember it
and spot your further ideas.
My original propose is:
virsh sysrq domain key -->
virDomainSendSysrq(domain, key, flags) -->
xen|libxl driver .domainSendSysrq(domain, key, flags)
According to suggestions:
The workflow would be:
virsh sysrq domain reboot|crash|... -->
virDomainSendSysrq(domain, VIR_DOMAIN_SYSRQ_REBOOT, flags) -->
xen|libxl driver, convert enum to key first -->
xen|libxl driver, call xen|libxl sending key API.
Main reason I still think the new way cannot work is:
xen|libxl driver doesn't know how to convert enum to key correctly, since sysrq key
is very specific to guest, different guests (Windows or Linux), sysrq key and meaning
are different, even xen hypervisor doesn't know which key means what to the guest,
it just send the key blindly to guest. So it's user who should get familiar with the
sysrq key of the destination guest.
How do you think? Can we preserve original implementation, or any other ideas?
Thanks very much!
- Chunyan
>
> John
> >> Was there a
> >> mechanism I missed to return and display that output? Do you have sample
> >> output to show on a system with these changes applied?
> >
> > I don't know how if any other hypervisor behaves differently, for
> xen/libxl,
> > they just send sysrq key to guest blindly if I understand correctly. So,
> which letter
> > is corresponding to which option is all the same with guest sysrq key
> definition,
> > in other words, it depends on guest sysrq key definition.
> >
> >>
> >>>
> >>>>
> >>>> Where if not provided key would be NULL, which doesn't look good
for how
> >>>> the code reads now.
> >>>
> >>> As said above, key is required, it couldn't be NULL, otherwise, it
will
> >> report error.
> >>>
> >>
> >> While the check in virsh because VSH_OFLAG_REQ is set for key is good,
> >> what if someone calls the API directly? You have no check there for
> >> "key" being non null - it just gets passed along.
> >>
> >>>> 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.
> >>>
> >>> I can add more description on how one could get those options, but the
way
> >>> I think is through 'sysrq help' and check guest message.
> >>>
> >>>>
> >>>> 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
> >>>> ...
> >>>
> >>> OK. That's what I'm concerned and why I hesitated to change API
parameter
> >>> from 'char key' to 'enum'. Personally I don't think
this is a better user
> >>> interface and has risk to miss some functionality, since we don't
know
> >>> which options those hypervisors can support.
> >>>
> >>
> >> If some other option is to be supported on some other hypervisor or some
> >> new option is created, then whomever makes the change to support the
> >> option adds the new option marking it as 'hypervisor X' only or
requires
> >> specific libvirt version.
> >>
> >> Although I do recognize the flexibility of being able to just provide a
> >> mechanism to pass any character. It's also possibly dangerous and
> >> difficult to document. For example, if hypervisor X says key 'h'
means
> >> 'help', by hypervisor Y says key 'h' means 'halt',
then what do you do?
> >> That's why you have a name to key mapping so that each hypervisor can
> >> implement the required functionality that it supports. Thus 'virsh
> >> sysrq domain halt' passes 'h' on hypervisor Y, while perhaps on
> >> hypervisor X it passes 'p' (pause) for the "equivalent
functionality".
> >>
> >
> > OK. I'll try to implement this way.
> >
> >>
> >>> I still prefer:
> >>> #virsh sysrq domain key_letter
> >>> One can first issue 'virsh sysrq domain h', and check guest
kernel message
> >>> for all sysrq options. Then send option as he need.
> >>> And as a result, I still think I don't see benefit of changing the
API
> >> parameter
> >>> from 'char key' to 'enum'.
> >>>
> >>> How do you think?
> >>
> >> I think I just don't have enough information yet. You asked in general
> >> for some ideas - I've tried to provide some ideas. Hope they help.
> >
> > Thanks for all your suggestions before holiday time. Really appreciated.
> > Merry Christmas!
> >
> > - Chunyan
> >>
> >> John
> >>
> >>
> >
>
>
2:48 a.m.
New subject: [libvirt] [PATCH V3 2/5] implement remote protocol for domainSendSysrq
Signed-off-by: Chunyan Liu <cyliu(a)suse.com>
---
Changes:
* change args 'key' from 'remote_nonnull_string' to 'char'
* add code to gendispatch.pl to handle 'char|unsigned char' type
* update remote_protocol-strcuts
src/remote/remote_driver.c | 1 +
src/remote/remote_protocol.x | 14 +++++++++++++-
src/remote_protocol-structs | 6 ++++++
src/rpc/gendispatch.pl | 12 ++++++++++++
4 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 999f16d..b24de5c 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -8285,6 +8285,7 @@ static virHypervisorDriver hypervisor_driver = {
.connectGetAllDomainStats = remoteConnectGetAllDomainStats, /* 1.2.8 */
.nodeAllocPages = remoteNodeAllocPages, /* 1.2.9 */
.domainGetFSInfo = remoteDomainGetFSInfo, /* 1.2.11 */
+ .domainSendSysrq = remoteDomainSendSysrq, /* 1.2.12 */
};
static virNetworkDriver network_driver = {
diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
index cbd3ec7..708983c 100644
--- a/src/remote/remote_protocol.x
+++ b/src/remote/remote_protocol.x
@@ -1084,6 +1084,12 @@ struct remote_domain_send_key_args {
unsigned int flags;
};
+struct remote_domain_send_sysrq_args {
+ remote_nonnull_domain dom;
+ char key;
+ unsigned int flags;
+};
+
struct remote_domain_send_process_signal_args {
remote_nonnull_domain dom;
hyper pid_value;
@@ -5550,5 +5556,11 @@ enum remote_procedure {
* @generate: none
* @acl: domain:fs_freeze
*/
- REMOTE_PROC_DOMAIN_GET_FSINFO = 349
+ REMOTE_PROC_DOMAIN_GET_FSINFO = 349,
+
+ /**
+ * @generate: both
+ * @acl: domain:send_input
+ */
+ REMOTE_PROC_DOMAIN_SEND_SYSRQ = 350
};
diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
index 2907fd5..afd9a8d 100644
--- a/src/remote_protocol-structs
+++ b/src/remote_protocol-structs
@@ -2605,6 +2605,11 @@ struct remote_domain_get_fsinfo_ret {
} info;
u_int ret;
};
+struct remote_domain_send_sysrq_args {
+ remote_nonnull_domain dom;
+ char key;
+ unsigned int flags;
+};
enum remote_procedure {
REMOTE_PROC_CONNECT_OPEN = 1,
REMOTE_PROC_CONNECT_CLOSE = 2,
@@ -2955,4 +2960,5 @@ enum remote_procedure {
REMOTE_PROC_NODE_ALLOC_PAGES = 347,
REMOTE_PROC_DOMAIN_EVENT_CALLBACK_AGENT_LIFECYCLE = 348,
REMOTE_PROC_DOMAIN_GET_FSINFO = 349,
+ REMOTE_PROC_DOMAIN_SEND_SYSRQ = 350,
};
diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl
index 0dc167a..cafe5ab 100755
--- a/src/rpc/gendispatch.pl
+++ b/src/rpc/gendispatch.pl
@@ -592,6 +592,12 @@ elsif ($mode eq "server") {
} else {
push(@args_list, "args->$arg_name");
}
+ } elsif ($args_member =~ m/^(unsigned )?char (\S+);/) {
+ if (! @args_list) {
+ push(@args_list, "priv->conn");
+ }
+
+ push(@args_list, "args->$2");
} elsif ($args_member =~ m/^(\/)?\*/) {
# ignore comments
} else {
@@ -1228,6 +1234,12 @@ elsif ($mode eq "client") {
push(@args_list, "$type_name $arg_name");
push(@setters_list, "args.$arg_name = $arg_name;");
+ } elsif ($args_member =~ m/^((?:unsigned )?char) (\S+);/) {
+ my $type_name = $1;
+ my $arg_name = $2;
+
+ push(@args_list, "$type_name $arg_name");
+ push(@setters_list, "args.$arg_name = $arg_name;");
} elsif ($args_member =~ m/^(\/)?\*/) {
# ignore comments
} else {
--
1.8.4.5
2:48 a.m.
New subject: [libvirt] [PATCH V3 3/5] virsh: add 'sysrq' command
All domainSendSysrq related API should be manageable from
the virsh command line. So, expose 'virsh sysrq' command.
Signed-off-by: Chunyan Liu <cyliu(a)suse.com>
---
tools/virsh-domain.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 54 insertions(+)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 750411b..bffcc8e 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -12289,6 +12289,54 @@ cmdDomFSInfo(vshControl *ctl, const vshCmd *cmd)
return ret >= 0;
}
+/*
+ * "sysrq" command
+ */
+static const vshCmdInfo info_sysrq[] = {
+ {.name = "help",
+ .data = N_("Send SysRq key to the guest")
+ },
+ {.name = "desc",
+ .data = N_("Send SysRq key to the guest")
+ },
+ {.name = NULL}
+};
+
+static const vshCmdOptDef opts_sysrq[] = {
+ {.name = "domain",
+ .type = VSH_OT_DATA,
+ .flags = VSH_OFLAG_REQ,
+ .help = N_("domain name, id or uuid")
+ },
+ {.name = "key",
+ .type = VSH_OT_DATA,
+ .flags = VSH_OFLAG_REQ,
+ .help = N_("the SysRq key")
+ },
+ {.name = NULL}
+};
+
+static bool
+cmdSysrq(vshControl *ctl, const vshCmd *cmd)
+{
+ virDomainPtr dom;
+ bool ret = false;
+ const char *key = NULL;
+
+ if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
+ return false;
+
+ if (vshCommandOptStringReq(ctl, cmd, "key", &key) < 0)
+ return false;
+
+ if (!(virDomainSendSysrq(dom, key[0], 0) < 0))
+ ret = true;
+
+ virDomainFree(dom);
+ return ret;
+}
+
+
const vshCmdDef domManagementCmds[] = {
{.name = "attach-device",
.handler = cmdAttachDevice,
@@ -12808,5 +12856,11 @@ const vshCmdDef domManagementCmds[] = {
.info = info_vncdisplay,
.flags = 0
},
+ {.name = "sysrq",
+ .handler = cmdSysrq,
+ .opts = opts_sysrq,
+ .info = info_sysrq,
+ .flags = 0
+ },
{.name = NULL}
};
--
1.8.4.5
6:14 a.m.
New subject: [libvirt] [PATCH V3 3/5] virsh: add 'sysrq' command
On 12/17/2014 09:48 AM, Chunyan Liu wrote:
All domainSendSysrq related API should be manageable from
the virsh command line. So, expose 'virsh sysrq' command.
Signed-off-by: Chunyan Liu <cyliu(a)suse.com>
---
tools/virsh-domain.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 54 insertions(+)
+
+static bool
+cmdSysrq(vshControl *ctl, const vshCmd *cmd)
+{
+ virDomainPtr dom;
+ bool ret = false;
+ const char *key = NULL;
+
+ if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
+ return false;
+
+ if (vshCommandOptStringReq(ctl, cmd, "key", &key) < 0)
+ return false;
dom needs to be freed.
+
+ if (!(virDomainSendSysrq(dom, key[0], 0) < 0))
+ ret = true;
+
Just a nitpick: I find our usual template more readable:
if (vir...() < 0)
goto cleanup;
ret = true
cleanup:
Jan
9:35 p.m.
New subject: [libvirt] [PATCH V3 3/5] virsh: add 'sysrq' command
>> On 12/17/2014 at 08:14 PM, in message
<549173AC.2000707(a)redhat.com>, Ján
Tomko<jtomko(a)redhat.com> wrote:
On 12/17/2014 09:48 AM, Chunyan Liu wrote:
> All domainSendSysrq related API should be manageable from
> the virsh command line. So, expose 'virsh sysrq' command.
>
> Signed-off-by: Chunyan Liu <cyliu(a)suse.com>
> ---
> tools/virsh-domain.c | 54
++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 54 insertions(+)
>
> +
> +static bool
> +cmdSysrq(vshControl *ctl, const vshCmd *cmd)
> +{
> + virDomainPtr dom;
> + bool ret = false;
> + const char *key = NULL;
> +
> + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
> + return false;
> +
> + if (vshCommandOptStringReq(ctl, cmd, "key", &key) < 0)
> + return false;
dom needs to be freed.
> +
> + if (!(virDomainSendSysrq(dom, key[0], 0) < 0))
> + ret = true;
> +
Just a nitpick: I find our usual template more readable:
if (vir...() < 0)
goto cleanup;
ret = true
cleanup:
Thanks, I'll update them.
Jan
2:48 a.m.
New subject: [libvirt] [PATCH V3 4/5] libxl: implement .domainSendSysrq method
Support .domainSendSysrq in libxl driver.
Signed-off-by: Chunyan Liu <cyliu(a)suse.com>
---
src/libxl/libxl_driver.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 53c87ce..7c96bab 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -4729,6 +4729,30 @@ libxlDomainMigrateConfirm3Params(virDomainPtr domain,
return libxlDomainMigrationConfirm(driver, vm, flags, cancelled);
}
+static int
+libxlDomainSendSysrq(virDomainPtr dom, char key, unsigned int flags)
+{
+ virDomainObjPtr vm;
+ libxlDomainObjPrivatePtr priv;
+ int ret = -1;
+
+ virCheckFlags(0, -1);
+
+ if (!(vm = libxlDomObjFromDomain(dom)))
+ goto cleanup;
+
+ priv = vm->privateData;
+
+ if (virDomainSendSysrqEnsureACL(dom->conn, vm->def) < 0)
+ goto cleanup;
+
+ ret = libxl_send_sysrq(priv->ctx, vm->def->id, key);
+
+ cleanup:
+ if (vm)
+ virObjectUnlock(vm);
+ return ret;
+}
static virHypervisorDriver libxlDriver = {
.no = VIR_DRV_LIBXL,
@@ -4824,6 +4848,7 @@ static virHypervisorDriver libxlDriver = {
.domainMigratePerform3Params = libxlDomainMigratePerform3Params, /* 1.2.6 */
.domainMigrateFinish3Params = libxlDomainMigrateFinish3Params, /* 1.2.6 */
.domainMigrateConfirm3Params = libxlDomainMigrateConfirm3Params, /* 1.2.6 */
+ .domainSendSysrq = libxlDomainSendSysrq, /* 1.2.12 */
};
static virStateDriver libxlStateDriver = {
--
1.8.4.5
2:48 a.m.
New subject: [libvirt] [PATCH V3 5/5] xen: add .domainSendSysrq method
Support sending sysrq key to guest.
Signed-off-by: Chunyan Liu <cyliu(a)suse.com>
---
src/xen/xen_driver.c | 21 +++++++++++++++++++++
src/xen/xend_internal.c | 26 ++++++++++++++++++++++++++
src/xen/xend_internal.h | 1 +
3 files changed, 48 insertions(+)
diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
index c9f4159..4a56b69 100644
--- a/src/xen/xen_driver.c
+++ b/src/xen/xen_driver.c
@@ -2738,6 +2738,26 @@ xenUnifiedNodeSuspendForDuration(virConnectPtr conn,
return nodeSuspendForDuration(target, duration, flags);
}
+static int
+xenUnifiedDomainSendSysrq(virDomainPtr dom, char key, unsigned int flags)
+{
+ int ret = -1;
+ virDomainDefPtr def = NULL;
+
+ virCheckFlags(0, -1);
+
+ if (!(def = xenGetDomainDefForDom(dom)))
+ goto cleanup;
+
+ if (virDomainSendSysrqEnsureACL(dom->conn, def) < 0)
+ goto cleanup;
+
+ ret = xenDaemonDomainSysrq(dom->conn, def, key);
+
+ cleanup:
+ virDomainDefFree(def);
+ return ret;
+}
/*----- Register with libvirt.c, and initialize Xen drivers. -----*/
@@ -2836,6 +2856,7 @@ static virHypervisorDriver xenUnifiedDriver = {
.nodeSuspendForDuration = xenUnifiedNodeSuspendForDuration, /* 0.9.8 */
.nodeGetMemoryParameters = xenUnifiedNodeGetMemoryParameters, /* 0.10.2 */
.nodeSetMemoryParameters = xenUnifiedNodeSetMemoryParameters, /* 0.10.2 */
+ .domainSendSysrq = xenUnifiedDomainSendSysrq, /* 1.2.12 */
};
/**
diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
index b233b6b..16532ad 100644
--- a/src/xen/xend_internal.c
+++ b/src/xen/xend_internal.c
@@ -3300,6 +3300,32 @@ xenDaemonDomainBlockPeek(virConnectPtr conn,
return ret;
}
+/*
+ * xenDaemonDomainSysrq:
+ * <at> conn: the connection object
+ * <at> def: the domain to destroy
+ * <at> key: SysRq key
+ *
+ * Send a sysrq to a domain.
+ *
+ * Returns 0 in case of success, -1 (with errno) in case of error.
+ */
+int
+xenDaemonDomainSysrq(virConnectPtr conn, virDomainDefPtr def, char key)
+{
+ char *buf;
+
+ if (def->id < 0) {
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ _("Domain %s isn't running."), def->name);
+ return -1;
+ }
+
+ if (virAsprintf(&buf, "%c", key) < 0)
+ return -1;
+
+ return xend_op(conn, def->name, "op", "sysrq",
"key", buf, NULL);
+}
/**
* virDomainXMLDevID:
diff --git a/src/xen/xend_internal.h b/src/xen/xend_internal.h
index 814330d..02f9d9b 100644
--- a/src/xen/xend_internal.h
+++ b/src/xen/xend_internal.h
@@ -213,5 +213,6 @@ int xenDaemonSetSchedulerParameters(virConnectPtr conn,
virDomainDefPtr def,
virTypedParameterPtr params,
int nparams);
+int xenDaemonDomainSysrq(virConnectPtr conn, virDomainDefPtr def, char key);
#endif /* __XEND_INTERNAL_H_ */
--
1.8.4.5
3607
days inactive
3628
days old
19 comments
6 participants
participants (6)
-
Chun Yan Liu -
Chunyan Liu
-
Jiri Denemark -
John Ferlan -
Ján Tomko -
Peter Krempa