Re: [Xen-devel] [PATCH 3 of 6] libxl: QMP stop/resume & refactor QEMU suspend/resume/save

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2012-01-31 at 23:53 +0000, Shriram Rajagopalan wrote:
> On Tue, Jan 31, 2012 at 9:32 AM, Shriram Rajagopalan
> <rshriram@xxxxxxxxx> wrote:
>         On Tue, Jan 31, 2012 at 1:54 AM, Ian Campbell
>         <Ian.Campbell@xxxxxxxxxx> wrote:
>         
>                 On Tue, 2012-01-31 at 01:05 +0000, rshriram@xxxxxxxxx
>                 wrote:
>                 
>                 > +int libxl__domain_resume_device_model(libxl__gc
>                 *gc, uint32_t domid)
>                 > +{
>                 > +
>                 > +    switch (libxl__device_model_version_running(gc,
>                 domid)) {
>                 > +    case
>                 LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL: {
>                 > +        libxl__qemu_traditional_cmd(gc, domid,
>                 "continue");
>                 
>                 
>                 No libxl__wait_for_device_model -> "running"?
>                 
>                 
>         
>         Nope. Thats how xend/remus also does it.

That doesn't mean they are correct. How do you handle the error case
where qemu does not restart correctly?

>          There seems to be no reference to such a state anywhere.

I see several calls to libxl__wait_for_device_model with the parameter
running in the libxl code base:

$ rgrep -E "libxl__wait_for_device_model.*\"running\"" tools/libxl/
tools/libxl/libxl_pci.c:        if (libxl__wait_for_device_model(gc, domid, 
"running",
tools/libxl/libxl_pci.c:        if (libxl__wait_for_device_model(gc, domid, 
"running",
tools/libxl/libxl.c:            libxl__wait_for_device_model(gc, domid, 
"running",

>          [...]
>                 
>                 >
>                 > @@ -541,6 +596,17 @@ static int
>                 libxl__domain_suspend_common_
>                 >
>                 >      LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "guest did
>                 not suspend");
>                 >      return 0;
>                 > +
>                 > + suspend_dm:
>                 
>                 
>                 The goto's make the code flow a bit tricky to follow
>                 (this function is
>                 already a bit tricksy with the early exit for the
>                 evtchn suspend case).
>                 
>                 Why not pass si to libxl__domain_suspend_device_model
>                 and let it contain
>                 the "if !hvm return" and logging stuff so you can just
>                 call in in place
>                 of the two goto statements?
>                 
>                 
>         
>         will do. 
>         
>         
>         
> 
> I gave it a shot. Passing suspendinfo struct to suspend_device_model
> is not feasible, as the function is declared in libxl_internal.h, but
> suspendinfo struct declaration is local to libxl_dom.c.  I am not sure
> if its worthwhile to declare a private struct like suspendinfo in
> libxl_internal.h, just to get rid of this goto.

I see two choices, you could either have a forward declaration of
suspendinfo in libxl_internal.h or you could make
libxl__domain_suspend_device_model static/internal to libxl_dom.c. I
slightly prefer the latter unless there is some reason to expose it to
the rest of libxl.

The same would go for libxl__domain_resume_device_model apart from the
call from libxl_domain_resume in libxl.c. There's an argument that this
function should be in libxl_dom.c anyway.

> OTOH, passing a dummy hvm parameter to the function looks a bit silly
> 
> libxl__domain_suspend_device_model(libxl__gc *gc, uint32_t domid, int
> hvm)
> 
> 
> What do you think? goto needs to go? 

Taking another look at the control flow perhaps it does make sense, if
you think of the label as the tail of the success case rather than
something specific to suspending the device model then it removes the
two "return success" cases and combines them into a common tail, so long
as all success cases go through here (which I think is the case).

We still have multiple error exit cases, including the one where you
just fall through the loop (which combined with the unusual 0==error
return is a little confusing), but I think we can live with those.

So on second thoughts I think just renaming the label "guest_suspended"
would be fine.

Whether you also want to make
libxl__domain_{suspend,resume}_device_model internal to libxl_dom.c is
up to you.

Ian.


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