Re: [Xen-devel] [PATCH v3-RESEND 24/28] libxl: ocaml: add VM lifecycle operations

[Rob Hoes <Rob.Hoes@xxxxxxxxxx>]

> On Mon, 2013-10-21 at 14:32 +0100, Rob Hoes wrote:
> 
> > +static int domain_wait_event(libxl_ctx *ctx, int domid, libxl_event
> > +**event_r)
> 
> Do you not have the infrastructure in place to bind this to ocaml and do the
> next level of the call chain in ocaml instead of C?

It turns out that we are not actually using this anymore in our code, nor the 
wait_shutdown function introduced in this patch. We already have the 
lower-level event functionality implemented. I'll remove these from the patch.

> > +{
> > +   int ret;
> > +   for (;;) {
> > +           ret = libxl_event_wait(ctx, event_r, LIBXL_EVENTMASK_ALL,
> 0,0);
> > +           if (ret) {
> > +                   return ret;
> > +           }
> > +           if ((*event_r)->domid != domid) {
> > +                   libxl_event_free(CTX, *event_r);
> > +                   continue;
> > +           }
> > +           return ret;
> > +   }
> > +}
> > +
> > +value stub_libxl_domain_create_new(value ctx, value domain_config,
> > +value async, value unit) {
> > [...]
> > +   libxl_asyncop_how ao_how;
> > +
> > +   if (async != Val_none) {
> > +           ao_how.callback = async_callback;
> > +           ao_how.u.for_callback = (void *) Some_val(async);
> > +   }
> [...]
> > +           async != Val_none ? &ao_how : NULL, NULL);
> 
> Lots of this patten. What about
> #define OCAML_ASYNC_HOW \
>       libxl_asyncop_how _ao_how; \
>       libxl_asyncop_how *ao_how = NULL; \
>       if (async != Val_none) \
>               _ao_how.callback = ...
>               _ao_how.u.for_callback = ...
>               ao_how = &_ao_how
> 
> Then an unconditional ao_how at the point of use?
> 
> This relies on this file being compiled to allow mixed code and variable
> definitions. We do that for libxl itself, not sure about here. Could be
> enabled.
> 
> Or at least the initialisation of ao_how could be made a function.
> Perhaps
>       static value Val_oahow(value aohow struct aohow *c_aohow)
>       {
>               if (aohow == Val_none) return NULL;
> 
>               c_aohow->etc
>               return c_aohow
>       }
> 
> then
>       libxl_aohow aohow;
> 
>       libxl_do_foo(....
>               Val_aohow(async, &aohow), ...);

Yes, we should really separate this out indeed. I'll do that now.

Rob

> > +value stub_libxl_domain_wait_shutdown(value ctx, value domid) {
> 
> This is the thing I meant could you do in ocaml above...
> 
> > +   CAMLparam2(ctx, domid);
> > +   int ret;
> > +   libxl_event *event;
> > +   libxl_evgen_domain_death *deathw;
> > +   ret = libxl_evenable_domain_death(CTX, Int_val(domid), 0,
> &deathw);
> > +   if (ret)
> > +           failwith_xl(ret, "domain_wait_shutdown");
> > +
> > +   for (;;) {
> > +           ret = domain_wait_event(CTX, Int_val(domid), &event);
> > +           if (ret) {
> > +                   libxl_evdisable_domain_death(CTX, deathw);
> > +                   failwith_xl(ret, "domain_wait_shutdown");
> > +           }
> > +
> > +           switch (event->type) {
> > +           case LIBXL_EVENT_TYPE_DOMAIN_DEATH:
> > +                   goto done;
> > +           case LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN:
> > +                   goto done;
> > +           default:
> > +                   break;
> > +           }
> > +           libxl_event_free(CTX, event);
> > +   }
> > +done:
> > +   libxl_event_free(CTX, event);
> 
> I don't know, but this might be clearer with
>       the_right-type type = event->type;
>       lbixl_event_free(...)
> before the switch, which then becomes switch(type)?
> 
> Ian.


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