Xen devel - Sep 2013 - guests started with xl create -V have no monitoring process

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

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

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

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.

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

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

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.