[PATCH] Fix bootloader handling when empty string is being output

[PATCH] Fix bootloader handling when empty string is being output

[Michal Novotny]

[-- Attachment #1: Type: text/plain, Size: 2236 bytes --]

Hi,
this is the patch to fix empty string as the output value
of the bootloader string. If there is no output then
xend is being hung indefinitely unless you press Ctrl + C keys
to trigger SIGINT. This patch fixes the issue by implementing
timeout to the select() function for reading from pipe and also
checking for state of the process - i.e. whether the process is
running or whether it's dead or zombie (because fork() call does
leave zombies when the process is done but the parent process is
still running).

I tried it with the bogus bootloader that just returns with
error code 0 and also the bogus bootloader that sleeps for 10
seconds and then returns with error code 0. According to my
testing when the bootloader process has finished and the output
of the process is empty it fails with "bootloader didn't return
any data" message which is the expected behaviour. This patch
has also been tested with the various timeout values (incl. no
timeout specified) for pyGrub and everything was working fine
since it was failing *only* in the case both output from pyGrub
was empty and the bootloader process was not running according
the pid.

The check for bootloader running is implemented by checking
the presence of /proc/{$PID} and also state in the stat file
(/proc/{$PID}/stat).

Testing: The patch has been tested on RHEL-5.5 x86_64 dom0 with
RHEL-5.4 PV guest both with good and bad bootloader setup. By bad
bootloader I mean just the shell script exiting with code 0 and
also shell script sleeping for 10 seconds and exiting with code 0.
Before my patch applied it got stuck indefinitely in the xend
because of waiting for some data on select() call and after my
patch applied it's opening the FIFO with O_NDELAY and tries to
read the data indefinitely every second using the select() call
with 1 second as the timeout argument. If there were no data
output using the read call it just fails after the child
(bootloader) process exited. Otherwise, when bootloader did
return some data, the guest started successfully no matter
what the timeout was defined in pyGrub.

Signed-off-by: Michal Novotny <minovotn@redhat.com>

Michal

-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat


[-- Attachment #2: fix-bootloader-empty-output-handling.patch --]
[-- Type: text/x-patch, Size: 4891 bytes --]

>From b145011be18b700e89234f6ddc12e32872c2c121 Mon Sep 17 00:00:00 2001
From: Michal Novotny <minovotn@redhat.com>
Date: Mon, 30 Aug 2010 15:06:05 +0200
Subject: [PATCH] Fix bootloader handling when empty string is being output

This is the patch to fix empty string as the output value
of the bootloader string. If there is no output then
xend is being hung indefinitely unless you press Ctrl + C keys
to trigger SIGINT. This patch fixes the issue by implementing
timeout to the select() function for reading from pipe and also
checking for state of the process - i.e. whether the process is
running or whether it's dead or zombie (because fork() call does
leave zombies when the process is done but the parent process is
still running).

I tried it with the bogus bootloader that just returns with
error code 0 and also the bogus bootloader that sleeps for 10
seconds and then returns with error code 0. According to my
testing when the bootloader process has finished and the output
of the process is empty it fails with "bootloader didn't return
any data" message which is the expected behaviour. This patch
has also been tested with the various timeout values (incl. no
timeout specified) for pyGrub and everything was working fine
since it was failing *only* in the case both output from pyGrub
was empty and the bootloader process was not running according
the pid.

The check for bootloader running is implemented by checking
the presence of /proc/{$PID} and also state in the stat file
(/proc/{$PID}/stat).

Testing: The patch has been tested on RHEL-5.5 x86_64 dom0 with
RHEL-5.4 PV guest both with good and bad bootloader setup. By bad
bootloader I mean just the shell script exiting with code 0 and
also shell script sleeping for 10 seconds and exiting with code 0.
Before my patch applied it got stuck indefinitely in the xend
because of waiting for some data on select() call and after my
patch applied it's opening the FIFO with O_NDELAY and tries to
read the data indefinitely every second using the select() call
with 1 second as the timeout argument. If there were no data
output using the read call it just fails after the child
(bootloader) process exited. Otherwise, when bootloader did
return some data, the guest started successfully no matter
what the timeout was defined in pyGrub.

Signed-off-by: Michal Novotny <minovotn@redhat.com>

Michal
---
 tools/python/xen/xend/XendBootloader.py |   36 ++++++++++++++++++++++++++----
 1 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/tools/python/xen/xend/XendBootloader.py b/tools/python/xen/xend/XendBootloader.py
