[PATCH 0/2] Two stubdom patches

[PATCH 0/2] Two stubdom patches

[Wei Liu]

I discovered some problems as I played along with my OSSTest stubdom test case.
Here are two patches to fix them.

Wei.

Wei Liu (2):
  libxl: remove code in stubdom creation failure path and callback
  libxl: clean up qemu-save and qemu-resume files

 tools/libxl/libxl.c    |  9 +++++++++
 tools/libxl/libxl_dm.c | 27 ---------------------------
 2 files changed, 9 insertions(+), 27 deletions(-)

-- 
1.9.1

[PATCH 1/2] libxl: remove code in stubdom creation failure path and callback

[Wei Liu]

The snippet to destroy stubdom and the callback were added in 1fc3aeb3
("libxl: use new QEMU xenstore protocol"). The intention was to destroy
stubdom when it is not responsive. That approach is problematic because
rc is not propagate back to sdss->callback, hence the guest is leaked.

The solution is simple. The destruction of stubdom can be done later in
sdss->callback. That code path already does the right thing to destroy
both the guest and the stubdom that serves the guest.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_dm.c | 27 ---------------------------
 1 file changed, 27 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 3dd7c04..33f9ce6 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1036,9 +1036,6 @@ static void stubdom_pvqemu_cb(libxl__egc *egc,
                               libxl__multidev *aodevs,
                               int rc);
 
-static void spawn_stubdom_pvqemu_destroy_cb(libxl__egc *egc,
-                                            libxl__destroy_domid_state *dis,
-                                            int rc);
 static void stubdom_xswait_cb(libxl__egc *egc, libxl__xswait_state *xswait,
                               int rc, const char *p);
 
