[PATCH] various updates/fixes. (v2)

[PATCH] various updates/fixes. (v2)

[Konrad Rzeszutek Wilk]

Hey!

These are various fixes/updates I had in my tree. Some of them came
about the review of my patches - other through me reviewing the
COLO patches.

Please take a look - if you are please Ack and I will gladly commit them in.

 tools/libxc/xc_resume.c          | 17 +++++++++++++----
 tools/libxl/libxl.c              | 34 ++++++++++++++++++----------------
 tools/libxl/libxl_save_callout.c | 11 ++++++++++-
 3 files changed, 41 insertions(+), 21 deletions(-)

Konrad Rzeszutek Wilk (4):
      libxl: Use libxl_strdup instead of strdup on libxl_version_info
      libxc/xc_domain_resume: Update comment.
      libxl/remus: Change the assert for info to an return
      tools/libxl: run_helper - add #define for arguments.

[PATCH 1/4] libxl: Use libxl_strdup instead of strdup on libxl_version_info

[Konrad Rzeszutek Wilk]

The change is simple replace of raw strdup with a libxl variant.
The benefit of that is the libxl variant has the extra
behaviour of abort-on-alloc-fail - and will improve error handling.

libxl_version_info is a bit odd - it is a public function and as libxl.h
mentions - the callers of libxl_ public function needs to call the appropiate
_dispose() function.

"However libxl_get_version_info() is special and returns a cached
result from the ctx which cannot and should not be freed (as evidenced
by it returning a const struct). This data is freed in libxl_ctx_free()
by calling libxl_version_info_dispose(). This is why none of the callers
remember to free -- they shouldn't be doing so." (Ian Campbell)

So the patch makes sure to use the NOGC.

