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

>> On 12/23/2014 at 11:42 AM, in message <5498E4BA.1000403(a)redhat.com&gt;, John

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&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) > 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 >> >> >