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

[Shriram Rajagopalan <rshriram@cs.ubc.ca>]

[-- Attachment #1.1: Type: text/plain, Size: 2847 bytes --]

On Tue, Jan 31, 2012 at 9:32 AM, Shriram Rajagopalan <rshriram@cs.ubc.ca>wrote:

> On Tue, Jan 31, 2012 at 1:54 AM, Ian Campbell <Ian.Campbell@citrix.com>wrote:
>
>> On Tue, 2012-01-31 at 01:05 +0000, rshriram@cs.ubc.ca 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. There seems to be no reference to
> such
> a state anywhere.
>
>
>>  > +        break;
>> > +    }
>> > +    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
>> > +        if (libxl__qmp_resume(gc, domid))
>> > +            return ERROR_FAIL;
>> > +    default:
>> > +        return ERROR_INVAL;
>> > +    }
>> > +
>> > +    return 0;
>> > +}
>> > +
>> >  static int libxl__domain_suspend_common_callback(void *data)
>> >  {
>> >      struct suspendinfo *si = data;
>> > @@ -454,7 +509,7 @@ static int libxl__domain_suspend_common_
>> >              return 0;
>> >          }
>> >          si->guest_responded = 1;
>> > -        return 1;
>> > +        goto suspend_dm;
>> >      }
>> >
>> >      if (si->hvm && (!hvm_pvdrv || hvm_s_state)) {
>> > @@ -532,7 +587,7 @@ static int libxl__domain_suspend_common_
>> >              shutdown_reason = (info.flags >> XEN_DOMINF_shutdownshift)
>> & XEN_DOMINF_shutdownmask;
>> >              if (shutdown_reason == SHUTDOWN_suspend) {
>> >                  LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "guest has
>> suspended");
>> > -                return 1;
>> > +                goto suspend_dm;
>> >              }
>> >          }
>> >
>> > @@ -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.

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?

shriram

[-- Attachment #1.2: Type: text/html, Size: 4191 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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