| [Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
 Re: [Xen-devel] [PATCH 3 of 6] libxl: QMP stop/resume & refactor	QEMU suspend/resume/save
 
 What do you think? goto needs to go?On Tue, Jan 31, 2012 at 9:32 AM, Shriram Rajagopalan <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. There seems to be no reference to such
 a state anywhere.
 
 
 
The goto's make the code flow a bit tricky to follow (this function is
> +        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:
 
 
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)
 
 
 shriram
 
 _______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
 
 |