Re: [Xen-devel] [RFC] Shutdown event script for xl toolstack

[Joseph Glanville <joseph.glanville@xxxxxxxxxxxxxx>]

On 3 April 2012 01:00, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel