guests started with xl create -V have no monitoring process

guests started with xl create -V have no monitoring process

[Olaf Hering]

The following command leaves no xl monitoring process for the domU. The
result is that shutdown for this guest is not working properly, there is
always a stale domid left behind:

xl -v create -V -d -f some.cfg < /dev/null &

Removing the -V option is a workaround. This happens with 4.3 and is not
fixed in master.

Olaf

Re: guests started with xl create -V have no monitoring process

[Ian Campbell]

On Fri, 2013-09-27 at 11:10 +0200, Olaf Hering wrote:
> The following command leaves no xl monitoring process for the domU. The
> result is that shutdown for this guest is not working properly, there is
> always a stale domid left behind:
> 
> xl -v create -V -d -f some.cfg < /dev/null &
> 
> Removing the -V option is a workaround. This happens with 4.3 and is not
> fixed in master.

The placement of 
    if (dom_info->vnc)
        vncviewer(domid, vncautopass);
seems pretty bogus, given that function exec()s without forking!

Does this help? Build tested only.

8<------------------

>From 93c3ee2d9158cbcc3e7377c44cecff971ffd35fd Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Fri, 27 Sep 2013 11:16:22 +0100
Subject: [PATCH] xl: fork before execing vncviewer

Otherwise we don't daemonize to monitor the domain.

Heavily cargo-culted from autoconnect-console and only compile tested.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Olaf Hering <olaf@aepfle.de>
---
 tools/libxl/xl.h         |    2 +-
 tools/libxl/xl_cmdimpl.c |   33 ++++++++++++++++++++++++++++++++-
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index e72a7d2..e005c39 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -128,7 +128,7 @@ typedef struct {
 } xlchild;
 
 typedef enum {
-    child_console, child_waitdaemon, child_migration,
+    child_console, child_waitdaemon, child_migration, child_vncviewer,
     child_max
 } xlchildnum;
 
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 3d7eaad..a282891 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -194,6 +194,37 @@ static int vncviewer(uint32_t domid, int autopass)
     return 1;
 }
 
+static void vncviewer_child_report(void)
+{
+    if (xl_child_pid(child_vncviewer)) {
+        int status;
+        pid_t got = xl_waitpid(child_vncviewer, &status, 0);
+        if (got < 0)
+            perror("xl: warning, failed to waitpid for vncviewer child");
+        else if (status)
+            libxl_report_child_exitstatus(ctx, XTL_ERROR, "vncviewer child",
+                                          xl_child_pid(child_vncviewer), status);
+    }
+}
+
+static void autoconnect_vncviewer(uint32_t domid, int autopass)
+{
+    vncviewer_child_report();
+
+    pid_t pid = xl_fork(child_vncviewer);
+    if (pid < 0) {
+        perror("unable to fork vncviewer");
+        return;
+    } else if (pid > 0)
+        return;
+
+    postfork();
+
+    sleep(1);
+    vncviewer(domid, autopass);
+    _exit(1);
+}
+
 static int acquire_lock(void)
 {
     int rc;
@@ -2092,7 +2123,7 @@ start:
         goto out;
 
     if (dom_info->vnc)
-        vncviewer(domid, vncautopass);
+        autoconnect_vncviewer(domid, vncautopass);
 
     if (need_daemon) {
         char *fullname, *name;
-- 
1.7.10.4

Re: guests started with xl create -V have no monitoring process

[Olaf Hering]

On Fri, Sep 27, Ian Campbell wrote:

> On Fri, 2013-09-27 at 11:10 +0200, Olaf Hering wrote:
> > The following command leaves no xl monitoring process for the domU. The
> > result is that shutdown for this guest is not working properly, there is
> > always a stale domid left behind:
> > 
> > xl -v create -V -d -f some.cfg < /dev/null &
> > 
> > Removing the -V option is a workaround. This happens with 4.3 and is not
> > fixed in master.
> 
> The placement of 
>     if (dom_info->vnc)
>         vncviewer(domid, vncautopass);
> seems pretty bogus, given that function exec()s without forking!
> 
> Does this help? Build tested only.

Yes, that works. Thanks.


Olaf

Re: guests started with xl create -V have no monitoring process

[Ian Campbell]

On Fri, 2013-09-27 at 14:53 +0200, Olaf Hering wrote:
> On Fri, Sep 27, Ian Campbell wrote:
> 
> > On Fri, 2013-09-27 at 11:10 +0200, Olaf Hering wrote:
> > > The following command leaves no xl monitoring process for the domU. The
> > > result is that shutdown for this guest is not working properly, there is
> > > always a stale domid left behind:
> > > 
> > > xl -v create -V -d -f some.cfg < /dev/null &
> > > 
> > > Removing the -V option is a workaround. This happens with 4.3 and is not
> > > fixed in master.
> > 
> > The placement of 
> >     if (dom_info->vnc)
> >         vncviewer(domid, vncautopass);
> > seems pretty bogus, given that function exec()s without forking!
> > 
> > Does this help? Build tested only.
> 
> Yes, that works. Thanks.

Thanks, I'll translate that into a Tested-by.

Ideally we'd also wait for an Ack from Ian J (since he understands all
this forking infra), but he's away so I've gone ahead and committed. We
can always revert if it turns out to be wrong.

Ian.

Re: guests started with xl create -V have no monitoring process

[Olaf Hering]

On Thu, Oct 03, Ian Campbell wrote:

> Ideally we'd also wait for an Ack from Ian J (since he understands all
> this forking infra), but he's away so I've gone ahead and committed. We
> can always revert if it turns out to be wrong.

Please consider backporting of the final patch to 4.3.


Olaf

Re: guests started with xl create -V have no monitoring process

[Ian Campbell]

On Fri, 2013-10-04 at 10:20 +0200, Olaf Hering wrote:
> On Thu, Oct 03, Ian Campbell wrote:
> 
> > Ideally we'd also wait for an Ack from Ian J (since he understands all
> > this forking infra), but he's away so I've gone ahead and committed. We
> > can always revert if it turns out to be wrong.
> 
> Please consider backporting of the final patch to 4.3.

That's one for Ian J when he gets back, but yes it seems like a good
idea.

Ian

Re: guests started with xl create -V have no monitoring process

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] guests started with xl create -V have no monitoring process"):
> Ideally we'd also wait for an Ack from Ian J (since he understands all
> this forking infra), but he's away so I've gone ahead and committed. We
> can always revert if it turns out to be wrong.

The new code looks correct to me.  But it's rather too clone-and-hack
for my taste.  I'll see if I can improve it.

Thanks,
Ian.