[PATCH] pygrub: make it work

[PATCH] pygrub: make it work

[Christoph Egger]

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


Hi,

Attached patch makes interactive pygrub working with Linux DomUs
in a NetBSD Dom0.

Please apply this patch to -unstable and 3.3-testing tree.

Patch is based on
http://hg.opensolaris.org/sc/src/xen-gate/devel-unstable-patches/pty-fixes

Solaris changes have been ok'd by Sun (Frank van der Linden).

Signed-off-by: Frank van der Linden <frank.vanderlinden@sun.com>
Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Jochen Polster, Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

[-- Attachment #2: xen_pygrub.diff --]
[-- Type: text/x-diff, Size: 4010 bytes --]

$NetBSD: $
--- python/xen/xend/XendBootloader.py.orig	2009-02-13 11:59:48.000000000 +0000
+++ python/xen/xend/XendBootloader.py
@@ -68,7 +68,21 @@ def bootloader(blexec, disk, dom, quiet 
 
     (m1, s1) = pty.openpty()
-    tty.setraw(m1);
-    fcntl.fcntl(m1, fcntl.F_SETFL, os.O_NDELAY);
-    os.close(s1)
+
+    # On Solaris, the pty master side will get cranky if we try
+    # to write to it while there is no slave. To work around this,
+    # keep the slave descriptor open until we're done. Set it
+    # to raw terminal parameters, otherwise it will echo back
+    # characters, which will confuse the I/O loop below.
+    # Furthermore, a raw master pty device has no terminal
+    # semantics on Solaris, so don't try to set any attributes
+    # for it.
+    if os.uname()[0] != 'SunOS' and os.uname()[0] != 'NetBSD':
+        tty.setraw(m1)
+        os.close(s1)
+    else:
+        tty.setraw(s1)
+
+    fcntl.fcntl(m1, fcntl.F_SETFL, os.O_NDELAY)
+
     slavename = ptsname.ptsname(m1)
     dom.storeDom("console/tty", slavename)
@@ -109,5 +123,9 @@ def bootloader(blexec, disk, dom, quiet 
     dom.bootloader_pid = child
 
-    tty.setraw(m2);
+    # On Solaris, the master pty side does not have terminal semantics,
+    # so don't try to set any attributes, as it will fail.
+    if os.uname()[0] != 'SunOS':
+        tty.setraw(m2);
+
     fcntl.fcntl(m2, fcntl.F_SETFL, os.O_NDELAY);
     while True:
@@ -118,15 +136,38 @@ def bootloader(blexec, disk, dom, quiet 
                 continue
         break
+
+    fcntl.fcntl(r, fcntl.F_SETFL, os.O_NDELAY);
+
     ret = ""
     inbuf=""; outbuf="";
+    # filedescriptors:
+    #   r - input from the bootloader (bootstring output)
+    #   m1 - input/output from/to xenconsole
+    #   m2 - input/output from/to pty that controls the bootloader
+    # The filedescriptors are NDELAY, so it's ok to try to read
+    # bigger chunks than may be available, to keep e.g. curses
+    # screen redraws in the bootloader efficient. m1 is the side that
+    # gets xenconsole input, which will be keystrokes, so a small number
+    # is sufficient. m2 is pygrub output, which will be curses screen
+    # updates, so a larger number (1024) is appropriate there.
+    #
+    # For writeable descriptors, only include them in the set for select
+    # if there is actual data to write, otherwise this would loop too fast,
+    # eating up CPU time.
+
     while True:
-        sel = select.select([r, m1, m2], [m1, m2], [])
+        wsel = []
+        if len(outbuf) != 0:
+            wsel = wsel + [m1]
+        if len(inbuf) != 0:
+            wsel = wsel + [m2]
+        sel = select.select([r, m1, m2], wsel, [])
         try: 
             if m1 in sel[0]:
-                s = os.read(m1, 1)
+                s = os.read(m1, 16)
                 inbuf += s
-            if m2 in sel[1] and len(inbuf) != 0:
-                os.write(m2, inbuf[0])
-                inbuf = inbuf[1:]
+            if m2 in sel[1]:
+                n = os.write(m2, inbuf)
+                inbuf = inbuf[n:]
         except OSError, e:
             if e.errno == errno.EIO:
@@ -134,14 +175,14 @@ def bootloader(blexec, disk, dom, quiet 
         try:
             if m2 in sel[0]:
-                s = os.read(m2, 1)
+                s = os.read(m2, 1024)
                 outbuf += s
-            if m1 in sel[1] and len(outbuf) != 0:
-                os.write(m1, outbuf[0])
-                outbuf = outbuf[1:]
+            if m1 in sel[1]:
+                n = os.write(m1, outbuf)
+                outbuf = outbuf[n:]
         except OSError, e:
             if e.errno == errno.EIO:
                 pass
         if r in sel[0]:
-            s = os.read(r, 1)
+            s = os.read(r, 128)
             ret = ret + s
             if len(s) == 0:
@@ -153,4 +194,6 @@ def bootloader(blexec, disk, dom, quiet 
     os.close(m2)
     os.close(m1)
+    if os.uname()[0] == 'SunOS' or os.uname()[0] == 'NetBSD':
+        os.close(s1)
     os.unlink(fifo)
 

[-- 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] pygrub: make it work

[Ian Jackson]

Christoph Egger writes ("[Xen-devel] [PATCH] pygrub: make it work"):
> -    tty.setraw(m1);
> -    fcntl.fcntl(m1, fcntl.F_SETFL, os.O_NDELAY);
> -    os.close(s1)
> +
> +    # On Solaris, the pty master side will get cranky if we try
> +    # to write to it while there is no slave. To work around this,
> +    # keep the slave descriptor open until we're done. Set it
> +    # to raw terminal parameters, otherwise it will echo back
> +    # characters, which will confuse the I/O loop below.
> +    # Furthermore, a raw master pty device has no terminal
> +    # semantics on Solaris, so don't try to set any attributes
> +    # for it.
> +    if os.uname()[0] != 'SunOS' and os.uname()[0] != 'NetBSD':

Surely this dependence on uname is wrong.  Why would set the termios
flags on the pty master ?  They should be set on the slave
unconditionally.

And keeping the slave fd around for a while is harmless too.  So I
guess I'm asking why these changes need to be conditional.

> +    # filedescriptors:
> +    #   r - input from the bootloader (bootstring output)
> +    #   m1 - input/output from/to xenconsole
> +    #   m2 - input/output from/to pty that controls the bootloader

Can you explain what the purpose of this new code is or relevantly how
it differs from the old (ie, what was wrong with the old code) ?

Ian.

Re: [PATCH] pygrub: make it work

[Frank van der Linden]

Ian Jackson wrote:

> Surely this dependence on uname is wrong.  Why would set the termios
> flags on the pty master ?  They should be set on the slave
> unconditionally.

It's been a while since I originally did this patch, so my memory is a 
little rusty, but as far as I remember, I simply wanted to avoid 
breaking Linux after an earlier version of the patch did so. I tested 
the final version of the patch on Linux, and it worked fine. It could be 
that the uname tests aren't needed anymore.

> 
> And keeping the slave fd around for a while is harmless too.  So I
> guess I'm asking why these changes need to be conditional.
> 
>> +    # filedescriptors:
>> +    #   r - input from the bootloader (bootstring output)
>> +    #   m1 - input/output from/to xenconsole
>> +    #   m2 - input/output from/to pty that controls the bootloader
> 
> Can you explain what the purpose of this new code is or relevantly how
> it differs from the old (ie, what was wrong with the old code) ?

The comments I added explain it:

+    # The filedescriptors are NDELAY, so it's ok to try to read
+    # bigger chunks than may be available, to keep e.g. curses
+    # screen redraws in the bootloader efficient. m1 is the side that
+    # gets xenconsole input, which will be keystrokes, so a small number
+    # is sufficient. m2 is pygrub output, which will be curses screen
+    # updates, so a larger number (1024) is appropriate there.
+    #
+    # For writeable descriptors, only include them in the set for select
+    # if there is actual data to write, otherwise this would loop too fast,
+    # eating up CPU time.


- Frank

Re: [PATCH] pygrub: make it work

[Ian Jackson]

Frank van der Linden writes ("Re: [Xen-devel] [PATCH] pygrub: make it work"):
> It's been a while since I originally did this patch, so my memory is a 
> little rusty, but as far as I remember, I simply wanted to avoid 
> breaking Linux after an earlier version of the patch did so. I tested 
> the final version of the patch on Linux, and it worked fine. It could be 
> that the uname tests aren't needed anymore.

I think it would be better to have something that is the same
everywhere so I'd encourage you to resubmit without the special
casing (and I'll help fix it if it goes wrong).

Although Keir may disagree :-).

> Ian Jackson wrote:
> > Can you explain what the purpose of this new code is or relevantly how
> > it differs from the old (ie, what was wrong with the old code) ?
> 
> The comments I added explain it:
> 
> +    # The filedescriptors are NDELAY, so it's ok to try to read

Yes, that's a good explanation of the new code.  What seems to be
lacking is an explanation of what was wrong with the old code - ie,
what should go in the changeset comment.

Ian.

Re: [PATCH] pygrub: make it work

[Frank van der Linden]

Ian Jackson wrote:
> 
> I think it would be better to have something that is the same
> everywhere so I'd encourage you to resubmit without the special
> casing (and I'll help fix it if it goes wrong).

I don't have much time right now to test on Linux, but I'll see if I 
can. Maybe Christoph can too.

> Yes, that's a good explanation of the new code.  What seems to be
> lacking is an explanation of what was wrong with the old code - ie,
> what should go in the changeset comment.
>

The comments do explain what was wrong with the old code..

The old code had the following problems:

1) It was reading and writing data one byte at the time, leading
    to overhead. However, it's safe to try to read/write bigger
    chunks, since the filedescriptors are NDELAY.
2) Instead of checking len(outbuf) / len(inbuf) for writes,
    simply do not include those filedescriptors in the select if there is
    nothing to write. Otherwise, the loop would run around in circles:
    the select would find that the output filedescriptors were writeable,
    but then the loop code would find that it had nothing to write to
    them. In the next iteration, the write filedescs would still be
    writeable, so select returns immediately, but there would still be no
    data to write. Etc, etc. It was essentially a while (1) until
    there was actual data to write. A waste of CPU cycles.

- Frank

Re: [PATCH] pygrub: make it work

[Ian Jackson]

Frank van der Linden writes ("Re: [Xen-devel] [PATCH] pygrub: make it work"):
> Ian Jackson wrote:
> > I think it would be better to have something that is the same
> > everywhere so I'd encourage you to resubmit without the special
> > casing (and I'll help fix it if it goes wrong).
> 
> I don't have much time right now to test on Linux, but I'll see if I 
> can. Maybe Christoph can too.

No, I mean, please resubmit the patch without test for Linux.  We
should apply it.  The changes you quote look sensible to me.  If they
break for someone I will help them debug it or do it myself.

> The comments do explain what was wrong with the old code..
> The old code had the following problems:

The explanation you just gave was what I was looking for.  In general
I think there is a difference between explaining the code as it
currently is and explaining what was wrong with the old code.

The former should be done in comments (if and only if it is not
sufficiently clear from the code itself), and the latter should be
done in checkin comments.

Thanks,
Ian.