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

Message ID 20200205093724.2854-1-pdurrant@amazon.com
State Superseded
Headers show
Series
  • libxl: fix assertion failure in stub domain creation
Related show

Commit Message

Paul Durrant Feb. 5, 2020, 9:37 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().

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>

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 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Anthony PERARD Feb. 5, 2020, 10:47 a.m. UTC | #1
On Wed, Feb 05, 2020 at 09:37:24AM +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().
> 
> Fixes: 75259239d85d ("libxl_create: make 'soft reset' explicit")
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> ---
>  tools/libxl/libxl_dm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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;

How this works? INVALID_DOMID seems to be directly feed to
xc_domain_create(), which is using XEN_DOMCTL_createdomain.
But a comment in domctl.h for XEN_DOMCTL_createdomain read:
    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.
So, is xc_domain_create going to create a new domain with
domid==INVALID_DOMID?

Thanks,
Durrant, Paul Feb. 5, 2020, 10:50 a.m. UTC | #2
> -----Original Message-----
> From: Anthony PERARD <anthony.perard@citrix.com>
> Sent: 05 February 2020 10:47
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: xen-devel@lists.xenproject.org; Ian Jackson
> <ian.jackson@eu.citrix.com>; Wei Liu <wl@xen.org>; Roger Pau Monné
> <roger.pau@citrix.com>
> Subject: Re: [PATCH] libxl: fix assertion failure in stub domain creation
> 
> On Wed, Feb 05, 2020 at 09:37:24AM +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().
> >
> > Fixes: 75259239d85d ("libxl_create: make 'soft reset' explicit")
> > Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> > ---
> >  tools/libxl/libxl_dm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > 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;
> 
> How this works? INVALID_DOMID seems to be directly feed to
> xc_domain_create(), which is using XEN_DOMCTL_createdomain.
> But a comment in domctl.h for XEN_DOMCTL_createdomain read:
>     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.
> So, is xc_domain_create going to create a new domain with
> domid==INVALID_DOMID?
> 

As it happens, no. That comment is wrong. It should read "If it is not set to a valid value, an id is auto-allocated and returned".

  Paul
Anthony PERARD Feb. 5, 2020, 11:23 a.m. UTC | #3
On Wed, Feb 05, 2020 at 10:50:46AM +0000, Durrant, Paul wrote:
> > From: Anthony PERARD <anthony.perard@citrix.com>
> > On Wed, Feb 05, 2020 at 09:37:24AM +0000, Paul Durrant wrote:
> > >
> > > -    sdss->pvqemu.guest_domid = 0;
> > > +    sdss->pvqemu.guest_domid = INVALID_DOMID;
> > 
> > How this works? INVALID_DOMID seems to be directly feed to
> > xc_domain_create(), which is using XEN_DOMCTL_createdomain.
> > But a comment in domctl.h for XEN_DOMCTL_createdomain read:
> >     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.
> > So, is xc_domain_create going to create a new domain with
> > domid==INVALID_DOMID?
> > 
> 
> As it happens, no. That comment is wrong. It should read "If it is not set to a valid value, an id is auto-allocated and returned".

Ok, then:
Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

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;