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

>>> On 12/22/2014 at 08:17 PM, in message <54980BF2.1060206(a)redhat.com&gt;, John Ferlan <jferlan(a)redhat.com&gt; 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&gt;, John >> Ferlan <jferlan(a)redhat.com&gt; 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&gt;, "Chun Yan Liu" >>>> <cyliu(a)suse.com&gt; wrote: >>>> >>>>> >>>>>>>> On 12/17/2014 at 06:52 PM, in message <20141217105227.GQ136165(a)orkuz.home&gt;, >>>>> Jiri Denemark <jdenemar(a)redhat.com&gt; 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&gt; >>>>>>> --- >>>>>>> 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 > >