Suggested-by: Wei Liu <wei.liu2@citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxl/libxl.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2bde0f5..548e2e2 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5258,6 +5258,7 @@ libxl_numainfo *libxl_get_numainfo(libxl_ctx *ctx, int *nr)
 
 const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
 {
+    GC_INIT(ctx);
     union {
         xen_extraversion_t xen_extra;
         xen_compile_info_t xen_cc;
@@ -5270,26 +5271,26 @@ const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
     libxl_version_info *info = &ctx->version_info;
 
     if (info->xen_version_extra != NULL)
-        return info;
+        goto out;
 
     xen_version = xc_version(ctx->xch, XENVER_version, NULL);
     info->xen_version_major = xen_version >> 16;
     info->xen_version_minor = xen_version & 0xFF;
 
     xc_version(ctx->xch, XENVER_extraversion, &u.xen_extra);
-    info->xen_version_extra = strdup(u.xen_extra);
+    info->xen_version_extra = libxl__strdup(NOGC, u.xen_extra);
 
     xc_version(ctx->xch, XENVER_compile_info, &u.xen_cc);
-    info->compiler = strdup(u.xen_cc.compiler);
-    info->compile_by = strdup(u.xen_cc.compile_by);
-    info->compile_domain = strdup(u.xen_cc.compile_domain);
-    info->compile_date = strdup(u.xen_cc.compile_date);
+    info->compiler = libxl__strdup(NOGC, u.xen_cc.compiler);
+    info->compile_by = libxl__strdup(NOGC, u.xen_cc.compile_by);
+    info->compile_domain = libxl__strdup(NOGC, u.xen_cc.compile_domain);
+    info->compile_date = libxl__strdup(NOGC, u.xen_cc.compile_date);
 
     xc_version(ctx->xch, XENVER_capabilities, &u.xen_caps);
-    info->capabilities = strdup(u.xen_caps);
+    info->capabilities = libxl__strdup(NOGC, u.xen_caps);
 
     xc_version(ctx->xch, XENVER_changeset, &u.xen_chgset);
-    info->changeset = strdup(u.xen_chgset);
+    info->changeset = libxl__strdup(NOGC, u.xen_chgset);
 
     xc_version(ctx->xch, XENVER_platform_parameters, &u.p_parms);
     info->virt_start = u.p_parms.virt_start;
@@ -5297,8 +5298,10 @@ const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
     info->pagesize = xc_version(ctx->xch, XENVER_pagesize, NULL);
 
     xc_version(ctx->xch, XENVER_commandline, &u.xen_commandline);
-    info->commandline = strdup(u.xen_commandline);
+    info->commandline = libxl__strdup(NOGC, u.xen_commandline);
 
+ out:
+    GC_FREE;
     return info;
 }
 
-- 
2.1.0

Re: [PATCH 1/4] libxl: Use libxl_strdup instead of strdup on libxl_version_info

[Wei Liu]

On Tue, Jan 26, 2016 at 04:30:57PM -0500, Konrad Rzeszutek Wilk wrote:
> The change is simple replace of raw strdup with a libxl variant.
> The benefit of that is the libxl variant has the extra
> behaviour of abort-on-alloc-fail - and will improve error handling.
> 
> libxl_version_info is a bit odd - it is a public function and as libxl.h
> mentions - the callers of libxl_ public function needs to call the appropiate
> _dispose() function.
> 
> "However libxl_get_version_info() is special and returns a cached
> result from the ctx which cannot and should not be freed (as evidenced
> by it returning a const struct). This data is freed in libxl_ctx_free()
> by calling libxl_version_info_dispose(). This is why none of the callers
> remember to free -- they shouldn't be doing so." (Ian Campbell)
> 
> So the patch makes sure to use the NOGC.
> 
> Suggested-by: Wei Liu <wei.liu2@citrix.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

Re: [PATCH 1/4] libxl: Use libxl_strdup instead of strdup on libxl_version_info

[Ian Campbell]

On Mon, 2016-02-01 at 12:13 +0000, Wei Liu wrote:
> On Tue, Jan 26, 2016 at 04:30:57PM -0500, Konrad Rzeszutek Wilk wrote:
> > The change is simple replace of raw strdup with a libxl variant.
> > The benefit of that is the libxl variant has the extra
> > behaviour of abort-on-alloc-fail - and will improve error handling.
> > 
> > libxl_version_info is a bit odd - it is a public function and as
> > libxl.h
> > mentions - the callers of libxl_ public function needs to call the
> > appropiate
> > _dispose() function.
> > 
> > "However libxl_get_version_info() is special and returns a cached
> > result from the ctx which cannot and should not be freed (as evidenced
> > by it returning a const struct). This data is freed in libxl_ctx_free()
> > by calling libxl_version_info_dispose(). This is why none of the
> > callers
> > remember to free -- they shouldn't be doing so." (Ian Campbell)
> > 
> > So the patch makes sure to use the NOGC.
> > 
> > Suggested-by: Wei Liu <wei.liu2@citrix.com>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>

Applied.

[PATCH 2/4] libxc/xc_domain_resume: Update comment.

[Konrad Rzeszutek Wilk]

To hopefully clarify what it meant. Also point out that mechanism
by which the return 1 value is done is via an intimate knowledge of the
hypercall ABI (i.e. which register - eax - is the return value).

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxc/xc_resume.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/tools/libxc/xc_resume.c b/tools/libxc/xc_resume.c
index 87d4324..e692b81 100644
--- a/tools/libxc/xc_resume.c
+++ b/tools/libxc/xc_resume.c
@@ -248,11 +248,20 @@ out:
 /*
  * Resume execution of a domain after suspend shutdown.
  * This can happen in one of two ways:
- *  1. Resume with special return code.
- *  2. Reset guest environment so it believes it is resumed in a new
- *     domain context.
+ *  1. (fast=1) Resume the guest without resetting the domain environment.
+ *     The guests's call to SCHEDOP_shutdown(SHUTDOWN_suspend) will return 1.
+ *
+ *  2. (fast=0) Reset guest environment so it believes it is resumed in a new
+ *     domain context. The guests's call to SCHEDOP_shutdown(SHUTDOWN_suspend)
+ *     will return 0.
+ *
+ * (1) should only by used for guests which can handle the special return
+ * code. Also note that the insertion of the return code is quite interesting
+ * and that the guest MUST be paused - otherwise we would be corrupting
+ * the guest vCPU state.
+ *
  * (2) should be used only for guests which cannot handle the special
- * new return code. (1) is always safe (but slower).
+ * new return code - and it is always safe (but slower).
  */
 int xc_domain_resume(xc_interface *xch, uint32_t domid, int fast)
 {
-- 
2.1.0

Re: [PATCH 2/4] libxc/xc_domain_resume: Update comment.

[Wei Liu]

On Tue, Jan 26, 2016 at 04:30:58PM -0500, Konrad Rzeszutek Wilk wrote:
> To hopefully clarify what it meant. Also point out that mechanism
> by which the return 1 value is done is via an intimate knowledge of the
> hypercall ABI (i.e. which register - eax - is the return value).
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

As far as I can tell all concerns in previous versions have been
addressed.

Acked-by: Wei Liu <wei.liu2@citrix.com>

Re: [PATCH 2/4] libxc/xc_domain_resume: Update comment.

[Ian Campbell]

On Mon, 2016-02-01 at 12:14 +0000, Wei Liu wrote:
> On Tue, Jan 26, 2016 at 04:30:58PM -0500, Konrad Rzeszutek Wilk wrote:
> > To hopefully clarify what it meant. Also point out that mechanism
> > by which the return 1 value is done is via an intimate knowledge of the
> > hypercall ABI (i.e. which register - eax - is the return value).
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> As far as I can tell all concerns in previous versions have been
> addressed.
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>

Applied.

[PATCH 3/4] libxl/remus: Change the assert for info to an return

[Konrad Rzeszutek Wilk]

The assert(info) is after quite a lot of manipulations
on 'info' - which makes the assert pointless because if
info was NULL it would have crashed earlier.

Remove it and make it an return. Also since most of the
error paths are for the same rc, unify them.

CC: Wen Congyang <wency@cn.fujitsu.com>
CC: Yang Hongyang <hongyang.yang@easystack.cn>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxl/libxl.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 548e2e2..228b241 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -847,13 +847,14 @@ int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info,
 {
     AO_CREATE(ctx, domid, ao_how);
     libxl__domain_suspend_state *dss;
-    int rc;
+    int rc = ERROR_FAIL;
 
     libxl_domain_type type = libxl__domain_type(gc, domid);
-    if (type == LIBXL_DOMAIN_TYPE_INVALID) {
-        rc = ERROR_FAIL;
+    if (type == LIBXL_DOMAIN_TYPE_INVALID)
+        goto out;
+
+    if (!info)
         goto out;
-    }
 
     libxl_defbool_setdefault(&info->allow_unsafe, false);
     libxl_defbool_setdefault(&info->blackhole, false);
@@ -867,7 +868,6 @@ int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info,
          !libxl_defbool_val(info->diskbuf))) {
         LOG(ERROR, "Unsafe mode must be enabled to replicate to /dev/null,"
                    "disable network buffering and disk replication");
-        rc = ERROR_FAIL;
         goto out;
     }
 
@@ -883,8 +883,7 @@ int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info,
     dss->debug = 0;
     dss->remus = info;
 
-    assert(info);
-
+    rc = 0;
     /* Point of no return */
     libxl__remus_setup(egc, dss);
     return AO_INPROGRESS;
-- 
2.1.0

Re: [PATCH 3/4] libxl/remus: Change the assert for info to an return

[Wei Liu]

On Tue, Jan 26, 2016 at 04:30:59PM -0500, Konrad Rzeszutek Wilk wrote:
> The assert(info) is after quite a lot of manipulations
> on 'info' - which makes the assert pointless because if
> info was NULL it would have crashed earlier.
> 

This sounds sensible.

> Remove it and make it an return. Also since most of the
> error paths are for the same rc, unify them.
> 
> CC: Wen Congyang <wency@cn.fujitsu.com>
> CC: Yang Hongyang <hongyang.yang@easystack.cn>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  tools/libxl/libxl.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 548e2e2..228b241 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -847,13 +847,14 @@ int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info,
>  {
>      AO_CREATE(ctx, domid, ao_how);
>      libxl__domain_suspend_state *dss;
> -    int rc;
> +    int rc = ERROR_FAIL;

This violates coding style:

 83   * If the function is to return a libxl error value, `rc' is
 84     used to contain the error code, but it is NOT initialised:
 85             int rc;

>  
>      libxl_domain_type type = libxl__domain_type(gc, domid);
> -    if (type == LIBXL_DOMAIN_TYPE_INVALID) {
> -        rc = ERROR_FAIL;
> +    if (type == LIBXL_DOMAIN_TYPE_INVALID)
> +        goto out;
> +
> +    if (!info)
>          goto out;
> -    }
>  
>      libxl_defbool_setdefault(&info->allow_unsafe, false);
>      libxl_defbool_setdefault(&info->blackhole, false);
> @@ -867,7 +868,6 @@ int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info,
>           !libxl_defbool_val(info->diskbuf))) {
>          LOG(ERROR, "Unsafe mode must be enabled to replicate to /dev/null,"
>                     "disable network buffering and disk replication");
> -        rc = ERROR_FAIL;
>          goto out;
>      }
>  
> @@ -883,8 +883,7 @@ int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info,
>      dss->debug = 0;
>      dss->remus = info;
>  
> -    assert(info);
> -
> +    rc = 0;
>      /* Point of no return */
>      libxl__remus_setup(egc, dss);
>      return AO_INPROGRESS;
> -- 
> 2.1.0
> 

[PATCH] toolstack documentation/asserts cleanups. (v1)

[Konrad Rzeszutek Wilk]

Hey!

As I was reviewing the v6 of the remus/COLO patches I ran into
some parts of code that was not exactly clear to me.

These patches update the comments or move the code a bit to make
more sense. Please review at your convience.


 tools/libxc/xc_resume.c          |  7 +++++--
 tools/libxl/libxl.c              |  4 ++--
 tools/libxl/libxl_save_callout.c | 11 ++++++++++-
 3 files changed, 17 insertions(+), 5 deletions(-)


Konrad Rzeszutek Wilk (3):
      libxc/xc_domain_resume: Update comment.
      libxl/remus: Move the assert before the info is used.
      tools/libxl: run_helper - add #define for arguments.

[PATCH 1/3] libxc/xc_domain_resume: Update comment.

[Konrad Rzeszutek Wilk]

To hopefully clarify what it meant.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxc/xc_resume.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/libxc/xc_resume.c b/tools/libxc/xc_resume.c
index 87d4324..19ba2a3 100644
--- a/tools/libxc/xc_resume.c
+++ b/tools/libxc/xc_resume.c
@@ -248,9 +248,12 @@ out:
 /*
  * Resume execution of a domain after suspend shutdown.
  * This can happen in one of two ways:
- *  1. Resume with special return code.
- *  2. Reset guest environment so it believes it is resumed in a new
+ *  1. (fast=1) Resume with special return code (1) that the guest
+ *     gets from SCHEDOP_shutdown:SHUTDOWN_suspend.
+ *
+ *  2. (fast=0) Reset guest environment so it believes it is resumed in a new
  *     domain context.
+ *
  * (2) should be used only for guests which cannot handle the special
  * new return code. (1) is always safe (but slower).
  */
-- 
2.4.3

Re: [PATCH 1/3] libxc/xc_domain_resume: Update comment.

[Ian Campbell]

On Mon, 2016-01-25 at 16:06 -0500, Konrad Rzeszutek Wilk wrote:
> To hopefully clarify what it meant.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  tools/libxc/xc_resume.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxc/xc_resume.c b/tools/libxc/xc_resume.c
> index 87d4324..19ba2a3 100644
> --- a/tools/libxc/xc_resume.c
> +++ b/tools/libxc/xc_resume.c
> @@ -248,9 +248,12 @@ out:
>  /*
>   * Resume execution of a domain after suspend shutdown.
>   * This can happen in one of two ways:
> - *  1. Resume with special return code.
> - *  2. Reset guest environment so it believes it is resumed in a new
> + *  1. (fast=1) Resume with special return code (1) that the guest
> + *     gets from SCHEDOP_shutdown:SHUTDOWN_suspend.

"SCHEDOP_shutdown(SHUTDOWN_suspend)" looks more like the function call
which this in effect is.

I think I'd say "Resume the guest without resetting the domain environment.
The guests's call to SCHEDOP_shutdown(SHUTDOWN_suspend) will return 1".

(assuming that is true re resetting)

> + *
> + *  2. (fast=0) Reset guest environment so it believes it is resumed in a new
>   *     domain context.

with the above I would suggesting adding "The guests's call to
SCHEDOP_shutdown(SHUTDOWN_suspend) will return 0".

> + *
>   * (2) should be used only for guests which cannot handle the special
>   * new return code. (1) is always safe (but slower).

Is this correct? I'd have said (2) was always safe but slow?

And I would invert the first, that is to say that (1) should be used in
preference with guests which support it.

Ian.

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

Re: [PATCH 1/3] libxc/xc_domain_resume: Update comment.

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 1/3] libxc/xc_domain_resume: Update comment."):
> On Mon, 2016-01-25 at 16:06 -0500, Konrad Rzeszutek Wilk wrote:
> > To hopefully clarify what it meant.
...
> > + *  1. (fast=1) Resume with special return code (1) that the guest
> > + *     gets from SCHEDOP_shutdown:SHUTDOWN_suspend.
> 
> "SCHEDOP_shutdown(SHUTDOWN_suspend)" looks more like the function call
> which this in effect is.
> 
> I think I'd say "Resume the guest without resetting the domain environment.
> The guests's call to SCHEDOP_shutdown(SHUTDOWN_suspend) will return 1".
> 
> (assuming that is true re resetting)

I'm not sure that `will return 1' is correct.  IIRC there is some
... unpleasantness here, with something effectively corrupting the
guest state in a way that the guest is supposed to expect and
cooperate with.

I haven't investigated the details recently.  I do remember it being
fiddly.

Ian.

Re: [PATCH 1/3] libxc/xc_domain_resume: Update comment.

[Ian Campbell]

On Tue, 2016-01-26 at 16:22 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 1/3] libxc/xc_domain_resume: Update
> comment."):
> > On Mon, 2016-01-25 at 16:06 -0500, Konrad Rzeszutek Wilk wrote:
> > > To hopefully clarify what it meant.
> ...
> > > + *  1. (fast=1) Resume with special return code (1) that the guest
> > > + *     gets from SCHEDOP_shutdown:SHUTDOWN_suspend.
> > 
> > "SCHEDOP_shutdown(SHUTDOWN_suspend)" looks more like the function call
> > which this in effect is.
> > 
> > I think I'd say "Resume the guest without resetting the domain
> > environment.
> > The guests's call to SCHEDOP_shutdown(SHUTDOWN_suspend) will return 1".
> > 
> > (assuming that is true re resetting)
> 
> I'm not sure that `will return 1' is correct.  IIRC there is some
> ... unpleasantness here, with something effectively corrupting the
> guest state in a way that the guest is supposed to expect and
> cooperate with.

The tools arrange for the hypercall to return 1, which the guest is indeed
expected to expect and cooperate, as with any PV interface call it makes.

They do this by intimate knowledge of the hypercall ABI (i.e. which
register is the return value) and one could certainly argue it ought to be
arranged in a less horrific way, but I think to characterise it as
"corrupting" is probably going to far.


> 
> I haven't investigated the details recently.  I do remember it being
> fiddly.
> 
> Ian.

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

Re: [PATCH 1/3] libxc/xc_domain_resume: Update comment.

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 1/3] libxc/xc_domain_resume: Update comment."):
> On Tue, 2016-01-26 at 16:22 +0000, Ian Jackson wrote:
> > I'm not sure that `will return 1' is correct.  IIRC there is some
> > ... unpleasantness here, with something effectively corrupting the
> > guest state in a way that the guest is supposed to expect and
> > cooperate with.
> 
> The tools arrange for the hypercall to return 1, which the guest is indeed
> expected to expect and cooperate, as with any PV interface call it makes.
> 
> They do this by intimate knowledge of the hypercall ABI (i.e. which
> register is the return value) and one could certainly argue it ought to be
> arranged in a less horrific way, but I think to characterise it as
> "corrupting" is probably going to far.

Ian C had a conversation about this in person.  We think (ie, I am now
convinced) that provided that this xc resume call is only made when
the guest is suspended, that the worst outcome will indeed be that the
guest experiences the hypercall returning 1, and then finding itself
in a state it's not expecting.  The guest will hopefully crash due
to the unexpected return value but is in any case likely to implode
soon due to event channel misconfiguration etc.

Only if the `resume' is attempted with the guest running, would the
guest's %eax actually be `corrupted' in this sense.

Ian.

Re: [PATCH 1/3] libxc/xc_domain_resume: Update comment.

[Konrad Rzeszutek Wilk]

On Tue, Jan 26, 2016 at 04:52:06PM +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 1/3] libxc/xc_domain_resume: Update comment."):
> > On Tue, 2016-01-26 at 16:22 +0000, Ian Jackson wrote:
> > > I'm not sure that `will return 1' is correct.  IIRC there is some
> > > ... unpleasantness here, with something effectively corrupting the
> > > guest state in a way that the guest is supposed to expect and
> > > cooperate with.
> > 
> > The tools arrange for the hypercall to return 1, which the guest is indeed
> > expected to expect and cooperate, as with any PV interface call it makes.
> > 
> > They do this by intimate knowledge of the hypercall ABI (i.e. which
> > register is the return value) and one could certainly argue it ought to be
> > arranged in a less horrific way, but I think to characterise it as
> > "corrupting" is probably going to far.
> 
> Ian C had a conversation about this in person.  We think (ie, I am now
> convinced) that provided that this xc resume call is only made when
> the guest is suspended, that the worst outcome will indeed be that the
> guest experiences the hypercall returning 1, and then finding itself
> in a state it's not expecting.  The guest will hopefully crash due
> to the unexpected return value but is in any case likely to implode
> soon due to event channel misconfiguration etc.

<nods>
> 
> Only if the `resume' is attempted with the guest running, would the
> guest's %eax actually be `corrupted' in this sense.

Right. That code doing the set_vcpu manipulations is quite .. ahhh
interesting!

I will update the comment to be more clear.
> 
> Ian.

Re: [PATCH 1/3] libxc/xc_domain_resume: Update comment.

[Konrad Rzeszutek Wilk]

On Tue, Jan 26, 2016 at 04:19:54PM +0000, Ian Campbell wrote:
> On Mon, 2016-01-25 at 16:06 -0500, Konrad Rzeszutek Wilk wrote:
> > To hopefully clarify what it meant.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  tools/libxc/xc_resume.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/libxc/xc_resume.c b/tools/libxc/xc_resume.c
> > index 87d4324..19ba2a3 100644
> > --- a/tools/libxc/xc_resume.c
> > +++ b/tools/libxc/xc_resume.c
> > @@ -248,9 +248,12 @@ out:
> >  /*
> >   * Resume execution of a domain after suspend shutdown.
> >   * This can happen in one of two ways:
> > - *  1. Resume with special return code.
> > - *  2. Reset guest environment so it believes it is resumed in a new
> > + *  1. (fast=1) Resume with special return code (1) that the guest
> > + *     gets from SCHEDOP_shutdown:SHUTDOWN_suspend.
> 
> "SCHEDOP_shutdown(SHUTDOWN_suspend)" looks more like the function call
> which this in effect is.
> 
> I think I'd say "Resume the guest without resetting the domain environment.
> The guests's call to SCHEDOP_shutdown(SHUTDOWN_suspend) will return 1".
> 
> (assuming that is true re resetting)
> 
> > + *
> > + *  2. (fast=0) Reset guest environment so it believes it is resumed in a new
> >   *     domain context.
> 
> with the above I would suggesting adding "The guests's call to
> SCHEDOP_shutdown(SHUTDOWN_suspend) will return 0".
> 
> > + *
> >   * (2) should be used only for guests which cannot handle the special
> >   * new return code. (1) is always safe (but slower).
> 
> Is this correct? I'd have said (2) was always safe but slow?

That does not sound right. It should have said that fast=1 
would be fast but not safe. And 2) (fast=0) is safe but slower.

Let me resend this - with it hopefully being more clear.
> 
> And I would invert the first, that is to say that (1) should be used in
> preference with guests which support it.

Reading the 1) I am bit perplexed. It says "safe" but what it does is far
from safe - it manipulates the vCPU eax register to be 1. Granted it does
it on a "paused" vCPU and once the vCPU resume it can read it.

I guess in the olden days this was considered safe. Along with driving
without seatbelts.

> 
> Ian.

Re: [PATCH 1/3] libxc/xc_domain_resume: Update comment.

[Ian Campbell]

On Tue, 2016-01-26 at 14:47 -0500, Konrad Rzeszutek Wilk wrote:
> On Tue, Jan 26, 2016 at 04:19:54PM +0000, Ian Campbell wrote:
> > On Mon, 2016-01-25 at 16:06 -0500, Konrad Rzeszutek Wilk wrote:
> > > To hopefully clarify what it meant.
> > > 
> > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > > ---
> > >  tools/libxc/xc_resume.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tools/libxc/xc_resume.c b/tools/libxc/xc_resume.c
> > > index 87d4324..19ba2a3 100644
> > > --- a/tools/libxc/xc_resume.c
> > > +++ b/tools/libxc/xc_resume.c
> > > @@ -248,9 +248,12 @@ out:
> > >  /*
> > >   * Resume execution of a domain after suspend shutdown.
> > >   * This can happen in one of two ways:
> > > - *  1. Resume with special return code.
> > > - *  2. Reset guest environment so it believes it is resumed in a new
> > > + *  1. (fast=1) Resume with special return code (1) that the guest
> > > + *     gets from SCHEDOP_shutdown:SHUTDOWN_suspend.
> > 
> > "SCHEDOP_shutdown(SHUTDOWN_suspend)" looks more like the function call
> > which this in effect is.
> > 
> > I think I'd say "Resume the guest without resetting the domain
> > environment.
> > The guests's call to SCHEDOP_shutdown(SHUTDOWN_suspend) will return 1".
> > 
> > (assuming that is true re resetting)
> > 
> > > + *
> > > + *  2. (fast=0) Reset guest environment so it believes it is resumed
> > > in a new
> > >   *     domain context.
> > 
> > with the above I would suggesting adding "The guests's call to
> > SCHEDOP_shutdown(SHUTDOWN_suspend) will return 0".
> > 
> > > + *
> > >   * (2) should be used only for guests which cannot handle the
> > > special
> > >   * new return code. (1) is always safe (but slower).
> > 
> > Is this correct? I'd have said (2) was always safe but slow?
> 
> That does not sound right. It should have said that fast=1 
> would be fast but not safe. And 2) (fast=0) is safe but slower.
> 
> Let me resend this - with it hopefully being more clear.
> > 
> > And I would invert the first, that is to say that (1) should be used in
> > preference with guests which support it.
> 
> Reading the 1) I am bit perplexed. It says "safe" but what it does is far
> from safe - it manipulates the vCPU eax register to be 1. Granted it does
> it on a "paused" vCPU and once the vCPU resume it can read it.

I think "(1) is always safe (but slower)" should have always read "(2) is
always safe (but slower)". (1) has always been "fast and loose" and
requires guest side support.

Ian.

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

[PATCH 2/3] libxl/remus: Move the assert before the info is used.

[Konrad Rzeszutek Wilk]

The assert(info) is after quite a lot of manipulations
on 'info' - which makes the assert pointless because if
info was NULL it would have crashed earlier.

Move it earlier so that it guards before we try using
the 'info' structure.

CC: Wen Congyang <wency@cn.fujitsu.com>
CC: Yang Hongyang <hongyang.yang@easystack.cn>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxl/libxl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 2bde0f5..60974cc 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -855,6 +855,8 @@ int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info,
         goto out;
     }
 
+    assert(info);
+
     libxl_defbool_setdefault(&info->allow_unsafe, false);
     libxl_defbool_setdefault(&info->blackhole, false);
     libxl_defbool_setdefault(&info->compression, true);
@@ -883,8 +885,6 @@ int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info,
     dss->debug = 0;
     dss->remus = info;
 
-    assert(info);
-
     /* Point of no return */
     libxl__remus_setup(egc, dss);
     return AO_INPROGRESS;