index 0cef917..66a688f 100644
--- a/tools/python/xen/xend/XendBootloader.py
+++ b/tools/python/xen/xend/XendBootloader.py
@@ -24,6 +24,24 @@ from XendError import VmError
 import pty, termios, fcntl
 from xen.lowlevel import ptsname
 
+def hasPidInProcFS(pid):
+    path = "/proc/%s" % pid
+    return os.path.exists(path)
+
+def isRunning(pid):
+    path = "/proc/%s" % pid
+    if not os.path.exists(path):
+        return False
+    fd = open("%s/stat" % path)
+    val = fd.readline().split()[2]
+    fd.close()
+    try:
+        val.index('Z')
+        ret = False
+    except:
+        ret = True
+    return ret
+
 def bootloader(blexec, disk, dom, quiet = False, blargs = '', kernel = '',
                ramdisk = '', kernel_args = ''):
     """Run the boot loader executable on the given disk and return a
@@ -128,16 +146,22 @@ def bootloader(blexec, disk, dom, quiet = False, blargs = '', kernel = '',
     if os.uname()[0] != 'SunOS':
         tty.setraw(m2);
 
-    fcntl.fcntl(m2, fcntl.F_SETFL, os.O_NDELAY);
+    # We check whether OS supports procfs in /proc like Linux does
+    canUseProcfs = hasPidInProcFS( os.getpid() )
+
+    fcntl.fcntl(m2, fcntl.F_SETFL, os.O_NDELAY)
     while True:
         try:
-            r = os.open(fifo, os.O_RDONLY)
+            mode = os.O_RDONLY
+            if canUseProcfs:
+                mode |= os.O_NDELAY
+            r = os.open(fifo, mode)
         except OSError, e:
             if e.errno == errno.EINTR:
                 continue
         break
 
-    fcntl.fcntl(r, fcntl.F_SETFL, os.O_NDELAY);
+    fcntl.fcntl(r, fcntl.F_SETFL, os.O_NDELAY)
 
     ret = ""
     inbuf=""; outbuf="";
@@ -156,13 +180,15 @@ def bootloader(blexec, disk, dom, quiet = False, blargs = '', kernel = '',
     # if there is actual data to write, otherwise this would loop too fast,
     # eating up CPU time.
 
-    while True:
+    while ((canUseProcfs and isRunning(child)) or not canUseProcfs):
         wsel = []
         if len(outbuf) != 0:
             wsel = wsel + [m1]
         if len(inbuf) != 0:
             wsel = wsel + [m2]
-        sel = select.select([r, m1, m2], wsel, [])
+
+        sel = select.select([r, m1, m2], wsel, [], 1)
+
         try: 
             if m1 in sel[0]:
                 s = os.read(m1, 16)
-- 
1.5.5.6


[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] Fix bootloader handling when empty string is being output

[Ian Campbell]

On Mon, 2010-08-30 at 14:10 +0100, Michal Novotny wrote:
> Hi,
> this is the patch to fix empty string as the output value
> of the bootloader string. If there is no output then
> xend is being hung indefinitely unless you press Ctrl + C keys
> to trigger SIGINT.

I think a similar fix will be needed to libxl_bootloader.c, right?

A couple of questions:

Couldn't the non-portable usage of /proc be portably replaced with
something like waitpid+WNOHANG? I guess it would be sane to put one of
these inside the loop which performs the bootloader interaction to check
the bootloader still exists.

Could we potentially avoid the need to use a select timeout to poll for
bootloader exit by including the bootloader FIFO FD and/or the PTY FDs
in the select's exceptfds array? Presumably if the process on the other
end of such an FD exits that causes some sort of exceptional condition
(although historically I've had trouble finding the actual specified
behaviour in cases like this).

Ian.

Re: [PATCH] Fix bootloader handling when empty string is being output

[Paolo Bonzini]

On 08/31/2010 10:47 AM, Ian Campbell wrote:
> On Mon, 2010-08-30 at 14:10 +0100, Michal Novotny wrote:
>> Hi,
>> this is the patch to fix empty string as the output value
>> of the bootloader string. If there is no output then
>> xend is being hung indefinitely unless you press Ctrl + C keys
>> to trigger SIGINT.
>
> I think a similar fix will be needed to libxl_bootloader.c, right?

Yes.

> Could we potentially avoid the need to use a select timeout to poll for
> bootloader exit by including the bootloader FIFO FD and/or the PTY FDs
> in the select's exceptfds array? Presumably if the process on the other
> end of such an FD exits that causes some sort of exceptional condition
> (although historically I've had trouble finding the actual specified
> behaviour in cases like this).

Using the FIFO is not possible in general because you cannot be sure 
that the bootloader is opening it at all.  In fact, in this case both 
libxl and xend will hang at "fifo_fd = open(fifo, O_RDONLY);" so you 
have to apply O_NDELAY already when you open the FIFO.

Also, in the case the bootloader is not opening the FIFO at all, doing 
the waitpid in the same thread has a race if the process exits between 
the waitpid and select.  Fixing the race requires pselect and, 
especially in Python, introduces more complications than it removes.

So, for RHEL5 xend, Michal has a better version of the patch that will 
run the waitpid in a separate thread, and use a pipe to wake up the 
select.  This does the same thing as /proc, but portably and without the 
need for polling.

However, for both current xend and libxl, using the PTY sounds better, 
and it looks like an even simpler patch will work that:

- uses O_RDONLY|O_NDELAY when opening the FIFO, and

- exits bootloader_interact when bootloader_fd was in the returned rsel 
but ret == 0.

Paolo

Re: [PATCH] Fix bootloader handling when empty string is being output

[Michal Novotny]

On 08/31/2010 11:16 AM, Paolo Bonzini wrote:
> On 08/31/2010 10:47 AM, Ian Campbell wrote:
>> On Mon, 2010-08-30 at 14:10 +0100, Michal Novotny wrote:
>>> Hi,
>>> this is the patch to fix empty string as the output value
>>> of the bootloader string. If there is no output then
>>> xend is being hung indefinitely unless you press Ctrl + C keys
>>> to trigger SIGINT.
>>
>> I think a similar fix will be needed to libxl_bootloader.c, right?
>
> Yes.
>
>> Could we potentially avoid the need to use a select timeout to poll for
>> bootloader exit by including the bootloader FIFO FD and/or the PTY FDs
>> in the select's exceptfds array? Presumably if the process on the other
>> end of such an FD exits that causes some sort of exceptional condition
>> (although historically I've had trouble finding the actual specified
>> behaviour in cases like this).
>
> Using the FIFO is not possible in general because you cannot be sure 
> that the bootloader is opening it at all.  In fact, in this case both 
> libxl and xend will hang at "fifo_fd = open(fifo, O_RDONLY);" so you 
> have to apply O_NDELAY already when you open the FIFO.
>
> Also, in the case the bootloader is not opening the FIFO at all, doing 
> the waitpid in the same thread has a race if the process exits between 
> the waitpid and select.  Fixing the race requires pselect and, 
> especially in Python, introduces more complications than it removes.
>
> So, for RHEL5 xend, Michal has a better version of the patch that will 
> run the waitpid in a separate thread, and use a pipe to wake up the 
> select.  This does the same thing as /proc, but portably and without 
> the need for polling.
>
> However, for both current xend and libxl, using the PTY sounds better, 
> and it looks like an even simpler patch will work that:
>
> - uses O_RDONLY|O_NDELAY when opening the FIFO, and
>
> - exits bootloader_interact when bootloader_fd was in the returned 
> rsel but ret == 0.
>
> Paolo
So, Paolo, what do you recommend for upstream version? There's the PTY 
thing already so what should we do ? Ian, also, I don't know how it's 
working with upstream version since I found out that syntax like `xm 
create -c PVguest` with default settings (pyGrub bootloader) doesn't 
show the pyGrub at all so I don't know what's wrong with my setup. I'm 
using 2.6.32.15-xen kernel/hypervisor version with latest unstable 
user-space tools.

Any hint how this should be working Ian?

Thanks,
Michal

-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat

Re: [PATCH] Fix bootloader handling when empty string is being output

[Ian Campbell]

On Tue, 2010-08-31 at 11:00 +0100, Michal Novotny wrote:
> I don't know how it's 
> working with upstream version since I found out that syntax like `xm 
> create -c PVguest` with default settings (pyGrub bootloader) doesn't 
> show the pyGrub at all so I don't know what's wrong with my setup. I'm 
> using 2.6.32.15-xen kernel/hypervisor version with latest unstable 
> user-space tools.
> 
> Any hint how this should be working Ian?

It should be working as you expect, e.g. "xm create -c xxx" should show
you the pygrub output, unless you have used something like "--entry=x"
or "-q" which disable interactive mode in your bootloader_args.

I'm afraid I don't know what is broken, I'm reasonably sure it was
working for me when I developed libxl_bootloader.c since I was comparing
the two.

Ian.

Re: [PATCH] Fix bootloader handling when empty string is being output

[Michal Novotny]

On 08/31/2010 12:10 PM, Ian Campbell wrote:
> On Tue, 2010-08-31 at 11:00 +0100, Michal Novotny wrote:
>    
>> I don't know how it's
>> working with upstream version since I found out that syntax like `xm
>> create -c PVguest` with default settings (pyGrub bootloader) doesn't
>> show the pyGrub at all so I don't know what's wrong with my setup. I'm
>> using 2.6.32.15-xen kernel/hypervisor version with latest unstable
>> user-space tools.
>>
>> Any hint how this should be working Ian?
>>      
> It should be working as you expect, e.g. "xm create -c xxx" should show
> you the pygrub output, unless you have used something like "--entry=x"
> or "-q" which disable interactive mode in your bootloader_args.
>
> I'm afraid I don't know what is broken, I'm reasonably sure it was
> working for me when I developed libxl_bootloader.c since I was comparing
> the two.
>
> Ian.
>
>    
No Ian, it's not working. The config file is having:
...
bootloader = "/usr/bin/pygrub"
...
So it should show the pyGrub ncurses screen, right? But it doesn't show 
anything on version cloned from git yesterday. Any ideas what may be 
wrong? Maybe a configuration file should be somewhat different, I don't 
know but this is working fine to get the data from image - it just 
doesn't show the ncurses pyGrub screen. Isn't it possible this is 
somehow connected to the PTY patch ? How should the PTY c/s 13540 ? This 
is the rewrite that was there and honestly I don't remember whether I 
tried to run `xm create -c PVguest` on upstream Xen-4.1 ever so I don't 
know. Isn't it hypervisor/kernel related that it requires some data that 
are not coming from there?

Thanks,
Michal

-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat

Re: [PATCH] Fix bootloader handling when empty string is being output

[Paolo Bonzini]

On 08/31/2010 12:18 PM, Michal Novotny wrote:
> Isn't it possible this is
> somehow connected to the PTY patch ? How should the PTY c/s 13540 ?

This one has been there since Xen 3.1, and it's, ehm, unlikely that it 
broke that much time ago. :)

> Isn't it hypervisor/kernel related that it requires some data that
> are not coming from there?

Huh???

Paolo

Re: [PATCH] Fix bootloader handling when empty string is being output

[Ian Campbell]

On Tue, 2010-08-31 at 11:18 +0100, Michal Novotny wrote:
> On 08/31/2010 12:10 PM, Ian Campbell wrote:
> > On Tue, 2010-08-31 at 11:00 +0100, Michal Novotny wrote:
> >    
> >> I don't know how it's
> >> working with upstream version since I found out that syntax like `xm
> >> create -c PVguest` with default settings (pyGrub bootloader) doesn't
> >> show the pyGrub at all so I don't know what's wrong with my setup. I'm
> >> using 2.6.32.15-xen kernel/hypervisor version with latest unstable
> >> user-space tools.
> >>
> >> Any hint how this should be working Ian?
> >>      
> > It should be working as you expect, e.g. "xm create -c xxx" should show
> > you the pygrub output, unless you have used something like "--entry=x"
> > or "-q" which disable interactive mode in your bootloader_args.
> >
> > I'm afraid I don't know what is broken, I'm reasonably sure it was
> > working for me when I developed libxl_bootloader.c since I was comparing
> > the two.
> >
> > Ian.
> >
> >    
> No Ian, it's not working. The config file is having:
> ...
> bootloader = "/usr/bin/pygrub"
> ...
> So it should show the pyGrub ncurses screen, right?

Correct.

>  But it doesn't show 
> anything on version cloned from git yesterday.

xen-unstable or xen-4.0-testing or something else?

It looks like this was broken in xen-unstable with 21994:2e08ec0028e4.
The patch below should fix it.

Ian.

Subject: libxl+xend: use correct paths for PV console when running bootloader

Makes "{xl,xm} create -c GUEST" work again with pygrub in interactive
mode which was broken by 21994:2e08ec0028e4

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
diff -r f77e54fadc18 tools/libxl/libxl_bootloader.c
--- a/tools/libxl/libxl_bootloader.c	Tue Aug 31 09:54:18 2010 +0100
+++ b/tools/libxl/libxl_bootloader.c	Tue Aug 31 11:34:20 2010 +0100
@@ -383,7 +383,7 @@ int libxl_run_bootloader(libxl_ctx *ctx,
         goto out_close;
     }
 
-    dom_console_xs_path = libxl_sprintf(&gc, "%s/serial/0/tty", libxl_xs_get_dompath(&gc, domid));
+    dom_console_xs_path = libxl_sprintf(&gc, "%s/console/tty", libxl_xs_get_dompath(&gc, domid));
     libxl_xs_write(&gc, XBT_NULL, dom_console_xs_path, "%s", dom_console_slave_tty_path);
 
     pid = fork_exec_bootloader(&bootloader_fd, (char *)info->u.pv.bootloader, args);
diff -r f77e54fadc18 tools/python/xen/util/diagnose.py
--- a/tools/python/xen/util/diagnose.py	Tue Aug 31 09:54:18 2010 +0100
+++ b/tools/python/xen/util/diagnose.py	Tue Aug 31 11:34:20 2010 +0100
@@ -77,7 +77,7 @@ def diagnose_console():
 def diagnose_console():
     port    = xstransact.Read(dompath + '/console/port')
     ringref = xstransact.Read(dompath + '/console/ring-ref')
-    tty     = xstransact.Read(dompath + '/serial/0/tty')
+    tty     = xstransact.Read(dompath + '/console/tty')
 
     if not port:
         print "Console port is missing; Xend has failed."
diff -r f77e54fadc18 tools/python/xen/xend/XendBootloader.py
--- a/tools/python/xen/xend/XendBootloader.py	Tue Aug 31 09:54:18 2010 +0100
+++ b/tools/python/xen/xend/XendBootloader.py	Tue Aug 31 11:34:20 2010 +0100
@@ -85,7 +85,7 @@ def bootloader(blexec, disk, dom, quiet 
     fcntl.fcntl(m1, fcntl.F_SETFL, os.O_NDELAY)
 
     slavename = ptsname.ptsname(m1)
-    dom.storeDom("serial/0/tty", slavename)
+    dom.storeDom("console/tty", slavename)
 
     # Release the domain lock here, because we definitely don't want 
     # a stuck bootloader to deny service to other xend clients.

Re: [PATCH] Fix bootloader handling when empty string is being output

[Michal Novotny]

On 08/31/2010 12:30 PM, Paolo Bonzini wrote:
> On 08/31/2010 12:18 PM, Michal Novotny wrote:
>> Isn't it possible this is
>> somehow connected to the PTY patch ? How should the PTY c/s 13540 ?
>
> This one has been there since Xen 3.1, and it's, ehm, unlikely that it 
> broke that much time ago. :)

Oh, maybe but in fact I don't know what version I used it last time to 
use syntax like `xm create -c PVguest`.

>
>> Isn't it hypervisor/kernel related that it requires some data that
>> are not coming from there?
>
> Huh???
>
Oh, nevermind if it's been there since Xen 3.1.

Michal

-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat

Re: [PATCH] Fix bootloader handling when empty string is being output

[Michal Novotny]

On 08/31/2010 12:37 PM, Ian Campbell wrote:
> On Tue, 2010-08-31 at 11:18 +0100, Michal Novotny wrote:
>    
>> On 08/31/2010 12:10 PM, Ian Campbell wrote:
>>      
>>> On Tue, 2010-08-31 at 11:00 +0100, Michal Novotny wrote:
>>>
>>>        
>>>> I don't know how it's
>>>> working with upstream version since I found out that syntax like `xm
>>>> create -c PVguest` with default settings (pyGrub bootloader) doesn't
>>>> show the pyGrub at all so I don't know what's wrong with my setup. I'm
>>>> using 2.6.32.15-xen kernel/hypervisor version with latest unstable
>>>> user-space tools.
>>>>
>>>> Any hint how this should be working Ian?
>>>>
>>>>          
>>> It should be working as you expect, e.g. "xm create -c xxx" should show
>>> you the pygrub output, unless you have used something like "--entry=x"
>>> or "-q" which disable interactive mode in your bootloader_args.
>>>
>>> I'm afraid I don't know what is broken, I'm reasonably sure it was
>>> working for me when I developed libxl_bootloader.c since I was comparing
>>> the two.
>>>
>>> Ian.
>>>
>>>
>>>        
>> No Ian, it's not working. The config file is having:
>> ...
>> bootloader = "/usr/bin/pygrub"
>> ...
>> So it should show the pyGrub ncurses screen, right?
>>      
> Correct.
>
>    
>>   But it doesn't show
>> anything on version cloned from git yesterday.
>>      
> xen-unstable or xen-4.0-testing or something else?
>
> It looks like this was broken in xen-unstable with 21994:2e08ec0028e4.
> The patch below should fix it.
>
> Ian.
>
> Subject: libxl+xend: use correct paths for PV console when running bootloader
>
> Makes "{xl,xm} create -c GUEST" work again with pygrub in interactive
> mode which was broken by 21994:2e08ec0028e4
>
> Signed-off-by: Ian Campbell<ian.campbell@citrix.com>
> diff -r f77e54fadc18 tools/libxl/libxl_bootloader.c
> --- a/tools/libxl/libxl_bootloader.c	Tue Aug 31 09:54:18 2010 +0100
> +++ b/tools/libxl/libxl_bootloader.c	Tue Aug 31 11:34:20 2010 +0100
> @@ -383,7 +383,7 @@ int libxl_run_bootloader(libxl_ctx *ctx,
>           goto out_close;
>       }
>
> -    dom_console_xs_path = libxl_sprintf(&gc, "%s/serial/0/tty", libxl_xs_get_dompath(&gc, domid));
> +    dom_console_xs_path = libxl_sprintf(&gc, "%s/console/tty", libxl_xs_get_dompath(&gc, domid));
>       libxl_xs_write(&gc, XBT_NULL, dom_console_xs_path, "%s", dom_console_slave_tty_path);
>
>       pid = fork_exec_bootloader(&bootloader_fd, (char *)info->u.pv.bootloader, args);
> diff -r f77e54fadc18 tools/python/xen/util/diagnose.py
> --- a/tools/python/xen/util/diagnose.py	Tue Aug 31 09:54:18 2010 +0100
> +++ b/tools/python/xen/util/diagnose.py	Tue Aug 31 11:34:20 2010 +0100
> @@ -77,7 +77,7 @@ def diagnose_console():
>   def diagnose_console():
>       port    = xstransact.Read(dompath + '/console/port')
>       ringref = xstransact.Read(dompath + '/console/ring-ref')
> -    tty     = xstransact.Read(dompath + '/serial/0/tty')
> +    tty     = xstransact.Read(dompath + '/console/tty')
>
>       if not port:
>           print "Console port is missing; Xend has failed."
> diff -r f77e54fadc18 tools/python/xen/xend/XendBootloader.py
> --- a/tools/python/xen/xend/XendBootloader.py	Tue Aug 31 09:54:18 2010 +0100
> +++ b/tools/python/xen/xend/XendBootloader.py	Tue Aug 31 11:34:20 2010 +0100
> @@ -85,7 +85,7 @@ def bootloader(blexec, disk, dom, quiet
>       fcntl.fcntl(m1, fcntl.F_SETFL, os.O_NDELAY)
>
>       slavename = ptsname.ptsname(m1)
> -    dom.storeDom("serial/0/tty", slavename)
> +    dom.storeDom("console/tty", slavename)
>
>       # Release the domain lock here, because we definitely don't want
>       # a stuck bootloader to deny service to other xend clients.
>
>
>    
Oh, this may be the one. What I was using was staging xen-unstable. I'll 
try to revert this one.

Thanks,
Michal

-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat

Re: [PATCH] Fix bootloader handling when empty string is being output

[Michal Novotny]

On 08/31/2010 12:37 PM, Ian Campbell wrote:
> On Tue, 2010-08-31 at 11:18 +0100, Michal Novotny wrote:
>    
>> On 08/31/2010 12:10 PM, Ian Campbell wrote:
>>      
>>> On Tue, 2010-08-31 at 11:00 +0100, Michal Novotny wrote:
>>>
>>>        
>>>> I don't know how it's
>>>> working with upstream version since I found out that syntax like `xm
>>>> create -c PVguest` with default settings (pyGrub bootloader) doesn't
>>>> show the pyGrub at all so I don't know what's wrong with my setup. I'm
>>>> using 2.6.32.15-xen kernel/hypervisor version with latest unstable
>>>> user-space tools.
>>>>
>>>> Any hint how this should be working Ian?
>>>>
>>>>          
>>> It should be working as you expect, e.g. "xm create -c xxx" should show
>>> you the pygrub output, unless you have used something like "--entry=x"
>>> or "-q" which disable interactive mode in your bootloader_args.
>>>
>>> I'm afraid I don't know what is broken, I'm reasonably sure it was
>>> working for me when I developed libxl_bootloader.c since I was comparing
>>> the two.
>>>
>>> Ian.
>>>
>>>
>>>        
>> No Ian, it's not working. The config file is having:
>> ...
>> bootloader = "/usr/bin/pygrub"
>> ...
>> So it should show the pyGrub ncurses screen, right?
>>      
> Correct.
>
>    
>>   But it doesn't show
>> anything on version cloned from git yesterday.
>>      
> xen-unstable or xen-4.0-testing or something else?
>
> It looks like this was broken in xen-unstable with 21994:2e08ec0028e4.
> The patch below should fix it.
>
> Ian.
>
> Subject: libxl+xend: use correct paths for PV console when running bootloader
>
> Makes "{xl,xm} create -c GUEST" work again with pygrub in interactive
> mode which was broken by 21994:2e08ec0028e4
>
> Signed-off-by: Ian Campbell<ian.campbell@citrix.com>
> diff -r f77e54fadc18 tools/libxl/libxl_bootloader.c
> --- a/tools/libxl/libxl_bootloader.c	Tue Aug 31 09:54:18 2010 +0100
> +++ b/tools/libxl/libxl_bootloader.c	Tue Aug 31 11:34:20 2010 +0100
> @@ -383,7 +383,7 @@ int libxl_run_bootloader(libxl_ctx *ctx,
>           goto out_close;
>       }
>
> -    dom_console_xs_path = libxl_sprintf(&gc, "%s/serial/0/tty", libxl_xs_get_dompath(&gc, domid));
> +    dom_console_xs_path = libxl_sprintf(&gc, "%s/console/tty", libxl_xs_get_dompath(&gc, domid));
>       libxl_xs_write(&gc, XBT_NULL, dom_console_xs_path, "%s", dom_console_slave_tty_path);
>
>       pid = fork_exec_bootloader(&bootloader_fd, (char *)info->u.pv.bootloader, args);
> diff -r f77e54fadc18 tools/python/xen/util/diagnose.py
> --- a/tools/python/xen/util/diagnose.py	Tue Aug 31 09:54:18 2010 +0100
> +++ b/tools/python/xen/util/diagnose.py	Tue Aug 31 11:34:20 2010 +0100
> @@ -77,7 +77,7 @@ def diagnose_console():
>   def diagnose_console():
>       port    = xstransact.Read(dompath + '/console/port')
>       ringref = xstransact.Read(dompath + '/console/ring-ref')
> -    tty     = xstransact.Read(dompath + '/serial/0/tty')
> +    tty     = xstransact.Read(dompath + '/console/tty')
>
>       if not port:
>           print "Console port is missing; Xend has failed."
> diff -r f77e54fadc18 tools/python/xen/xend/XendBootloader.py
> --- a/tools/python/xen/xend/XendBootloader.py	Tue Aug 31 09:54:18 2010 +0100
> +++ b/tools/python/xen/xend/XendBootloader.py	Tue Aug 31 11:34:20 2010 +0100
> @@ -85,7 +85,7 @@ def bootloader(blexec, disk, dom, quiet
>       fcntl.fcntl(m1, fcntl.F_SETFL, os.O_NDELAY)
>
>       slavename = ptsname.ptsname(m1)
> -    dom.storeDom("serial/0/tty", slavename)
> +    dom.storeDom("console/tty", slavename)
>
>       # Release the domain lock here, because we definitely don't want
>       # a stuck bootloader to deny service to other xend clients.
>
>
>    
Well, the XendBootloader.py part seems to be the one (since I use xend 
with xm, no xl). I'll test it later.

Michal

-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat

Re: [PATCH] Fix bootloader handling when empty string is being output

[Michal Novotny]

On 08/31/2010 12:37 PM, Ian Campbell wrote:
> On Tue, 2010-08-31 at 11:18 +0100, Michal Novotny wrote:
>    
>> On 08/31/2010 12:10 PM, Ian Campbell wrote:
>>      
>>> On Tue, 2010-08-31 at 11:00 +0100, Michal Novotny wrote:
>>>
>>>        
>>>> I don't know how it's
>>>> working with upstream version since I found out that syntax like `xm
>>>> create -c PVguest` with default settings (pyGrub bootloader) doesn't
>>>> show the pyGrub at all so I don't know what's wrong with my setup. I'm
>>>> using 2.6.32.15-xen kernel/hypervisor version with latest unstable
>>>> user-space tools.
>>>>
>>>> Any hint how this should be working Ian?
>>>>
>>>>          
>>> It should be working as you expect, e.g. "xm create -c xxx" should show
>>> you the pygrub output, unless you have used something like "--entry=x"
>>> or "-q" which disable interactive mode in your bootloader_args.
>>>
>>> I'm afraid I don't know what is broken, I'm reasonably sure it was
>>> working for me when I developed libxl_bootloader.c since I was comparing
>>> the two.
>>>
>>> Ian.
>>>
>>>
>>>        
>> No Ian, it's not working. The config file is having:
>> ...
>> bootloader = "/usr/bin/pygrub"
>> ...
>> So it should show the pyGrub ncurses screen, right?
>>      
> Correct.
>
>    
>>   But it doesn't show
>> anything on version cloned from git yesterday.
>>      
> xen-unstable or xen-4.0-testing or something else?
>
> It looks like this was broken in xen-unstable with 21994:2e08ec0028e4.
> The patch below should fix it.
>
> Ian.
>
> Subject: libxl+xend: use correct paths for PV console when running bootloader
>
> Makes "{xl,xm} create -c GUEST" work again with pygrub in interactive
> mode which was broken by 21994:2e08ec0028e4
>
> Signed-off-by: Ian Campbell<ian.campbell@citrix.com>
> diff -r f77e54fadc18 tools/libxl/libxl_bootloader.c
> --- a/tools/libxl/libxl_bootloader.c	Tue Aug 31 09:54:18 2010 +0100
> +++ b/tools/libxl/libxl_bootloader.c	Tue Aug 31 11:34:20 2010 +0100
> @@ -383,7 +383,7 @@ int libxl_run_bootloader(libxl_ctx *ctx,
>           goto out_close;
>       }
>
> -    dom_console_xs_path = libxl_sprintf(&gc, "%s/serial/0/tty", libxl_xs_get_dompath(&gc, domid));
> +    dom_console_xs_path = libxl_sprintf(&gc, "%s/console/tty", libxl_xs_get_dompath(&gc, domid));
>       libxl_xs_write(&gc, XBT_NULL, dom_console_xs_path, "%s", dom_console_slave_tty_path);
>
>       pid = fork_exec_bootloader(&bootloader_fd, (char *)info->u.pv.bootloader, args);
> diff -r f77e54fadc18 tools/python/xen/util/diagnose.py
> --- a/tools/python/xen/util/diagnose.py	Tue Aug 31 09:54:18 2010 +0100
> +++ b/tools/python/xen/util/diagnose.py	Tue Aug 31 11:34:20 2010 +0100
> @@ -77,7 +77,7 @@ def diagnose_console():
>   def diagnose_console():
>       port    = xstransact.Read(dompath + '/console/port')
>       ringref = xstransact.Read(dompath + '/console/ring-ref')
> -    tty     = xstransact.Read(dompath + '/serial/0/tty')
> +    tty     = xstransact.Read(dompath + '/console/tty')
>
>       if not port:
>           print "Console port is missing; Xend has failed."
> diff -r f77e54fadc18 tools/python/xen/xend/XendBootloader.py
> --- a/tools/python/xen/xend/XendBootloader.py	Tue Aug 31 09:54:18 2010 +0100
> +++ b/tools/python/xen/xend/XendBootloader.py	Tue Aug 31 11:34:20 2010 +0100
> @@ -85,7 +85,7 @@ def bootloader(blexec, disk, dom, quiet
>       fcntl.fcntl(m1, fcntl.F_SETFL, os.O_NDELAY)
>
>       slavename = ptsname.ptsname(m1)
> -    dom.storeDom("serial/0/tty", slavename)
> +    dom.storeDom("console/tty", slavename)
>
>       # Release the domain lock here, because we definitely don't want
>       # a stuck bootloader to deny service to other xend clients.
>
>
>    
Thanks Ian! I've tested the patch and it's working fine. You can push it 
to the tree since it helped me with this one and `xm create -c PVguest` 
syntax was working fine when applied.

Michal

-- 
Michal Novotny<minovotn@redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat

Re: [PATCH] Fix bootloader handling when empty string is being output

[Ian Jackson]

Michal Novotny writes ("[Xen-devel] [PATCH] Fix bootloader handling when empty string is being output"):
> this is the patch to fix empty string as the output value
> of the bootloader string. If there is no output then
> xend is being hung indefinitely unless you press Ctrl + C keys
> to trigger SIGINT. This patch fixes the issue by implementing
> timeout to the select() function for reading from pipe and also
> checking for state of the process - i.e. whether the process is
> running or whether it's dead or zombie (because fork() call does
> leave zombies when the process is done but the parent process is
> still running).

This is a rather strange way to go about collecting the exit status
from a process.  What is pygrub's parent in this situation ?  Isn't it
xend ?  In which case xend has no need to grobbling around in /proc,
it can just call waitpid.

Ian.