Re: [Xen-devel] [PATCH 3 of 9] libxl: signal caller if domain already destroyed on domain death event

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 26 Jul 2010, Ian Campbell wrote:
> On Mon, 2010-07-26 at 15:58 +0100, Stefano Stabellini wrote:
> > On Mon, 26 Jul 2010, Ian Campbell wrote:
> > > On Mon, 2010-07-26 at 15:32 +0100, Stefano Stabellini wrote:
> > > > > +                ret = libxl_event_get_domain_death_info(&ctx, domid, 
> > > > > &event, &info);
> > > > > +
> > > > > +                if (ret < 0) continue;
> > > > [....]
> > > > > +                    if (info.shutdown_reason != SHUTDOWN_suspend) {
> > > > >                          LOG("Domain %d needs to be clean: destroying 
> > > > > the domain", domid);
> > > > >                          libxl_domain_destroy(&ctx, domid, 0);
> > > > > -                        if (info.shutdown && info.shutdown_reason == 
> > > > > SHUTDOWN_reboot) {
> > > > > +                        if (info.shutdown_reason == SHUTDOWN_reboot) 
> > > > > {
> > > > 
> > > > Isn't still possible to get here and have info.shutdown == 0 (and even
> > > > info.dying == 0, after reading the fourth patch)?
> > > > If so, the previous test is probably clearer.
> > > 
> > > Umm...
> > > 
> > > I think libxl_event_get_domain_death_info should have returned
> > > ERROR_INVAL in that case and we'd have take the earlier if (ret < 0)
> > > continue.
> >  
> > I think we should at least write in a comment in libxl.h that
> > shutdown_reason is valid when (shutdown || dying) and that when
> > libxl_event_get_domain_death_info returns 1 shutdown_reason is always
> > set. 
> 
> Will do.
> 
> > Also I still like the old test more, just for clarity.
> 
> It would need to be "(info.shutdown||info.crashed||info.dying) ||
> info.shutdown_reason ....." which duplicates the logic in libxl, which I
> wanted to get rid of (I think that was fully realised the next patch or
> so)
> 

I only meant the second test:

if ((info.shutdown || info.dying) && info.shutdown_reason == SHUTDOWN_reboot)

or we can set shutdown_reason to LIBXL_SHUTDOWN_INVAL by default and
keep this test as it is.
I wouldn't want users to believe that shutdown_reason
is LIBXL_SHUTDOWN_POWEROFF just because is 0.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel