[v2] libxl: fix assertion failure in stub domain creation
diff mbox series

Message ID 20200205112412.19414-1-pdurrant@amazon.com
State New
Headers show
Series
  • [v2] libxl: fix assertion failure in stub domain creation
Related show

Commit Message

Paul Durrant Feb. 5, 2020, 11:24 a.m. UTC
An assertion in libxl__domain_make():

'soft_reset || *domid == INVALID_DOMID'

does not hold true for stub domain creation, where soft_reset is false
but the passed in domid == 0. This is easily fixed by changing the
initializer in libxl__spawn_stub_dm().

NOTE: The comment for XEN_DOMCTL_createdomain in domctl.h is changed to
      reflect reality.

Fixes: 75259239d85d ("libxl_create: make 'soft reset' explicit")
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: Roger Pau Monné <roger.pau@citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>

An example of the assertion failure can be seen at:

http://logs.test-lab.xenproject.org/osstest/logs/146726/test-amd64-amd64-xl-qemut-stubdom-debianhvm-amd64-xsm/10.ts-debian-hvm-install.log
---
 tools/libxl/libxl_dm.c      | 2 +-
 xen/include/public/domctl.h | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

Comments

Wei Liu Feb. 5, 2020, 11:27 a.m. UTC | #1
On Wed, Feb 05, 2020 at 11:24:12AM +0000, Paul Durrant wrote:
> An assertion in libxl__domain_make():
> 
> 'soft_reset || *domid == INVALID_DOMID'
> 
> does not hold true for stub domain creation, where soft_reset is false
> but the passed in domid == 0. This is easily fixed by changing the
> initializer in libxl__spawn_stub_dm().
> 
> NOTE: The comment for XEN_DOMCTL_createdomain in domctl.h is changed to
>       reflect reality.
> 
> Fixes: 75259239d85d ("libxl_create: make 'soft reset' explicit")
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>

Acked-by: Wei Liu <wl@xen.org>

I will wait until end of today in case others want to comment on the
domctl.h change.

I will also carry over Anthony's ack from v1 while committing.

Wei.

> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wl@xen.org>
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> Cc: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Julien Grall <julien@xen.org>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> 
> An example of the assertion failure can be seen at:
> 
> http://logs.test-lab.xenproject.org/osstest/logs/146726/test-amd64-amd64-xl-qemut-stubdom-debianhvm-amd64-xsm/10.ts-debian-hvm-install.log
> ---
>  tools/libxl/libxl_dm.c      | 2 +-
>  xen/include/public/domctl.h | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index f758daf3b6..3b1da90167 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -2127,7 +2127,7 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
>          goto out;
>      }
>  
> -    sdss->pvqemu.guest_domid = 0;
> +    sdss->pvqemu.guest_domid = INVALID_DOMID;
>  
>      libxl_domain_create_info_init(&dm_config->c_info);
>      dm_config->c_info.type = LIBXL_DOMAIN_TYPE_PV;
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 2bb7397923..fec6f6fdd1 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -42,7 +42,8 @@
>  
>  /*
>   * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
> - * If it is specified as zero, an id is auto-allocated and returned.
> + * If it is specified as an invalid value (0 or >= DOMID_FIRST_RESERVED),
> + * an id is auto-allocated and returned.
>   */
>  /* XEN_DOMCTL_createdomain */
>  struct xen_domctl_createdomain {
> -- 
> 2.20.1
>
Jan Beulich Feb. 5, 2020, 11:29 a.m. UTC | #2
On 05.02.2020 12:24, Paul Durrant wrote:
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -42,7 +42,8 @@
>  
>  /*
>   * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
> - * If it is specified as zero, an id is auto-allocated and returned.
> + * If it is specified as an invalid value (0 or >= DOMID_FIRST_RESERVED),
> + * an id is auto-allocated and returned.
>   */
>  /* XEN_DOMCTL_createdomain */
>  struct xen_domctl_createdomain {

Acked-by: Jan Beulich <jbeulich@suse.com>
Anthony PERARD Feb. 5, 2020, 11:29 a.m. UTC | #3
On Wed, Feb 05, 2020 at 11:24:12AM +0000, Paul Durrant wrote:
> NOTE: The comment for XEN_DOMCTL_createdomain in domctl.h is changed to
>       reflect reality.

Surely, this change deserve to be in its own patch, isn't it?
Wei Liu Feb. 5, 2020, 11:43 a.m. UTC | #4
On Wed, Feb 05, 2020 at 11:29:30AM +0000, Anthony PERARD wrote:
> On Wed, Feb 05, 2020 at 11:24:12AM +0000, Paul Durrant wrote:
> > NOTE: The comment for XEN_DOMCTL_createdomain in domctl.h is changed to
> >       reflect reality.
> 
> Surely, this change deserve to be in its own patch, isn't it?

Ideally I would want that too. But this patch is trivial so I'm not too
fussed either way.

Wei.

> 
> -- 
> Anthony PERARD

Patch
diff mbox series

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index f758daf3b6..3b1da90167 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2127,7 +2127,7 @@  void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
         goto out;
     }
 
-    sdss->pvqemu.guest_domid = 0;
+    sdss->pvqemu.guest_domid = INVALID_DOMID;
 
     libxl_domain_create_info_init(&dm_config->c_info);
     dm_config->c_info.type = LIBXL_DOMAIN_TYPE_PV;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 2bb7397923..fec6f6fdd1 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -42,7 +42,8 @@ 
 
 /*
  * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
- * If it is specified as zero, an id is auto-allocated and returned.
+ * If it is specified as an invalid value (0 or >= DOMID_FIRST_RESERVED),
+ * an id is auto-allocated and returned.
  */
 /* XEN_DOMCTL_createdomain */
 struct xen_domctl_createdomain {