[libvirt] [PATCH V3 1/5] Add public API virDomainSendSysrq
Chun Yan Liu
cyliu at suse.com
Tue Dec 23 07:32:55 UTC 2014
>>> On 12/23/2014 at 11:42 AM, in message <5498E4BA.1000403 at redhat.com>, John
Ferlan <jferlan at 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 at redhat.com>, John
> > Ferlan <jferlan at 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 at redhat.com>, John
> >>> Ferlan <jferlan at 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 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]
> >>>
> >>> 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
> >>
> >>
> >
>
>
More information about the libvir-list
mailing list