-- 
2.4.3

Re: [PATCH 2/3] libxl/remus: Move the assert before the info is used.

[Ian Campbell]

On Mon, 2016-01-25 at 16:06 -0500, Konrad Rzeszutek Wilk wrote:
> The assert(info) is after quite a lot of manipulations
> on 'info' - which makes the assert pointless because if
> info was NULL it would have crashed earlier.
> 
> Move it earlier so that it guards before we try using
> the 'info' structure.

That assert (wherever it is placed) is rather aggressive for an application
provided argument. ERROR_INVALID would be more normal I think.

> 
> CC: Wen Congyang <wency@cn.fujitsu.com>
> CC: Yang Hongyang <hongyang.yang@easystack.cn>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  tools/libxl/libxl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 2bde0f5..60974cc 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -855,6 +855,8 @@ int libxl_domain_remus_start(libxl_ctx *ctx,
> libxl_domain_remus_info *info,
>          goto out;
>      }
>  
> +    assert(info);
> +
>      libxl_defbool_setdefault(&info->allow_unsafe, false);
>      libxl_defbool_setdefault(&info->blackhole, false);
>      libxl_defbool_setdefault(&info->compression, true);
> @@ -883,8 +885,6 @@ int libxl_domain_remus_start(libxl_ctx *ctx,
> libxl_domain_remus_info *info,
>      dss->debug = 0;
>      dss->remus = info;
>  
> -    assert(info);
> -
>      /* Point of no return */
>      libxl__remus_setup(egc, dss);
>      return AO_INPROGRESS;

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

Re: [PATCH 2/3] libxl/remus: Move the assert before the info is used.

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 2/3] libxl/remus: Move the assert before the info is used."):
> On Mon, 2016-01-25 at 16:06 -0500, Konrad Rzeszutek Wilk wrote:
> > The assert(info) is after quite a lot of manipulations
> > on 'info' - which makes the assert pointless because if
> > info was NULL it would have crashed earlier.
> > 
> > Move it earlier so that it guards before we try using
> > the 'info' structure.
> 
> That assert (wherever it is placed) is rather aggressive for an application
> provided argument. ERROR_INVALID would be more normal I think.

