>>> On 12/22/2014 at 08:17 PM, in message <54980BF2.1060206@xxxxxxxxxx>, John
Ferlan <jferlan@xxxxxxxxxx> wrote:
>
> On 12/21/2014 10:15 PM, Chun Yan Liu wrote:
> >
> >
> >>>> On 12/19/2014 at 08:03 PM, in message <54941429.8000802@xxxxxxxxxx>, John
> > Ferlan <jferlan@xxxxxxxxxx> wrote:
> >
> >>
> >> On 12/19/2014 12:31 AM, Chun Yan Liu wrote:
> >>>
> >>>
> >>>>>> On 12/18/2014 at 01:00 PM, in message
> >>> <5492D0080200006600086404@xxxxxxxxxxxxxxxxxxxxx>, "Chun Yan Liu"
> >>> <cyliu@xxxxxxxx> wrote:
> >>>
> >>>>
> >>>>>>> On 12/17/2014 at 06:52 PM, in message <20141217105227.GQ136165@xxxxxxxxxx>,
> >>>> Jiri Denemark <jdenemar@xxxxxxxxxx> 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@xxxxxxxx>
> >>>>>> ---
> >>>>>> 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
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list