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

[Rob Hoes <Rob.Hoes@xxxxxxxxxx>]

> > +static int domain_wait_event(libxl_ctx *ctx, int domid, libxl_event
> > +**event_r) {
> > +   int ret;
> > +   for (;;) {
> > +           ret = libxl_event_wait(ctx, event_r, LIBXL_EVENTMASK_ALL,
> 0,0);
> > +           if (ret) {
> > +                   return ret;
> > +           }
> > +           if ((*event_r)->domid != domid) {
> > +                   char *evstr = libxl_event_to_json(CTX, *event_r);
> > +                   free(evstr);
> 
> Create/allocate the json and immediately free it? (left over debug
> perhaps?)

Yes :(
I will remove it.

> > +                   libxl_event_free(CTX, *event_r);
> > +                   continue;
> > +           }
> > +           return ret;
> > +   }
> > +}
> [...]
> > +value stub_xl_domain_create_restore(value ctx, value domain_config,
> > +value restore_fd) {
> > +   CAMLparam2(ctx, domain_config);
> > +   int ret;
> > +   libxl_domain_config c_dconfig;
> > +   uint32_t c_domid;
> > +
> > +   ret = domain_config_val(CTX, &c_dconfig, domain_config);
> > +   if (ret != 0)
> > +           failwith_xl(ret, "domain_create_restore");
> > +
> > +   ret = libxl_domain_create_restore(CTX, &c_dconfig, &c_domid,
> Int_val(restore_fd), NULL, NULL);
> > +   if (ret != 0)
> > +           failwith_xl(ret, "domain_create_restore");
> > +
> > +   libxl_domain_config_dispose(&c_dconfig);
> > +
> > +   CAMLreturn(Val_int(c_domid));
> > +}
> > +
> > +value stub_libxl_domain_wait_shutdown(value ctx, value domid) {
> > +   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) {
> > +           fprintf(stderr,"wait for death failed (evgen, rc=%d)\n",ret);
> > +           exit(-1);
> > +   }
> > +
> > +   for (;;) {
> > +           ret = domain_wait_event(CTX, Int_val(domid), &event);
> > +           if (ret)
> > +                   failwith_xl(ret, "domain_shutdown");
> 
> This exits asynchronously, which leaves the domain death event enabled.
> Depending on what your exception handler does this may not be what you
> want?

I think it is best to cleanup here itself. I'll fix that.

> This case has only just occurred to me, so there may be other instances in
> earlier patches...

I have looked through the code and indeed found a few more instances where 
cleanup is needed before raising an exception. 

Cheers,
Rob

> > +
> > +           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);
> > +   libxl_evdisable_domain_death(CTX, deathw);
> > +
> > +   CAMLreturn(Val_unit);
> > +}
> > +
> > +value stub_libxl_domain_shutdown(value ctx, value domid) {
> > +   CAMLparam2(ctx, domid);
> > +   int ret;
> > +
> > +   ret = libxl_domain_shutdown(CTX, Int_val(domid));
> > +
> > +   if (ret != 0)
> > +           failwith_xl(ret, "domain_shutdown");
> > +
> > +   CAMLreturn(Val_unit);
> > +}
> 
> This and the rest look pretty mechanical, I just skimmed it...
> 
> Ian.

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