I think the assert should simply be removed.  We don't assert() other
pointer parameters for non-NULL-ness.

Certainly turning null pointer bugs into ERROR_INVALID is very
unfriendly.

Ian.

Re: [PATCH 2/3] libxl/remus: Move the assert before the info is used. [and 1 more messages]

[Ian Jackson]

Wei Liu writes ("Re: [PATCH 3/4] libxl/remus: Change the assert for info to an return"):
> On Tue, Jan 26, 2016 at 04:30:59PM -0500, Konrad Rzeszutek Wilk wrote:
> > The assert(info) is after quite a lot of manipulations
> > on 'info' - which makes the assert pointless because if
> > info was NULL it would have crashed earlier.
> 
> This sounds sensible.

I don't think I agree.

As I wrote in response to a previous version:

Ian Jackson writes ("Re: [PATCH 2/3] libxl/remus: Move the assert
before the info is used."):
> I think the assert should simply be removed.  We don't assert() other
> pointer parameters for non-NULL-ness.
> 
> Certainly turning null pointer bugs into ERROR_INVALID is very
> unfriendly.

Ian.

[PATCH 3/3] tools/libxl: run_helper - add #define for arguments.

[Konrad Rzeszutek Wilk]

Describe what the four (or more in the future) arguments
are for.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxl/libxl_save_callout.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_save_callout.c b/tools/libxl/libxl_save_callout.c
index 3af99af..45b9727 100644
--- a/tools/libxl/libxl_save_callout.c
+++ b/tools/libxl/libxl_save_callout.c
@@ -119,13 +119,22 @@ void libxl__save_helper_init(libxl__save_helper_state *shs)
 
 /*----- helper execution -----*/
 
+/*
+ * Both save and restore share four parameters:
+ * 1) Path to libxl-save-helper.
+ * 2) --[restore|save]-domain.
+ * 3) stream file descriptor.
+ * n) save/restore specific parameters.
+ * 4) A \0 at the end.
+ */
+#define HELPER_NR_ARGS 4
 static void run_helper(libxl__egc *egc, libxl__save_helper_state *shs,
                        const char *mode_arg, int stream_fd,
                        const int *preserve_fds, int num_preserve_fds,
                        const unsigned long *argnums, int num_argnums)
 {
     STATE_AO_GC(shs->ao);
-    const char *args[4 + num_argnums];
+    const char *args[HELPER_NR_ARGS + num_argnums];
     const char **arg = args;
     int i, rc;
 
-- 
2.4.3

Re: [PATCH 3/3] tools/libxl: run_helper - add #define for arguments.

[Ian Campbell]

On Mon, 2016-01-25 at 16:06 -0500, Konrad Rzeszutek Wilk wrote:
> Describe what the four (or more in the future) arguments
> are for.

I'd say that a code comment on the definition would be sufficient here, but
I'll defer to Ian J as author of this code.

> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  tools/libxl/libxl_save_callout.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl_save_callout.c
> b/tools/libxl/libxl_save_callout.c
> index 3af99af..45b9727 100644
> --- a/tools/libxl/libxl_save_callout.c
> +++ b/tools/libxl/libxl_save_callout.c
> @@ -119,13 +119,22 @@ void
> libxl__save_helper_init(libxl__save_helper_state *shs)
>  
>  /*----- helper execution -----*/
>  
> +/*
> + * Both save and restore share four parameters:
> + * 1) Path to libxl-save-helper.
> + * 2) --[restore|save]-domain.
> + * 3) stream file descriptor.
> + * n) save/restore specific parameters.
> + * 4) A \0 at the end.
> + */
> +#define HELPER_NR_ARGS 4
>  static void run_helper(libxl__egc *egc, libxl__save_helper_state *shs,
>                         const char *mode_arg, int stream_fd,
>                         const int *preserve_fds, int num_preserve_fds,
>                         const unsigned long *argnums, int num_argnums)
>  {
>      STATE_AO_GC(shs->ao);
> -    const char *args[4 + num_argnums];
> +    const char *args[HELPER_NR_ARGS + num_argnums];
>      const char **arg = args;
>      int i, rc;
>  

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

Re: [PATCH 3/3] tools/libxl: run_helper - add #define for arguments.

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH 3/3] tools/libxl: run_helper - add #define for arguments."):
> On Mon, 2016-01-25 at 16:06 -0500, Konrad Rzeszutek Wilk wrote:
> > Describe what the four (or more in the future) arguments
> > are for.
> 
> I'd say that a code comment on the definition would be sufficient here, but
> I'll defer to Ian J as author of this code.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [PATCH 3/3] tools/libxl: run_helper - add #define for arguments.

[Ian Campbell]

On Tue, 2016-01-26 at 16:25 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH 3/3] tools/libxl: run_helper - add
> #define for arguments."):
> > On Mon, 2016-01-25 at 16:06 -0500, Konrad Rzeszutek Wilk wrote:
> > > Describe what the four (or more in the future) arguments
> > > are for.
> > 
> > I'd say that a code comment on the definition would be sufficient here,
> > but
> > I'll defer to Ian J as author of this code.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Applied.

[PATCH 4/4] tools/libxl: run_helper - add #define for arguments.

[Konrad Rzeszutek Wilk]

Describe what the four (or more in the future) arguments
are for.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxl/libxl_save_callout.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_save_callout.c b/tools/libxl/libxl_save_callout.c
index 3af99af..45b9727 100644
--- a/tools/libxl/libxl_save_callout.c
+++ b/tools/libxl/libxl_save_callout.c
@@ -119,13 +119,22 @@ void libxl__save_helper_init(libxl__save_helper_state *shs)
 
 /*----- helper execution -----*/
 
+/*
+ * Both save and restore share four parameters:
+ * 1) Path to libxl-save-helper.
+ * 2) --[restore|save]-domain.
+ * 3) stream file descriptor.
+ * n) save/restore specific parameters.
+ * 4) A \0 at the end.
+ */
+#define HELPER_NR_ARGS 4
 static void run_helper(libxl__egc *egc, libxl__save_helper_state *shs,
                        const char *mode_arg, int stream_fd,
                        const int *preserve_fds, int num_preserve_fds,
                        const unsigned long *argnums, int num_argnums)
 {
     STATE_AO_GC(shs->ao);
-    const char *args[4 + num_argnums];
+    const char *args[HELPER_NR_ARGS + num_argnums];
     const char **arg = args;
     int i, rc;
 
-- 
2.1.0

Re: [PATCH 4/4] tools/libxl: run_helper - add #define for arguments.

[Ian Campbell]

On Tue, 2016-01-26 at 16:31 -0500, Konrad Rzeszutek Wilk wrote:
> Describe what the four (or more in the future) arguments
> are for.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>


applied.

> ---
>  tools/libxl/libxl_save_callout.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl_save_callout.c
> b/tools/libxl/libxl_save_callout.c
> index 3af99af..45b9727 100644
> --- a/tools/libxl/libxl_save_callout.c
> +++ b/tools/libxl/libxl_save_callout.c
> @@ -119,13 +119,22 @@ void
> libxl__save_helper_init(libxl__save_helper_state *shs)
>  
>  /*----- helper execution -----*/
>  
> +/*
> + * Both save and restore share four parameters:
> + * 1) Path to libxl-save-helper.
> + * 2) --[restore|save]-domain.
> + * 3) stream file descriptor.
> + * n) save/restore specific parameters.
> + * 4) A \0 at the end.
> + */
> +#define HELPER_NR_ARGS 4
>  static void run_helper(libxl__egc *egc, libxl__save_helper_state *shs,
>                         const char *mode_arg, int stream_fd,
>                         const int *preserve_fds, int num_preserve_fds,
>                         const unsigned long *argnums, int num_argnums)
>  {
>      STATE_AO_GC(shs->ao);
> -    const char *args[4 + num_argnums];
> +    const char *args[HELPER_NR_ARGS + num_argnums];
>      const char **arg = args;
>      int i, rc;
>  

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