@@ -1353,7 +1350,6 @@ static void stubdom_xswait_cb(libxl__egc *egc, libxl__xswait_state *xswait,
 {
     EGC_GC;
     libxl__stub_dm_spawn_state *sdss = CONTAINER_OF(xswait, *sdss, xswait);
-    uint32_t dm_domid = sdss->pvqemu.guest_domid;
 
     if (rc) {
         if (rc == ERROR_TIMEDOUT)
@@ -1367,29 +1363,6 @@ static void stubdom_xswait_cb(libxl__egc *egc, libxl__xswait_state *xswait,
         return;
  out:
     libxl__xswait_stop(gc, xswait);
-    if (rc) {
-        if (dm_domid) {
-            sdss->dis.ao = sdss->dm.spawn.ao;
-            sdss->dis.domid = dm_domid;
-            sdss->dis.callback = spawn_stubdom_pvqemu_destroy_cb;
-            libxl__destroy_domid(egc, &sdss->dis);
-            return;
-        }
-    }
-    sdss->callback(egc, &sdss->dm, rc);
-}
-
-static void spawn_stubdom_pvqemu_destroy_cb(libxl__egc *egc,
-                                            libxl__destroy_domid_state *dis,
-                                            int rc)
-{
-    libxl__stub_dm_spawn_state *sdss = CONTAINER_OF(dis, *sdss, dis);
-    STATE_AO_GC(sdss->dis.ao);
-
-    if (rc)
-        LOG(ERROR, "destruction of domain %u after failed creation failed",
-                   sdss->pvqemu.guest_domid);
-
     sdss->callback(egc, &sdss->dm, rc);
 }
 
-- 
1.9.1

[PATCH 2/2] libxl: clean up qemu-save and qemu-resume files

[Wei Liu]

These files are leaked when using qemu-trad stubdom.  They are
intermediate files created by libxc. Unfortunately they don't fit well
in our userdata scheme. Clean them up after we destroy guest, we're
sure they are not useful anymore at that point.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 9117b01..ad2290d 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1686,6 +1686,15 @@ static void devices_destroy_cb(libxl__egc *egc,
 
         rc = xc_domain_destroy(ctx->xch, domid);
         if (rc < 0) goto badchild;
+        /* Clean up qemu-save and qemu-resume files. They are
+         * intermediate files created by libxc. Unfortunately they
+         * don't fit in existing userdata scheme very well.
+         */
+        rc = libxl__remove_file(gc, libxl__device_model_savefile(gc, domid));
+        if (rc < 0) goto badchild;
+        rc = libxl__remove_file(gc,
+                 GCSPRINTF(XC_DEVICE_MODEL_RESTORE_FILE".%u", domid));
+        if (rc < 0) goto badchild;
         _exit(0);
 
     badchild:
-- 
1.9.1

Re: [PATCH 2/2] libxl: clean up qemu-save and qemu-resume files

[Ian Campbell]

On Mon, 2015-06-01 at 18:24 +0100, Wei Liu wrote:
> These files are leaked when using qemu-trad stubdom.  They are
> intermediate files created by libxc. Unfortunately they don't fit well
> in our userdata scheme. Clean them up after we destroy guest, we're
> sure they are not useful anymore at that point.

Could this be done in the parent process at some point following
domain_destroy_domid_cb or domain_destroy_cb perhaps?

I think we don't want to do things in sub processes which don't need to
be, just to keep things simpler, and I think the logging is more
reliable too.


> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 9117b01..ad2290d 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1686,6 +1686,15 @@ static void devices_destroy_cb(libxl__egc *egc,
>  
>          rc = xc_domain_destroy(ctx->xch, domid);
>          if (rc < 0) goto badchild;
> +        /* Clean up qemu-save and qemu-resume files. They are
> +         * intermediate files created by libxc. Unfortunately they
> +         * don't fit in existing userdata scheme very well.
> +         */
> +        rc = libxl__remove_file(gc, libxl__device_model_savefile(gc, domid));
> +        if (rc < 0) goto badchild;
> +        rc = libxl__remove_file(gc,
> +                 GCSPRINTF(XC_DEVICE_MODEL_RESTORE_FILE".%u", domid));
> +        if (rc < 0) goto badchild;
>          _exit(0);
>  
>      badchild:

Re: [PATCH 2/2] libxl: clean up qemu-save and qemu-resume files

[Wei Liu]

On Wed, Jun 03, 2015 at 10:58:51AM +0100, Ian Campbell wrote:
> On Mon, 2015-06-01 at 18:24 +0100, Wei Liu wrote:
> > These files are leaked when using qemu-trad stubdom.  They are
> > intermediate files created by libxc. Unfortunately they don't fit well
> > in our userdata scheme. Clean them up after we destroy guest, we're
> > sure they are not useful anymore at that point.
> 
> Could this be done in the parent process at some point following
> domain_destroy_domid_cb or domain_destroy_cb perhaps?
> 

Yes, of course.

> I think we don't want to do things in sub processes which don't need to
> be, just to keep things simpler, and I think the logging is more
> reliable too.
> 
> 
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> >  tools/libxl/libxl.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index 9117b01..ad2290d 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -1686,6 +1686,15 @@ static void devices_destroy_cb(libxl__egc *egc,
> >  
> >          rc = xc_domain_destroy(ctx->xch, domid);
> >          if (rc < 0) goto badchild;
> > +        /* Clean up qemu-save and qemu-resume files. They are
> > +         * intermediate files created by libxc. Unfortunately they
> > +         * don't fit in existing userdata scheme very well.
> > +         */
> > +        rc = libxl__remove_file(gc, libxl__device_model_savefile(gc, domid));
> > +        if (rc < 0) goto badchild;
> > +        rc = libxl__remove_file(gc,
> > +                 GCSPRINTF(XC_DEVICE_MODEL_RESTORE_FILE".%u", domid));
> > +        if (rc < 0) goto badchild;
> >          _exit(0);
> >  
> >      badchild:
> 

Re: [PATCH 2/2] libxl: clean up qemu-save and qemu-resume files

[Wei Liu]

On Wed, Jun 03, 2015 at 10:58:51AM +0100, Ian Campbell wrote:
> On Mon, 2015-06-01 at 18:24 +0100, Wei Liu wrote:
> > These files are leaked when using qemu-trad stubdom.  They are
> > intermediate files created by libxc. Unfortunately they don't fit well
> > in our userdata scheme. Clean them up after we destroy guest, we're
> > sure they are not useful anymore at that point.
> 
> Could this be done in the parent process at some point following
> domain_destroy_domid_cb or domain_destroy_cb perhaps?
> 

I just had a look. The deletion of qemu-* files can be moved a few lines
up, just after we destroy all userdata files and before forking. No need
to defer it in a callback.

Wei.

> I think we don't want to do things in sub processes which don't need to
> be, just to keep things simpler, and I think the logging is more
> reliable too.
> 
> 
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> >  tools/libxl/libxl.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index 9117b01..ad2290d 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -1686,6 +1686,15 @@ static void devices_destroy_cb(libxl__egc *egc,
> >  
> >          rc = xc_domain_destroy(ctx->xch, domid);
> >          if (rc < 0) goto badchild;
> > +        /* Clean up qemu-save and qemu-resume files. They are
> > +         * intermediate files created by libxc. Unfortunately they
> > +         * don't fit in existing userdata scheme very well.
> > +         */
> > +        rc = libxl__remove_file(gc, libxl__device_model_savefile(gc, domid));
> > +        if (rc < 0) goto badchild;
> > +        rc = libxl__remove_file(gc,
> > +                 GCSPRINTF(XC_DEVICE_MODEL_RESTORE_FILE".%u", domid));
> > +        if (rc < 0) goto badchild;
> >          _exit(0);
> >  
> >      badchild:
> 

Re: [PATCH 1/2] libxl: remove code in stubdom creation failure path and callback

[Ian Campbell]

On Mon, 2015-06-01 at 18:24 +0100, Wei Liu wrote:
> The snippet to destroy stubdom and the callback were added in 1fc3aeb3
> ("libxl: use new QEMU xenstore protocol"). The intention was to destroy
> stubdom when it is not responsive. That approach is problematic because
> rc is not propagate back to sdss->callback, hence the guest is leaked.
> 
> The solution is simple. The destruction of stubdom can be done later in
> sdss->callback. That code path already does the right thing to destroy
> both the guest and the stubdom that serves the guest.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked + applied, thanks,