Re: [Xen-devel] [PATCH v6 04/18] libxl/save: Refactor libxl__domain_suspend_state

2016-01-26 Thread Konrad Rzeszutek Wilk 
On Tue, Jan 26, 2016 at 10:23:52AM +0800, Wen Congyang wrote:
> On 01/26/2016 01:29 AM, Konrad Rzeszutek Wilk wrote:
> > .snip..
> >> --- a/tools/libxl/libxl_dom_suspend.c
> >> +++ b/tools/libxl/libxl_dom_suspend.c
> >> @@ -19,14 +19,71 @@
> >>  
> >>  /*== Domain suspend ===*/
> >>  
> >> +int libxl__domain_suspend_init(libxl__egc *egc,
> >> +   libxl__domain_suspend_state *dsps)
> >> +{
> >> +STATE_AO_GC(dsps->ao);
> >> +int rc = ERROR_FAIL;
> >> +int port;
> >> +libxl_domain_type type;
> >> +
> >> +/* Convenience aliases */
> >> +const uint32_t domid = dsps->domid;
> >> +
> >> +type = libxl__domain_type(gc, domid);
> >> +switch (type) {
> >> +case LIBXL_DOMAIN_TYPE_HVM: {
> >> +dsps->hvm = 1;
> >> +break;
> >> +}
> >> +case LIBXL_DOMAIN_TYPE_PV:
> >> +dsps->hvm = 0;
> >> +break;
> >> +default:
> >> +goto out;
> > 
> > This will mean we return back to libxl__domain_save which will goto out 
> > which calls:
> > domain_save_done. And that will try to use the dsps->guestevtchn leading to 
> > a crash since:
> 
> Yes, thanks for pointing it out. In which case, the type is not HVM or PV?

If you call those init routines before the switch statemet - such as the
libxl__xswait_init, etc, then you can still goto out
> 
> >> +}
> >> +
> >> +libxl__xswait_init(>pvcontrol);
> >> +libxl__ev_evtchn_init(>guest_evtchn);
> > 
> > we initialize them here.
> >> +libxl__ev_xswatch_init(>guest_watch);
> >> +libxl__ev_time_init(>guest_timeout);
> > 
> > I would instead recommend you move these initialization routines above the
> > 'type' check.
> 
> I think we should not return ERROR_FAIL when the type is not PV or HVM. We 
> should abort the program
> like what we do in libxl__domain_save().

I would rather return - this is a library after all - so the controlling 
program should
do such drastic measures - not an library.

> 
> > 
> >> +
> >> +dsps->guest_evtchn.port = -1;
> >> +dsps->guest_evtchn_lockfd = -1;
> >> +dsps->guest_responded = 0;
> >> +dsps->dm_savefile = libxl__device_model_savefile(gc, domid);
> >> +
> >> +port = xs_suspend_evtchn_port(domid);
> >> +
> >> +if (port >= 0) {
> >> +rc = libxl__ctx_evtchn_init(gc);
> >> +if (rc) goto out;
> >> +
> >> +dsps->guest_evtchn.port =
> >> +xc_suspend_evtchn_init_exclusive(CTX->xch, CTX->xce,
> >> +domid, port, 
> >> >guest_evtchn_lockfd);
> >> +
> >> +if (dsps->guest_evtchn.port < 0) {
> >> +LOG(WARN, "Suspend event channel initialization failed");
> >> +rc = ERROR_FAIL;
> >> +goto out;
> >> +}
> >> +}
> >> +
> >> +rc = 0;
> >> +
> >> +out:
> >> +return rc;
> >> +}
> >> +
> > 
> > .. snip..
> >>  struct libxl__domain_suspend_state {
> >> +/* set by caller of libxl__domain_suspend_init */
> >> +libxl__ao *ao;
> >> +uint32_t domid;
> >> +
> >> +/* private */
> >> +int hvm;
> > 
> > How about 'is_hvm' and just use 'libxl_domain_type' type?
> > instead of having an int? You can just do:
> 
> In dss, it is 'int hvm'.
> Before this patch:
> if (dss->hvm) ...
> After this patch:
> if (dsps->hvm) ...

Right..
> 
> Thanks
> Wen Congyang
> 
> > 
> > if (type == LIBXL_DOMAIN_TYPE_HVM) ..

But what if you use that? As in dsps->type == LIBXL_DOMAIAN_TYPE_HVM for 
example?

> > 
> > And to check for non-conforming types - you can make  
> > libxl__domain_suspend_init
> > do this:
> > 
> > if (type == LIBXL_DOMAIN_TYPE_INVALID) {
> > rc = ERROR_FAIL;
> > goto out; 
> > }
> > 
> > ?
> > 
> > 
> > .
> > 
> 
> 
> 

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

Re: [Xen-devel] [PATCH v6 04/18] libxl/save: Refactor libxl__domain_suspend_state

2016-01-25 Thread Konrad Rzeszutek Wilk 
.snip..
> --- a/tools/libxl/libxl_dom_suspend.c
> +++ b/tools/libxl/libxl_dom_suspend.c
> @@ -19,14 +19,71 @@
>  
>  /*== Domain suspend ===*/
>  
> +int libxl__domain_suspend_init(libxl__egc *egc,
> +   libxl__domain_suspend_state *dsps)
> +{
> +STATE_AO_GC(dsps->ao);
> +int rc = ERROR_FAIL;
> +int port;
> +libxl_domain_type type;
> +
> +/* Convenience aliases */
> +const uint32_t domid = dsps->domid;
> +
> +type = libxl__domain_type(gc, domid);
> +switch (type) {
> +case LIBXL_DOMAIN_TYPE_HVM: {
> +dsps->hvm = 1;
> +break;
> +}
> +case LIBXL_DOMAIN_TYPE_PV:
> +dsps->hvm = 0;
> +break;
> +default:
> +goto out;

This will mean we return back to libxl__domain_save which will goto out which 
calls:
domain_save_done. And that will try to use the dsps->guestevtchn leading to a 
crash since:
> +}
> +
> +libxl__xswait_init(>pvcontrol);
> +libxl__ev_evtchn_init(>guest_evtchn);

we initialize them here.
> +libxl__ev_xswatch_init(>guest_watch);
> +libxl__ev_time_init(>guest_timeout);

I would instead recommend you move these initialization routines above the
'type' check.

> +
> +dsps->guest_evtchn.port = -1;
> +dsps->guest_evtchn_lockfd = -1;
> +dsps->guest_responded = 0;
> +dsps->dm_savefile = libxl__device_model_savefile(gc, domid);
> +
> +port = xs_suspend_evtchn_port(domid);
> +
> +if (port >= 0) {
> +rc = libxl__ctx_evtchn_init(gc);
> +if (rc) goto out;
> +
> +dsps->guest_evtchn.port =
> +xc_suspend_evtchn_init_exclusive(CTX->xch, CTX->xce,
> +domid, port, >guest_evtchn_lockfd);
> +
> +if (dsps->guest_evtchn.port < 0) {
> +LOG(WARN, "Suspend event channel initialization failed");
> +rc = ERROR_FAIL;
> +goto out;
> +}
> +}
> +
> +rc = 0;
> +
> +out:
> +return rc;
> +}
> +

.. snip..
>  struct libxl__domain_suspend_state {
> +/* set by caller of libxl__domain_suspend_init */
> +libxl__ao *ao;
> +uint32_t domid;
> +
> +/* private */
> +int hvm;

How about 'is_hvm' and just use 'libxl_domain_type' type?
instead of having an int? You can just do:

if (type == LIBXL_DOMAIN_TYPE_HVM) ..

And to check for non-conforming types - you can make  libxl__domain_suspend_init
do this:

if (type == LIBXL_DOMAIN_TYPE_INVALID) {
rc = ERROR_FAIL;
goto out; 
}

?

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

Re: [Xen-devel] [PATCH v6 04/18] libxl/save: Refactor libxl__domain_suspend_state

2016-01-25 Thread Wen Congyang 
On 01/26/2016 01:29 AM, Konrad Rzeszutek Wilk wrote:
> .snip..
>> --- a/tools/libxl/libxl_dom_suspend.c
>> +++ b/tools/libxl/libxl_dom_suspend.c
>> @@ -19,14 +19,71 @@
>>  
>>  /*== Domain suspend ===*/
>>  
>> +int libxl__domain_suspend_init(libxl__egc *egc,
>> +   libxl__domain_suspend_state *dsps)
>> +{
>> +STATE_AO_GC(dsps->ao);
>> +int rc = ERROR_FAIL;
>> +int port;
>> +libxl_domain_type type;
>> +
>> +/* Convenience aliases */
>> +const uint32_t domid = dsps->domid;
>> +
>> +type = libxl__domain_type(gc, domid);
>> +switch (type) {
>> +case LIBXL_DOMAIN_TYPE_HVM: {
>> +dsps->hvm = 1;
>> +break;
>> +}
>> +case LIBXL_DOMAIN_TYPE_PV:
>> +dsps->hvm = 0;
>> +break;
>> +default:
>> +goto out;
> 
> This will mean we return back to libxl__domain_save which will goto out which 
> calls:
> domain_save_done. And that will try to use the dsps->guestevtchn leading to a 
> crash since:

Yes, thanks for pointing it out. In which case, the type is not HVM or PV?

>> +}
>> +
>> +libxl__xswait_init(>pvcontrol);
>> +libxl__ev_evtchn_init(>guest_evtchn);
> 
> we initialize them here.
>> +libxl__ev_xswatch_init(>guest_watch);
>> +libxl__ev_time_init(>guest_timeout);
> 
> I would instead recommend you move these initialization routines above the
> 'type' check.

I think we should not return ERROR_FAIL when the type is not PV or HVM. We 
should abort the program
like what we do in libxl__domain_save().

> 
>> +
>> +dsps->guest_evtchn.port = -1;
>> +dsps->guest_evtchn_lockfd = -1;
>> +dsps->guest_responded = 0;
>> +dsps->dm_savefile = libxl__device_model_savefile(gc, domid);
>> +
>> +port = xs_suspend_evtchn_port(domid);
>> +
>> +if (port >= 0) {
>> +rc = libxl__ctx_evtchn_init(gc);
>> +if (rc) goto out;
>> +
>> +dsps->guest_evtchn.port =
>> +xc_suspend_evtchn_init_exclusive(CTX->xch, CTX->xce,
>> +domid, port, 
>> >guest_evtchn_lockfd);
>> +
>> +if (dsps->guest_evtchn.port < 0) {
>> +LOG(WARN, "Suspend event channel initialization failed");
>> +rc = ERROR_FAIL;
>> +goto out;
>> +}
>> +}
>> +
>> +rc = 0;
>> +
>> +out:
>> +return rc;
>> +}
>> +
> 
> .. snip..
>>  struct libxl__domain_suspend_state {
>> +/* set by caller of libxl__domain_suspend_init */
>> +libxl__ao *ao;
>> +uint32_t domid;
>> +
>> +/* private */
>> +int hvm;
> 
> How about 'is_hvm' and just use 'libxl_domain_type' type?
> instead of having an int? You can just do:

In dss, it is 'int hvm'.
Before this patch:
if (dss->hvm) ...
After this patch:
if (dsps->hvm) ...

Thanks
Wen Congyang

> 
> if (type == LIBXL_DOMAIN_TYPE_HVM) ..
> 
> And to check for non-conforming types - you can make  
> libxl__domain_suspend_init
> do this:
> 
> if (type == LIBXL_DOMAIN_TYPE_INVALID) {
> rc = ERROR_FAIL;
> goto out; 
> }
> 
> ?
> 
> 
> .
> 




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

[Xen-devel] [PATCH v6 04/18] libxl/save: Refactor libxl__domain_suspend_state

2015-12-29 Thread Wen Congyang 
Currently struct libxl__domain_suspend_state contains 2 type of states,
one is save state, another is suspend state. This patch separates those
two out.
The motivation of this is that COLO will need to do suspend/resume
continuously, we need a more common suspend state.

After this change, dss stands for libxl__domain_save_state,
dsps stands for libxl__domain_suspend_state.

Also introduce libxl__domain_suspend_init to initialise the
libxl__domain_suspend_state.

Signed-off-by: Yang Hongyang 
Signed-off-by: Wen Congyang 
CC: Ian Campbell 
CC: Ian Jackson 
CC: Wei Liu 
CC: Andrew Cooper 
Acked-by:Ian Campbell 
---
 tools/libxl/libxl.c  |  10 +-
 tools/libxl/libxl_create.c   |  10 +-
 tools/libxl/libxl_dom_save.c |  61 ---
 tools/libxl/libxl_dom_suspend.c  | 217 +--
 tools/libxl/libxl_internal.h |  60 +++
 tools/libxl/libxl_netbuffer.c|   2 +-
 tools/libxl/libxl_remus.c|  37 ---
 tools/libxl/libxl_save_callout.c |   2 +-
 tools/libxl/libxl_stream_write.c |  16 +--
 9 files changed, 236 insertions(+), 179 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index bdd8ad0..f8f7158 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -833,7 +833,7 @@ out:
 }
 
 static void remus_failover_cb(libxl__egc *egc,
-  libxl__domain_suspend_state *dss, int rc);
+  libxl__domain_save_state *dss, int rc);
 
 /* TODO: Explicit Checkpoint acknowledgements via recv_fd. */
 int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info,
@@ -841,7 +841,7 @@ int libxl_domain_remus_start(libxl_ctx *ctx, 
libxl_domain_remus_info *info,
  const libxl_asyncop_how *ao_how)
 {
 AO_CREATE(ctx, domid, ao_how);
-libxl__domain_suspend_state *dss;
+libxl__domain_save_state *dss;
 int rc;
 
 libxl_domain_type type = libxl__domain_type(gc, domid);
@@ -889,7 +889,7 @@ int libxl_domain_remus_start(libxl_ctx *ctx, 
libxl_domain_remus_info *info,
 }
 
 static void remus_failover_cb(libxl__egc *egc,
-  libxl__domain_suspend_state *dss, int rc)
+  libxl__domain_save_state *dss, int rc)
 {
 STATE_AO_GC(dss->ao);
 /*
@@ -901,7 +901,7 @@ static void remus_failover_cb(libxl__egc *egc,
 }
 
 static void domain_suspend_cb(libxl__egc *egc,
-  libxl__domain_suspend_state *dss, int rc)
+  libxl__domain_save_state *dss, int rc)
 {
 STATE_AO_GC(dss->ao);
 int flrc;
@@ -926,7 +926,7 @@ int libxl_domain_suspend(libxl_ctx *ctx, uint32_t domid, 
int fd, int flags,
 goto out_err;
 }
 
-libxl__domain_suspend_state *dss;
+libxl__domain_save_state *dss;
 GCNEW(dss);
 
 dss->ao = ao;
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 0ee9a57..bfa0552 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1540,7 +1540,7 @@ typedef struct {
 typedef struct {
 libxl__app_domain_create_state cdcs;
 libxl__domain_destroy_state dds;
-libxl__domain_suspend_state dss;
+libxl__domain_save_state dss;
 char *toolstack_buf;
 uint32_t toolstack_len;
 } libxl__domain_soft_reset_state;
@@ -1635,7 +1635,7 @@ static int do_domain_soft_reset(libxl_ctx *ctx,
 libxl__app_domain_create_state *cdcs;
 libxl__domain_create_state *dcs;
 libxl__domain_build_state *state;
-libxl__domain_suspend_state *dss;
+libxl__domain_save_state *dss;
 char *dom_path, *xs_store_mfn, *xs_console_mfn;
 uint32_t domid_out;
 int rc;
@@ -1679,8 +1679,8 @@ static int do_domain_soft_reset(libxl_ctx *ctx,
 
 dss->ao = ao;
 dss->domid = domid_soft_reset;
-dss->dm_savefile = GCSPRINTF(LIBXL_DEVICE_MODEL_SAVE_FILE".%d",
- domid_soft_reset);
+dss->dsps.dm_savefile = GCSPRINTF(LIBXL_DEVICE_MODEL_SAVE_FILE".%d",
+  domid_soft_reset);
 
 rc = libxl__save_emulator_xenstore_data(dss, >toolstack_buf,
 >toolstack_len);
@@ -1689,7 +1689,7 @@ static int do_domain_soft_reset(libxl_ctx *ctx,
 goto out;
 }
 
-rc = libxl__domain_suspend_device_model(gc, dss);
+rc = libxl__domain_suspend_device_model(gc, >dsps);
 if (rc) {
 LOG(ERROR, "failed to suspend device model.");
 goto out;
diff --git a/tools/libxl/libxl_dom_save.c b/tools/libxl/libxl_dom_save.c
index 27fd58b..cc8cabe 100644
--- a/tools/libxl/libxl_dom_save.c
+++ b/tools/libxl/libxl_dom_save.c
@@ -24,7 +24,7 @@
 static void stream_done(libxl__egc *egc,
 libxl__stream_write_state *sws, int rc);
 static void