Hi guys,
Previously I had a series of out of tree patches to xend that added a
shutdown event callback on domain death that would call a script with
the reason why the domain shutdown.
Basically analagous to this:
/etc/xen/scripts/shutdown-event $event $domname $domid
I would like to add similar functionality to xl and attempt to
contribute it back because the feature is incredibly useful for event
based scripting.
Many Xen system admins before me have resorted to hacking it into xend
because it''s so useful for monitoring and automation. :P
Similar things can be done with xenstore-watch or the xenstore C API
but they lose information because the only thing communicated to
xenstore is intent.
If a domain crashes for instance it is basically just destroyed and
GC''d, nothing is ever written to xenstore - the path is just
rm''d.
The point of this feature is to take the guesswork out of event
handling of xen domains.
My general idea is to add a config option for a path to an event
handler script:
on_shutdown_event = "/etc/xen/scripts/shutdown-event"
Create the event during handle_domain_death in xl_cmdimpl.c and fire
the script once shutdown reason and action has been parsed.
I implemented a hacky version to illustrate my point but I would like
some advice on how to do this properly and what tools/framework within
libxl I could leverage to make it cleaner.
A quick overview of the following would help immensely:
Where to add in a new config option and what steps it goes through to
get to libxl_domain_config.
How to exec an external script safely, can I use usual fork/exec or
should I be using libxl__exec or libxl_spawn*?
Best place/way to get the event data, atm handle_domain_death looks
like an easy target but there might be more elegant ways..
Patch to show what I want to do is below, but it''s nasty and only
serves to illustrate my intented outcome.
I will probably write up some wiki articles to help others that want
to go down this path too. :)
Thanks in advance and kind regards,
Joseph.
diff --git a/tools/hotplug/Linux/Makefile b/tools/hotplug/Linux/Makefile
index 5401592..6b3a825 100644
--- a/tools/hotplug/Linux/Makefile
+++ b/tools/hotplug/Linux/Makefile
@@ -22,6 +22,7 @@ XEN_SCRIPTS += vtpm vtpm-delete
XEN_SCRIPTS += xen-hotplug-cleanup
XEN_SCRIPTS += external-device-migrate
XEN_SCRIPTS += vscsi
+XEN_SCRIPTS += shutdown-event
XEN_SCRIPT_DATA = xen-script-common.sh locking.sh logging.sh
XEN_SCRIPT_DATA += xen-hotplug-common.sh xen-network-common.sh vif-common.sh
XEN_SCRIPT_DATA += block-common.sh vtpm-common.sh vtpm-hotplug-common.sh
diff --git a/tools/hotplug/Linux/shutdown-event
b/tools/hotplug/Linux/shutdown-event
new file mode 100644
index 0000000..3f49d16
--- /dev/null
+++ b/tools/hotplug/Linux/shutdown-event
@@ -0,0 +1,19 @@
+#!/bin/bash
+event=$1
+name=$2
+domid=$3
+case $event in
+ poweroff)
+ # Handle action on poweroff shutdown here
+ ;;
+ reboot)
+ # Handle action on reboot shutdown here
+ ;;
+ crash)
+ # Handle action on crash shutdown here
+ ;;
+ watchdog)
+ # Handle action on watchdog shutdown here
+ ;;
+ unknown)
+ # Handle unknown shutdown reason here
+ ;;
+esac
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 2df96b8..94cc4c5 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1160,31 +1160,40 @@ skip_vfb:
xlu_cfg_destroy(config);
}
+#define SHUTDOWN_EVENT_SCRIPT "/etc/xen/scripts/shutdown-event %s %s
%d"
+
/* Returns 1 if domain should be restarted, 2 if domain should be
renamed then restarted */
static int handle_domain_death(libxl_ctx *ctx, uint32_t domid,
libxl_event *event,
libxl_domain_config *d_config,
libxl_dominfo *info)
{
- int restart = 0;
+ int restart = 0, ret;
enum libxl_action_on_shutdown action;
+ char event_name[10];
+ char event_cmd[100];
switch (info->shutdown_reason) {
case SHUTDOWN_poweroff:
action = d_config->on_poweroff;
+ strcpy(event_name,"poweroff");
break;
case SHUTDOWN_reboot:
action = d_config->on_reboot;
+ strcpy(event_name,"reboot");
break;
case SHUTDOWN_suspend:
return 0;
case SHUTDOWN_crash:
action = d_config->on_crash;
+ strcpy(event_name,"crash");
break;
case SHUTDOWN_watchdog:
action = d_config->on_watchdog;
+ strcpy(event_name,"watchdog");
break;
default:
LOG("Unknown shutdown reason code %d. Destroying domain.",
info->shutdown_reason);
action = LIBXL_ACTION_DESTROY;
+ strcpy(event_name,"unknown");
}
LOG("Action for shutdown reason code %d is %s",
info->shutdown_reason, action_on_shutdown_names[action]);
@@ -1230,6 +1239,8 @@ static int handle_domain_death(libxl_ctx *ctx,
uint32_t domid, libxl_event *even
abort();
}
+ snprintf(event_cmd, 100, SHUTDOWN_EVENT_SCRIPT, event_name,
d_config->c_info.name, domid);
+ ret = system(event_cmd);
return restart;
}
--
Founder | Director | VP Research
Orion Virtualisation Solutions | www.orionvm.com.au | Phone: 1300 56
99 52 | Mobile: 0428 754 846
Joseph Glanville writes ("[RFC] Shutdown event script for xl
toolstack"):> Previously I had a series of out of tree patches to xend that added a
> shutdown event callback on domain death that would call a script with
> the reason why the domain shutdown.
> Basically analagous to this:
> /etc/xen/scripts/shutdown-event $event $domname $domid
I think this is a good idea, although it shouldn''t be enabled by
default and I don''t see the need to provide an example script.
> My general idea is to add a config option for a path to an event
> handler script:
> on_shutdown_event = "/etc/xen/scripts/shutdown-event"
Yes, this is good. Shouldn''t the argument be a string list so that
you can specify all the arguments to exec() ? Will the script inherit
the domid or name in an environment variable ?
You need to document this fully I think, in docs/man/xl.cfg.pod.5.
It would also be good for the script to be able to give instructions
to the daemonic xl, for example to cause the daemonic xl to neither
reboot nor preserve the domain. So if you wanted your domain to get a
different config when it reboots, you write a hook script which
destroys the previous domain and starts the new one by hand.
> Create the event during handle_domain_death in xl_cmdimpl.c and fire
> the script once shutdown reason and action has been parsed.
When you say "create the event" what do you mean ?
> I implemented a hacky version to illustrate my point but I would like
> some advice on how to do this properly and what tools/framework within
> libxl I could leverage to make it cleaner.
This is going in the right direction. I''ll comment in more detail
below.
> A quick overview of the following would help immensely:
> Where to add in a new config option and what steps it goes through to
> get to libxl_domain_config.
This hook script functionality should be part of xl, not part of
libxl, so arguably you shouldn''t add it to libxl_domain_config.
Indeed I think it''s arguably a mistake that the on_{poweroff,...}
items are in libxl_domain_config rather than in something provided in
xl.
The config parsing is done in parse_config_data.
> How to exec an external script safely, can I use usual fork/exec or
> should I be using libxl__exec or libxl_spawn*?
In xl I think you should use fork/exec. You may not use any functions
libxl__* as they are private for libxl.
> Best place/way to get the event data, atm handle_domain_death looks
> like an easy target but there might be more elegant ways..
handle_domain_death is called in only one place, the event loop in the
daemonic xl. And yes, it seems like the right place to do this.
> diff --git a/tools/hotplug/Linux/shutdown-event
> b/tools/hotplug/Linux/shutdown-event
As I say I don''t think we need an example like this.
Also do we think asking the user to handle this with a switch in their
event script is really better than providing four separate config
options for separate commands ? Doing the latter will make the
scripts simpler, which is good because they''ll be very annoying to
debug.
> + char event_name[10];
> + char event_cmd[100];
>
> switch (info->shutdown_reason) {
> case SHUTDOWN_poweroff:
> action = d_config->on_poweroff;
> + strcpy(event_name,"poweroff");
Surely event_name should be
const char *event_name;
and then you can do
event_name = "poweroff";
But this goes away if you have four separate hook script config
options.
> + snprintf(event_cmd, 100, SHUTDOWN_EVENT_SCRIPT, event_name,
> d_config->c_info.name, domid);
> + ret = system(event_cmd);
This part needs serious work. We need firstly to define an
interface. And, I don''t really like system(). You should be using
fork/exec.
Thanks,
Ian.
On 31 March 2012 01:03, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:> Joseph Glanville writes ("[RFC] Shutdown event script for xl toolstack"): >> Previously I had a series of out of tree patches to xend that added a >> shutdown event callback on domain death that would call a script with >> the reason why the domain shutdown. >> Basically analagous to this: >> /etc/xen/scripts/shutdown-event $event $domname $domid > > I think this is a good idea, although it shouldn''t be enabled by > default and I don''t see the need to provide an example script.Yep fair enough, agree on both points.> >> My general idea is to add a config option for a path to an event >> handler script: >> on_shutdown_event = "/etc/xen/scripts/shutdown-event" > > Yes, this is good. Shouldn''t the argument be a string list so that > you can specify all the arguments to exec() ? Will the script inherit > the domid or name in an environment variable ? > > You need to document this fully I think, in docs/man/xl.cfg.pod.5.I didn''t think of this but you make a very valid point, especially considering we can use environment vars to provide the domain data as you suggested.> > It would also be good for the script to be able to give instructions > to the daemonic xl, for example to cause the daemonic xl to neither > reboot nor preserve the domain. So if you wanted your domain to get a > different config when it reboots, you write a hook script which > destroys the previous domain and starts the new one by hand.Hmm this is interesting and something I hadn''t really thought of implementing. Something that uses the return code of the script I think would be appropriate.> >> Create the event during handle_domain_death in xl_cmdimpl.c and fire >> the script once shutdown reason and action has been parsed. > > When you say "create the event" what do you mean ?I think "create" was most definitely incorrect terminology. :) "catch" probably would have been a much better way of terming it now I look at it.> >> I implemented a hacky version to illustrate my point but I would like >> some advice on how to do this properly and what tools/framework within >> libxl I could leverage to make it cleaner. > > This is going in the right direction. I''ll comment in more detail > below. > >> A quick overview of the following would help immensely: >> Where to add in a new config option and what steps it goes through to >> get to libxl_domain_config. > > This hook script functionality should be part of xl, not part of > libxl, so arguably you shouldn''t add it to libxl_domain_config. > > Indeed I think it''s arguably a mistake that the on_{poweroff,...} > items are in libxl_domain_config rather than in something provided in > xl. > > The config parsing is done in parse_config_data.Cheers will take a look.> >> How to exec an external script safely, can I use usual fork/exec or >> should I be using libxl__exec or libxl_spawn*? > > In xl I think you should use fork/exec. You may not use any functions > libxl__* as they are private for libxl.Thanks for clarifying, I also read the naming convention docs this morning which cleared this up too.> >> Best place/way to get the event data, atm handle_domain_death looks >> like an easy target but there might be more elegant ways.. > > handle_domain_death is called in only one place, the event loop in the > daemonic xl. And yes, it seems like the right place to do this. > >> diff --git a/tools/hotplug/Linux/shutdown-event >> b/tools/hotplug/Linux/shutdown-event > > As I say I don''t think we need an example like this. > > Also do we think asking the user to handle this with a switch in their > event script is really better than providing four separate config > options for separate commands ? Doing the latter will make the > scripts simpler, which is good because they''ll be very annoying to > debug.Agreed, that is a much better approach.> >> + char event_name[10]; >> + char event_cmd[100]; >> >> switch (info->shutdown_reason) { >> case SHUTDOWN_poweroff: >> action = d_config->on_poweroff; >> + strcpy(event_name,"poweroff"); > > Surely event_name should be > const char *event_name; > and then you can do > event_name = "poweroff"; > > But this goes away if you have four separate hook script config > options. > >> + snprintf(event_cmd, 100, SHUTDOWN_EVENT_SCRIPT, event_name, >> d_config->c_info.name, domid); >> + ret = system(event_cmd); > > This part needs serious work. We need firstly to define an > interface. And, I don''t really like system(). You should be using > fork/exec.Yep, now that has been clarified I will work on something cleaner. I think taking the array of string approach in the config file and then parsing that array verbatim as the args to exec is a great idea.> > Thanks, > Ian.I will refactor this into a v1 patch after I work out the config file stuff, sensible script interface and return values etc. Thanks for all of your help! Joseph. -- Founder | Director | VP Research Orion Virtualisation Solutions | www.orionvm.com.au | Phone: 1300 56 99 52 | Mobile: 0428 754 846
On 31 March 2012 05:08, Joseph Glanville <joseph.glanville@orionvm.com.au> wrote:> On 31 March 2012 01:03, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote: >> Joseph Glanville writes ("[RFC] Shutdown event script for xl toolstack"): >>> Previously I had a series of out of tree patches to xend that added a >>> shutdown event callback on domain death that would call a script with >>> the reason why the domain shutdown. >>> Basically analagous to this: >>> /etc/xen/scripts/shutdown-event $event $domname $domid >> >> I think this is a good idea, although it shouldn''t be enabled by >> default and I don''t see the need to provide an example script. > > Yep fair enough, agree on both points. > >> >>> My general idea is to add a config option for a path to an event >>> handler script: >>> on_shutdown_event = "/etc/xen/scripts/shutdown-event" >> >> Yes, this is good. Shouldn''t the argument be a string list so that >> you can specify all the arguments to exec() ? Will the script inherit >> the domid or name in an environment variable ? >> >> You need to document this fully I think, in docs/man/xl.cfg.pod.5. > > I didn''t think of this but you make a very valid point, especially > considering we can use environment vars to provide the domain data as > you suggested. > >> >> It would also be good for the script to be able to give instructions >> to the daemonic xl, for example to cause the daemonic xl to neither >> reboot nor preserve the domain. So if you wanted your domain to get a >> different config when it reboots, you write a hook script which >> destroys the previous domain and starts the new one by hand. > > Hmm this is interesting and something I hadn''t really thought of implementing. > Something that uses the return code of the script I think would be appropriate. > >> >>> Create the event during handle_domain_death in xl_cmdimpl.c and fire >>> the script once shutdown reason and action has been parsed. >> >> When you say "create the event" what do you mean ? > > I think "create" was most definitely incorrect terminology. :) > "catch" probably would have been a much better way of terming it now I > look at it. > >> >>> I implemented a hacky version to illustrate my point but I would like >>> some advice on how to do this properly and what tools/framework within >>> libxl I could leverage to make it cleaner. >> >> This is going in the right direction. I''ll comment in more detail >> below. >> >>> A quick overview of the following would help immensely: >>> Where to add in a new config option and what steps it goes through to >>> get to libxl_domain_config. >> >> This hook script functionality should be part of xl, not part of >> libxl, so arguably you shouldn''t add it to libxl_domain_config. >> >> Indeed I think it''s arguably a mistake that the on_{poweroff,...} >> items are in libxl_domain_config rather than in something provided in >> xl. >> >> The config parsing is done in parse_config_data. > > Cheers will take a look. > >> >>> How to exec an external script safely, can I use usual fork/exec or >>> should I be using libxl__exec or libxl_spawn*? >> >> In xl I think you should use fork/exec. You may not use any functions >> libxl__* as they are private for libxl. > > Thanks for clarifying, I also read the naming convention docs this > morning which cleared this up too. > >> >>> Best place/way to get the event data, atm handle_domain_death looks >>> like an easy target but there might be more elegant ways.. >> >> handle_domain_death is called in only one place, the event loop in the >> daemonic xl. And yes, it seems like the right place to do this. >> >>> diff --git a/tools/hotplug/Linux/shutdown-event >>> b/tools/hotplug/Linux/shutdown-event >> >> As I say I don''t think we need an example like this. >> >> Also do we think asking the user to handle this with a switch in their >> event script is really better than providing four separate config >> options for separate commands ? Doing the latter will make the >> scripts simpler, which is good because they''ll be very annoying to >> debug. > > Agreed, that is a much better approach. > >> >>> + char event_name[10]; >>> + char event_cmd[100]; >>> >>> switch (info->shutdown_reason) { >>> case SHUTDOWN_poweroff: >>> action = d_config->on_poweroff; >>> + strcpy(event_name,"poweroff"); >> >> Surely event_name should be >> const char *event_name; >> and then you can do >> event_name = "poweroff"; >> >> But this goes away if you have four separate hook script config >> options. >> >>> + snprintf(event_cmd, 100, SHUTDOWN_EVENT_SCRIPT, event_name, >>> d_config->c_info.name, domid); >>> + ret = system(event_cmd); >> >> This part needs serious work. We need firstly to define an >> interface. And, I don''t really like system(). You should be using >> fork/exec. > > Yep, now that has been clarified I will work on something cleaner. > I think taking the array of string approach in the config file and > then parsing that array verbatim as the args to exec is a great idea. > >> >> Thanks, >> Ian. > > I will refactor this into a v1 patch after I work out the config file > stuff, sensible script interface and return values etc. > > Thanks for all of your help! > > Joseph. > > -- > Founder | Director | VP Research > Orion Virtualisation Solutions | www.orionvm.com.au | Phone: 1300 56 > 99 52 | Mobile: 0428 754 846Hi Ian, I have a few more questions. As I have become more acquainted with the codebase I can see the line between libxl and xl quite clearly. However the config file territory seems somewhat ambiguous. Domain configuration is stored in libxl_domain_config which is a part of libxl but there isn''t currently a structure for data that is private to xl, that is xl daemon or utility specific configuration. Is it considered bad form to add a configuration variable to libxl_domain_config that only xl will benefit from? if so what is the best course of action here? I could create another structure for private data but I feel seeking guidance on this as prudent. In the meantime I had added it to libxl_domain_config but I intend to have found/made a cleaner solution before submitting the patch for inclusion. In terms of interface I have come up with what I think is inherently simple and reliable. The shutdown_event_handler is executed with DOM_ID, DOM_NAME, ACTION and REASON in it''s environment. The return code stipulates what action xl will then take, in correspondence with the LIBXL_ACTION_ON_SHUTDOWN* counterparts. I am yet to document this in the xl.cfg.pod file, I will do so in the next revision along with adding support for arguments - once we have where we are going to store them sorted out at least. Patch is only compile tested at this time, I am away from my testing environment atm. Thanks in advance and kind regards, Joseph. diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h index 6b69030..b3e5fb1 100644 --- a/tools/libxl/libxl.h +++ b/tools/libxl/libxl.h @@ -370,6 +370,8 @@ typedef struct { libxl_action_on_shutdown on_reboot; libxl_action_on_shutdown on_watchdog; libxl_action_on_shutdown on_crash; + + char *shutdown_event_handler; } libxl_domain_config; char *libxl_domain_config_to_json(libxl_ctx *ctx, libxl_domain_config *p); diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c index 1d59b89..1096f23 100644 --- a/tools/libxl/xl_cmdimpl.c +++ b/tools/libxl/xl_cmdimpl.c @@ -664,6 +664,10 @@ static void parse_config_data(const char *configfile_filename_report, exit(1); } + if (xlu_cfg_replace_string (config, "shutdown_event_handler", + &d_config->shutdown_event_handler, 0)) + d_config->shutdown_event_handler = NULL; + /* libxl_get_required_shadow_memory() must be called after final values * (default or specified) for vcpus and memory are set, because the * calculation depends on those values. */ @@ -1206,6 +1210,63 @@ skip_vfb: xlu_cfg_destroy(config); } +static int xl_exec(const char *arg0, char **args, char **envp, + useconds_t timeout) +{ + int status; + useconds_t sleep_time = 10, wait_time = 0; + pid_t child_pid, wpid; + + child_pid = fork(); + if (child_pid < 0) { + LOG("Failed to fork in xl_exec"); + exit(-1); + } else if (child_pid == 0) { + execvpe(arg0,args,envp); + } else { + do { + wpid = waitpid(child_pid, &status, WNOHANG); + if (wpid == 0) { + if (wait_time < timeout) { + usleep(sleep_time); + wait_time += sleep_time; + } else { + kill(child_pid, SIGKILL); + } + } + } while (wpid == 0 && wait_time <= timeout); + + if (WIFSIGNALED(status)) { + return WEXITSTATUS(status); + } + } + return WTERMSIG(status); +} + +static int user_shutdown_action(libxl_domain_config *d_config, uint32_t domid, + int action, unsigned shutdown_reason) +{ + int ret; + const char* arg0 = d_config->shutdown_event_handler; + char* argv[] = {NULL}; + char* envp[4]; + useconds_t timeout = 1000; + + asprintf(&envp[0],"DOM_ID=%d", domid); + asprintf(&envp[1],"DOM_NAME=%s", d_config->c_info.name); + asprintf(&envp[2],"ACTION=%d", action); + asprintf(&envp[3],"REASON=%u", shutdown_reason); + envp[4] = NULL; + + ret = xl_exec(arg0,argv,envp,timeout); + + if ((ret < 0) || ( ret > 6)) { + LOG("Invalid shutdown action requested: %d, ignoring",ret); + return action; + } + return ret; +} + /* Returns 1 if domain should be restarted, * 2 if domain should be renamed then restarted, or 0 */ static int handle_domain_death(libxl_ctx *ctx, uint32_t domid, @@ -1237,6 +1298,10 @@ static int handle_domain_death(libxl_ctx *ctx, uint32_t domid, action = LIBXL_ACTION_ON_SHUTDOWN_DESTROY; } + if (d_config->shutdown_event_handler) + action = user_shutdown_action(d_config, domid, action, + event->u.domain_shutdown.shutdown_reason); + LOG("Action for shutdown reason code %d is %s", event->u.domain_shutdown.shutdown_reason, action_on_shutdown_names[action]) -- Founder | Director | VP Research Orion Virtualisation Solutions | www.orionvm.com.au | Phone: 1300 56 99 52 | Mobile: 0428 754 846
On 1 April 2012 09:13, Joseph Glanville <joseph.glanville@orionvm.com.au> wrote:> On 31 March 2012 05:08, Joseph Glanville > <joseph.glanville@orionvm.com.au> wrote: >> On 31 March 2012 01:03, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote: >>> Joseph Glanville writes ("[RFC] Shutdown event script for xl toolstack"): >>>> Previously I had a series of out of tree patches to xend that added a >>>> shutdown event callback on domain death that would call a script with >>>> the reason why the domain shutdown. >>>> Basically analagous to this: >>>> /etc/xen/scripts/shutdown-event $event $domname $domid >>> >>> I think this is a good idea, although it shouldn''t be enabled by >>> default and I don''t see the need to provide an example script. >> >> Yep fair enough, agree on both points. >> >>> >>>> My general idea is to add a config option for a path to an event >>>> handler script: >>>> on_shutdown_event = "/etc/xen/scripts/shutdown-event" >>> >>> Yes, this is good. Shouldn''t the argument be a string list so that >>> you can specify all the arguments to exec() ? Will the script inherit >>> the domid or name in an environment variable ? >>> >>> You need to document this fully I think, in docs/man/xl.cfg.pod.5. >> >> I didn''t think of this but you make a very valid point, especially >> considering we can use environment vars to provide the domain data as >> you suggested. >> >>> >>> It would also be good for the script to be able to give instructions >>> to the daemonic xl, for example to cause the daemonic xl to neither >>> reboot nor preserve the domain. So if you wanted your domain to get a >>> different config when it reboots, you write a hook script which >>> destroys the previous domain and starts the new one by hand. >> >> Hmm this is interesting and something I hadn''t really thought of implementing. >> Something that uses the return code of the script I think would be appropriate. >> >>> >>>> Create the event during handle_domain_death in xl_cmdimpl.c and fire >>>> the script once shutdown reason and action has been parsed. >>> >>> When you say "create the event" what do you mean ? >> >> I think "create" was most definitely incorrect terminology. :) >> "catch" probably would have been a much better way of terming it now I >> look at it. >> >>> >>>> I implemented a hacky version to illustrate my point but I would like >>>> some advice on how to do this properly and what tools/framework within >>>> libxl I could leverage to make it cleaner. >>> >>> This is going in the right direction. I''ll comment in more detail >>> below. >>> >>>> A quick overview of the following would help immensely: >>>> Where to add in a new config option and what steps it goes through to >>>> get to libxl_domain_config. >>> >>> This hook script functionality should be part of xl, not part of >>> libxl, so arguably you shouldn''t add it to libxl_domain_config. >>> >>> Indeed I think it''s arguably a mistake that the on_{poweroff,...} >>> items are in libxl_domain_config rather than in something provided in >>> xl. >>> >>> The config parsing is done in parse_config_data. >> >> Cheers will take a look. >> >>> >>>> How to exec an external script safely, can I use usual fork/exec or >>>> should I be using libxl__exec or libxl_spawn*? >>> >>> In xl I think you should use fork/exec. You may not use any functions >>> libxl__* as they are private for libxl. >> >> Thanks for clarifying, I also read the naming convention docs this >> morning which cleared this up too. >> >>> >>>> Best place/way to get the event data, atm handle_domain_death looks >>>> like an easy target but there might be more elegant ways.. >>> >>> handle_domain_death is called in only one place, the event loop in the >>> daemonic xl. And yes, it seems like the right place to do this. >>> >>>> diff --git a/tools/hotplug/Linux/shutdown-event >>>> b/tools/hotplug/Linux/shutdown-event >>> >>> As I say I don''t think we need an example like this. >>> >>> Also do we think asking the user to handle this with a switch in their >>> event script is really better than providing four separate config >>> options for separate commands ? Doing the latter will make the >>> scripts simpler, which is good because they''ll be very annoying to >>> debug. >> >> Agreed, that is a much better approach. >> >>> >>>> + char event_name[10]; >>>> + char event_cmd[100]; >>>> >>>> switch (info->shutdown_reason) { >>>> case SHUTDOWN_poweroff: >>>> action = d_config->on_poweroff; >>>> + strcpy(event_name,"poweroff"); >>> >>> Surely event_name should be >>> const char *event_name; >>> and then you can do >>> event_name = "poweroff"; >>> >>> But this goes away if you have four separate hook script config >>> options. >>> >>>> + snprintf(event_cmd, 100, SHUTDOWN_EVENT_SCRIPT, event_name, >>>> d_config->c_info.name, domid); >>>> + ret = system(event_cmd); >>> >>> This part needs serious work. We need firstly to define an >>> interface. And, I don''t really like system(). You should be using >>> fork/exec. >> >> Yep, now that has been clarified I will work on something cleaner. >> I think taking the array of string approach in the config file and >> then parsing that array verbatim as the args to exec is a great idea. >> >>> >>> Thanks, >>> Ian. >> >> I will refactor this into a v1 patch after I work out the config file >> stuff, sensible script interface and return values etc. >> >> Thanks for all of your help! >> >> Joseph. >> >> -- >> Founder | Director | VP Research >> Orion Virtualisation Solutions | www.orionvm.com.au | Phone: 1300 56 >> 99 52 | Mobile: 0428 754 846 > > Hi Ian, > > I have a few more questions. > > As I have become more acquainted with the codebase I can see the line > between libxl and xl quite clearly. > However the config file territory seems somewhat ambiguous. > Domain configuration is stored in libxl_domain_config which is a part > of libxl but there isn''t currently a structure for data that is > private to xl, that is xl daemon or utility specific configuration. > Is it considered bad form to add a configuration variable to > libxl_domain_config that only xl will benefit from? if so what is the > best course of action here? > I could create another structure for private data but I feel seeking > guidance on this as prudent. > > In the meantime I had added it to libxl_domain_config but I intend to > have found/made a cleaner solution before submitting the patch for > inclusion. > > In terms of interface I have come up with what I think is inherently > simple and reliable. > The shutdown_event_handler is executed with DOM_ID, DOM_NAME, ACTION > and REASON in it''s environment. > The return code stipulates what action xl will then take, in > correspondence with the LIBXL_ACTION_ON_SHUTDOWN* counterparts. > > I am yet to document this in the xl.cfg.pod file, I will do so in the > next revision along with adding support for arguments - once we have > where we are going to store them sorted out at least. > Patch is only compile tested at this time, I am away from my testing > environment atm. > > Thanks in advance and kind regards, > Joseph. > > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h > index 6b69030..b3e5fb1 100644 > --- a/tools/libxl/libxl.h > +++ b/tools/libxl/libxl.h > @@ -370,6 +370,8 @@ typedef struct { > libxl_action_on_shutdown on_reboot; > libxl_action_on_shutdown on_watchdog; > libxl_action_on_shutdown on_crash; > + > + char *shutdown_event_handler; > } libxl_domain_config; > char *libxl_domain_config_to_json(libxl_ctx *ctx, libxl_domain_config *p); > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > index 1d59b89..1096f23 100644 > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -664,6 +664,10 @@ static void parse_config_data(const char > *configfile_filename_report, > exit(1); > } > > + if (xlu_cfg_replace_string (config, "shutdown_event_handler", > + &d_config->shutdown_event_handler, 0)) > + d_config->shutdown_event_handler = NULL; > + > /* libxl_get_required_shadow_memory() must be called after final values > * (default or specified) for vcpus and memory are set, because the > * calculation depends on those values. */ > @@ -1206,6 +1210,63 @@ skip_vfb: > xlu_cfg_destroy(config); > } > > +static int xl_exec(const char *arg0, char **args, char **envp, > + useconds_t timeout) > +{ > + int status; > + useconds_t sleep_time = 10, wait_time = 0; > + pid_t child_pid, wpid; > + > + child_pid = fork(); > + if (child_pid < 0) { > + LOG("Failed to fork in xl_exec"); > + exit(-1); > + } else if (child_pid == 0) { > + execvpe(arg0,args,envp); > + } else { > + do { > + wpid = waitpid(child_pid, &status, WNOHANG); > + if (wpid == 0) { > + if (wait_time < timeout) { > + usleep(sleep_time); > + wait_time += sleep_time; > + } else { > + kill(child_pid, SIGKILL); > + } > + } > + } while (wpid == 0 && wait_time <= timeout); > + > + if (WIFSIGNALED(status)) { > + return WEXITSTATUS(status); > + } > + } > + return WTERMSIG(status); > +}Ahh I fumbled it. Obviously WTERMSIG and WEXITSTATUS should be reversed here. No idea how I missed this on my last glance over.> + > +static int user_shutdown_action(libxl_domain_config *d_config, uint32_t domid, > + int action, unsigned shutdown_reason) > +{ > + int ret; > + const char* arg0 = d_config->shutdown_event_handler; > + char* argv[] = {NULL}; > + char* envp[4]; > + useconds_t timeout = 1000; > + > + asprintf(&envp[0],"DOM_ID=%d", domid); > + asprintf(&envp[1],"DOM_NAME=%s", d_config->c_info.name); > + asprintf(&envp[2],"ACTION=%d", action); > + asprintf(&envp[3],"REASON=%u", shutdown_reason); > + envp[4] = NULL; > + > + ret = xl_exec(arg0,argv,envp,timeout); > + > + if ((ret < 0) || ( ret > 6)) { > + LOG("Invalid shutdown action requested: %d, ignoring",ret); > + return action; > + } > + return ret; > +} > + > /* Returns 1 if domain should be restarted, > * 2 if domain should be renamed then restarted, or 0 */ > static int handle_domain_death(libxl_ctx *ctx, uint32_t domid, > @@ -1237,6 +1298,10 @@ static int handle_domain_death(libxl_ctx *ctx, > uint32_t domid, > action = LIBXL_ACTION_ON_SHUTDOWN_DESTROY; > } > > + if (d_config->shutdown_event_handler) > + action = user_shutdown_action(d_config, domid, action, > + > event->u.domain_shutdown.shutdown_reason); > + > LOG("Action for shutdown reason code %d is %s", > event->u.domain_shutdown.shutdown_reason, > action_on_shutdown_names[action]) > > -- > Founder | Director | VP Research > Orion Virtualisation Solutions | www.orionvm.com.au | Phone: 1300 56 > 99 52 | Mobile: 0428 754 846-- Founder | Director | VP Research Orion Virtualisation Solutions | www.orionvm.com.au | Phone: 1300 56 99 52 | Mobile: 0428 754 846
On Sun, 2012-04-01 at 00:13 +0100, Joseph Glanville wrote:> Domain configuration is stored in libxl_domain_config which is a part > of libxl but there isn''t currently a structure for data that is > private to xl, that is xl daemon or utility specific configuration. > Is it considered bad form to add a configuration variable to > libxl_domain_config that only xl will benefit from? if so what is the > best course of action here? > I could create another structure for private data but I feel seeking > guidance on this as prudent.xl_cmdimpl.c defines struct domain_create to contain a bunch of this sort of thing, is that a convenient location? Ian.
On 2 April 2012 18:46, Ian Campbell <Ian.Campbell@citrix.com> wrote:> On Sun, 2012-04-01 at 00:13 +0100, Joseph Glanville wrote: >> Domain configuration is stored in libxl_domain_config which is a part >> of libxl but there isn''t currently a structure for data that is >> private to xl, that is xl daemon or utility specific configuration. >> Is it considered bad form to add a configuration variable to >> libxl_domain_config that only xl will benefit from? if so what is the >> best course of action here? >> I could create another structure for private data but I feel seeking >> guidance on this as prudent. > > xl_cmdimpl.c defines struct domain_create to contain a bunch of this > sort of thing, is that a convenient location?That could work. domain_create isn''t currently passed to parse_config_data or handle_domain_death yet but it seems like a reasonable place to put this stuff> > Ian. >I will rehash this tonight once I work out how the xlu_cfg_get_list stuff works and post a proper v1. Joseph. -- Founder | Director | VP Research Orion Virtualisation Solutions | www.orionvm.com.au | Phone: 1300 56 99 52 | Mobile: 0428 754 846
Thanks for your submission; this is coming along but still needs some
work.
Joseph Glanville writes ("Re: [Xen-devel] [RFC] Shutdown event script for
xl toolstack"):> On 31 March 2012 05:08, Joseph Glanville
> <joseph.glanville@orionvm.com.au> wrote:
> + execvpe(arg0,args,envp);
I''m afraid that execvpe doesn''t do what you want: it entirely
replaces
the environment with the one you specify. You need to use putenv or
setenv.
> + if (xlu_cfg_replace_string (config,
"shutdown_event_handler",
> + &d_config->shutdown_event_handler, 0))
> + d_config->shutdown_event_handler = NULL;
Weren''t we going to have one event handler per type of event ?
> +{
> + int status;
> + useconds_t sleep_time = 10, wait_time = 0;
> + pid_t child_pid, wpid;
I''m afraid this timeout approach is not correct; you would need
something to interrupt the wait, rather than polling like this. I
think this is impractical. You should, rather, just not implement a
timeout.
I would advise against reusing exit statuses 0..6. You should start
your new exit statuses at 50 or something. This is because many
programs use status 1 to mean "I failed" and you would want to avoid
that being misinterpreted.
Other exit statuses, and deaths due to signals, should be reported
properly as errors using libxl_report_child_exitstatus.
> + ret = xl_exec(arg0,argv,envp,timeout);
I think calling this function xl_exec is a bit unfortunate. It
doesn''t just exec; it also forks.
> + if (d_config->shutdown_event_handler)
> + action = user_shutdown_action(d_config, domid, action,
> +
> event->u.domain_shutdown.shutdown_reason);
Something has linewrapped this. You should keep to within 75-80
lines.
Thanks,
Ian.
On 3 April 2012 01:00, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:> Thanks for your submission; this is coming along but still needs some > work. > > Joseph Glanville writes ("Re: [Xen-devel] [RFC] Shutdown event script for xl toolstack"): >> On 31 March 2012 05:08, Joseph Glanville >> <joseph.glanville@orionvm.com.au> wrote: >> + execvpe(arg0,args,envp); > > I''m afraid that execvpe doesn''t do what you want: it entirely replaces > the environment with the one you specify. You need to use putenv or > setenv.Ahh I see, my mistake.> >> + if (xlu_cfg_replace_string (config, "shutdown_event_handler", >> + &d_config->shutdown_event_handler, 0)) >> + d_config->shutdown_event_handler = NULL; > > Weren''t we going to have one event handler per type of event ?I have implemented the list stuff now so we can have arguments to to the handler and I also implemented it split into 4 handlers in the next revision.> >> +{ >> + int status; >> + useconds_t sleep_time = 10, wait_time = 0; >> + pid_t child_pid, wpid; > > I''m afraid this timeout approach is not correct; you would need > something to interrupt the wait, rather than polling like this. I > think this is impractical. You should, rather, just not implement a > timeout.Fair enough. I will drop the timeout.> > I would advise against reusing exit statuses 0..6. You should start > your new exit statuses at 50 or something. This is because many > programs use status 1 to mean "I failed" and you would want to avoid > that being misinterpreted. > > Other exit statuses, and deaths due to signals, should be reported > properly as errors using libxl_report_child_exitstatus.Ok, I will take a look at that and make use of it.> >> + ret = xl_exec(arg0,argv,envp,timeout); > > I think calling this function xl_exec is a bit unfortunate. It > doesn''t just exec; it also forks.Hmm that is true.. naming is not my strong point. xl_fork_exec? :)> >> + if (d_config->shutdown_event_handler) >> + action = user_shutdown_action(d_config, domid, action, >> + >> event->u.domain_shutdown.shutdown_reason); > > Something has linewrapped this. You should keep to within 75-80 > lines.Yep no problem.> > Thanks, > Ian.What is your call about moving this stuff into domain_create rather than libxl_domain_config? Thankyou for all of your comments/help, most appreciated. Joseph. -- Founder | Director | VP Research Orion Virtualisation Solutions | www.orionvm.com.au | Phone: 1300 56 99 52 | Mobile: 0428 754 846
Joseph Glanville writes ("Re: [Xen-devel] [RFC] Shutdown event script for
xl toolstack"):> [stuff]
Thanks for your reply.
> What is your call about moving this stuff into domain_create rather
> than libxl_domain_config?
I think that would be correct.
Thanks,
Ian.
On Fri, 2012-03-30 at 15:03 +0100, Ian Jackson wrote:> Indeed I think it''s arguably a mistake that the on_{poweroff,...} > items are in libxl_domain_config rather than in something provided in > xl.If we''re going to change that we should do it before the API gets declared stable in 4.2... Ian.
Ian Campbell writes ("Re: [Xen-devel] [RFC] Shutdown event script for xl
toolstack"):> On Fri, 2012-03-30 at 15:03 +0100, Ian Jackson wrote:
> > Indeed I think it''s arguably a mistake that the
on_{poweroff,...}
> > items are in libxl_domain_config rather than in something provided in
> > xl.
>
> If we''re going to change that we should do it before the API gets
> declared stable in 4.2...
Yes. Do you agree with my opinion that they should be moved ?
Ian.
On Tue, 2012-04-03 at 11:30 +0100, Ian Jackson wrote:> Ian Campbell writes ("Re: [Xen-devel] [RFC] Shutdown event script for xl toolstack"): > > On Fri, 2012-03-30 at 15:03 +0100, Ian Jackson wrote: > > > Indeed I think it''s arguably a mistake that the on_{poweroff,...} > > > items are in libxl_domain_config rather than in something provided in > > > xl. > > > > If we''re going to change that we should do it before the API gets > > declared stable in 4.2... > > Yes. Do you agree with my opinion that they should be moved ?It seems reasonable to err on the side of not including things in the API initially. The enum itself could be moved too but the IDL provided functions for the Enum are quite handy, it''d be a shame to lose them and I''m not sure how well the IDL will function for non-libxl use -- it uses some internal functions of which libxl__enum_from_string is of particular interest here. Ian.
Ian Campbell writes ("Re: [Xen-devel] [RFC] Shutdown event script for xl
toolstack"):> The enum itself could be moved too but the IDL provided functions for
> the Enum are quite handy, it''d be a shame to lose them and
I''m not sure
> how well the IDL will function for non-libxl use -- it uses some
> internal functions of which libxl__enum_from_string is of particular
> interest here.
Can libxl__enum_from_string be made public ? Or is this too much of a
ball of string ?
Ian.
On Tue, 2012-04-03 at 11:47 +0100, Ian Jackson wrote:> Ian Campbell writes ("Re: [Xen-devel] [RFC] Shutdown event script for xl toolstack"): > > The enum itself could be moved too but the IDL provided functions for > > the Enum are quite handy, it''d be a shame to lose them and I''m not sure > > how well the IDL will function for non-libxl use -- it uses some > > internal functions of which libxl__enum_from_string is of particular > > interest here. > > Can libxl__enum_from_string be made public ?I think that one is likely to be ok, it''s pretty simple and self contained. There others are though, like libxl__object_to_json, libxl__yajl_gen_enum and libxl__string_gen_json (I think that''s all). They''d need slightly more careful handling to restrict the exposure of yajl stuff only to callers who explicitly ask for libxl_json.h.> Or is this too much of a ball of string ?That''s my worry. Ian.