[PATCH] xen-booloader: pygrub improvement & bug fix

[PATCH] xen-booloader: pygrub improvement & bug fix

[aq]

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

hello,

here is a patch to improve and fix few bugs in pygrub of xen-booloader. 

1) If you already patched my last 2 patches on top of last Jeremy's
patch, then go on to apply pygrub.aq.patch.

# diffstat pygrub.aq.patch 
 pygrub |   53 +++++++++++++++++++++++++++--------------------------
 1 files changed, 27 insertions(+), 26 deletions(-)

2) If you havent applied any patches on top of last Jeremy's patch,
the take pygrub.aq2.patch instead (this patch includes fix on
/boot/grub/menu.lst problem).

# diffstat pygrub.aq2.patch 
 pygrub |   63 ++++++++++++++++++++++++++++++++++++---------------------------
 1 files changed, 36 insertions(+), 27 deletions(-)

List of changes:
- temporarily remove usage of use_default_colors(), since python 2.3
doesnt support this method.
- allow user to press 'q' to quit pygrub
- corretly handle timeout feature
- fix bug on fill_entries (incorrect check & display menu items)
- incorrect check on boot entry (idx variable)
- remove abundant checking on disk image
- deinitialize curses before quitting.
- fix few typos

Signed-off-by: Nguyen Anh Quynh <aquynh@gmail.com>

[-- Attachment #2: pygrub.aq.patch --]
[-- Type: application/octet-stream, Size: 5277 bytes --]

--- pygrub.org/src/pygrub	2005-04-29 00:07:31.000000000 +0900
+++ pygrub/src/pygrub	2005-04-29 12:36:18.937028000 +0900
@@ -16,7 +16,7 @@
 import os, sys, string, struct, tempfile
 import logging
 
-import curses, _curses, curses.wrapper
+import curses, curses.wrapper
 import getopt
 
 sys.path = [ '/usr/lib/python' ] + sys.path
@@ -29,7 +29,7 @@ PYGRUB_VER = 0.02
 
 def draw_window():
     stdscr = curses.initscr()
-    curses.use_default_colors()
+    # curses.use_default_colors()
     try:
         curses.curs_set(0)
     except _curses.error:
@@ -45,6 +45,7 @@ def draw_window():
     stdscr.addstr(13, 5, "Press enter to boot the selected OS. 'e' to edit the")
     stdscr.addstr(14, 5, "commands before booting, 'a' to modify the kernel arguments ")
     stdscr.addstr(15, 5, "before booting, or 'c' for a command line.")
+    stdscr.addstr(16, 5, "Press 'q' to quit.")
     stdscr.addch(12, 13, curses.ACS_UARROW)
     stdscr.addch(12, 19, curses.ACS_DARROW)
     (y, x) = stdscr.getmaxyx()
@@ -57,13 +58,13 @@ def fill_entries(win, cfg, selected):
     y = 0
 
     for i in cfg.images:
-        if (0, y) > win.getmaxyx():
+        if y > win.getmaxyx()[0]:
             break
         if y == selected:
             attr = curses.A_REVERSE
         else:
             attr = 0
-        win.addstr(y + 1, 2, i.title.ljust(70), attr)
+        win.addstr(y + 1, 2, i.title.ljust(70)[:70], attr)
         y += 1
     win.refresh()
 
@@ -77,7 +78,7 @@ def is_disk_image(file):
     buf = os.read(fd, 512)
     os.close(fd)
 
-    if len(buf) >= 512 and struct.unpack("H", buf[0x1fe: 0x200]) == (0xaaff):
+    if len(buf) == 512 and struct.unpack("H", buf[0x1fe: 0x200]) == (0xaaff):
         return True
     return False
 
@@ -88,7 +89,7 @@ def get_config(fn):
     cf = grub.GrubConf.GrubConfigFile()
 
     if is_disk_image(fn):
-        raise RuntimeError, "appears to be a full disk image... unable to handle this yet"
+        raise RuntimeError, "Appears to be a full disk image... unable to handle this yet"
 
     # open the image and read the grub config
     fs = None
@@ -103,7 +104,7 @@ def get_config(fn):
         elif fs.file_exist("/boot/grub/grub.conf"):
             grubfile = "/boot/grub/grub.conf"
         else:
-            raise RuntimeError, "we couldn't find /boot/grub{menu.lst,grub.conf} " + \
+            raise RuntimeError, "We couldn't find /boot/grub/{menu.lst,grub.conf} " + \
                                 "in the image provided. halt!"
         f = fs.open_file(grubfile)
         buf = f.read()
@@ -149,11 +150,12 @@ def main(cf = None):
             
         fill_entries(win, cf, selected)
         c = stdscr.getch()
-        if mytime != -1:
-            mytime += 1
-#        if c == ord('q'):
-#            selected = -1
-#            break
+        if (c == -1):
+            if mytime != -1:
+                mytime += 1
+        if c == ord('q'):
+           selected = -1
+           break
         elif c == ord('c'):
             # FIXME: needs to go to command line mode
             continue
@@ -180,14 +182,14 @@ def main(cf = None):
         elif selected >= len(cf.images):
             selected = len(cf.images) - 1
 
-    if selected >= 0:
-        return selected
+    return selected
 
 if __name__ == "__main__":
     sel = None
     
     def run_main(scr, *args):
         global sel
+        global cf
         sel = main(cf)
 
     def usage():
@@ -229,13 +231,16 @@ if __name__ == "__main__":
     cf = get_config(file)
     if interactive:
         curses.wrapper(run_main)
+        if sel == -1:   # user has chosen to quit
+            curses.endwin()
+            sys.exit(1)
     else:
         sel = cf.default
 
     # set the entry to boot as requested
     if entry is not None:
         idx = get_entry_idx(cf, entry)
-        if idx is not None and idx > 0 and idx < len(cf.images):
+        if idx is not None and idx >= 0 and idx < len(cf.images):
             sel = idx
 
     img = cf.images[sel]
@@ -244,35 +249,31 @@ if __name__ == "__main__":
     if img.initrd:
         print "  initrd: %s" %(img.initrd[1],)
 
-    if is_disk_image(file):
-        raise RuntimeError, "unable to handle full disk images yet"
-
     # read the kernel and initrd onto the hostfs
     fs = None
     for fstype in grub.fsys.fstypes.values():
         if fstype.sniff_magic(file):
             fs = fstype.open_fs(file)
             break
-
-    if fs is None:
-        raise RuntimeError, "Unable to open filesystem"
+    else:
+        raise RuntimeError, "Unable to open filesystem or filesystem not supported"
 
     kernel = fs.open_file(img.kernel[1],).read()
     (tfd, fn) = tempfile.mkstemp(prefix="vmlinuz.")
     os.write(tfd, kernel)
     os.close(tfd)
-    sxp = "linux (kernel %s)" %(fn,)
+    sxp = "linux (kernel %s) " %(fn,)
 
     if img.initrd:
         initrd = fs.open_file(img.initrd[1],).read()
         (tfd, fn) = tempfile.mkstemp(prefix="initrd.")
         os.write(tfd, initrd)
         os.close(tfd)
-        sxp += "(ramdisk %s)" %(fn,)
-    else:
-        initrd = None
-    sxp += "(args '%s')" %(img.args,)
+        sxp += "(ramdisk %s) " %(fn,)
+
+    sxp += "(args '%s')\n" %(img.args,)
 
     sys.stdout.flush()
     os.write(fd, sxp)
     
+    curses.endwin()

[-- Attachment #3: pygrub.aq2.patch --]
[-- Type: application/octet-stream, Size: 5567 bytes --]

--- unstable.27.3.org/tools/pygrub/src/pygrub	2005-04-27 17:53:05.000000000 +0900
+++ unstable.27.3/tools/pygrub/src/pygrub	2005-04-29 12:36:18.937028000 +0900
@@ -16,9 +16,11 @@
 import os, sys, string, struct, tempfile
 import logging
 
-import curses, _curses, curses.wrapper
+import curses, curses.wrapper
 import getopt
 
+sys.path = [ '/usr/lib/python' ] + sys.path
+
 import grub.GrubConf
 import grub.fsys
 
@@ -27,7 +29,7 @@ PYGRUB_VER = 0.02
 
 def draw_window():
     stdscr = curses.initscr()
-    curses.use_default_colors()
+    # curses.use_default_colors()
     try:
         curses.curs_set(0)
     except _curses.error:
@@ -43,6 +45,7 @@ def draw_window():
     stdscr.addstr(13, 5, "Press enter to boot the selected OS. 'e' to edit the")
     stdscr.addstr(14, 5, "commands before booting, 'a' to modify the kernel arguments ")
     stdscr.addstr(15, 5, "before booting, or 'c' for a command line.")
+    stdscr.addstr(16, 5, "Press 'q' to quit.")
     stdscr.addch(12, 13, curses.ACS_UARROW)
     stdscr.addch(12, 19, curses.ACS_DARROW)
     (y, x) = stdscr.getmaxyx()
@@ -55,13 +58,13 @@ def fill_entries(win, cfg, selected):
     y = 0
 
     for i in cfg.images:
-        if (0, y) > win.getmaxyx():
+        if y > win.getmaxyx()[0]:
             break
         if y == selected:
             attr = curses.A_REVERSE
         else:
             attr = 0
-        win.addstr(y + 1, 2, i.title.ljust(70), attr)
+        win.addstr(y + 1, 2, i.title.ljust(70)[:70], attr)
         y += 1
     win.refresh()
 
@@ -75,10 +78,9 @@ def is_disk_image(file):
     buf = os.read(fd, 512)
     os.close(fd)
 
-    if len(buf) >= 512 and struct.unpack("H", buf[0x1fe: 0x200]) == (0xaaff):
+    if len(buf) == 512 and struct.unpack("H", buf[0x1fe: 0x200]) == (0xaaff):
         return True
     return False
-    
 
 def get_config(fn):
     if not os.access(fn, os.R_OK):
@@ -87,7 +89,7 @@ def get_config(fn):
     cf = grub.GrubConf.GrubConfigFile()
 
     if is_disk_image(fn):
-        raise RuntimeError, "appears to be a full disk image... unable to handle this yet"
+        raise RuntimeError, "Appears to be a full disk image... unable to handle this yet"
 
     # open the image and read the grub config
     fs = None
@@ -97,7 +99,14 @@ def get_config(fn):
             break
 
     if fs is not None:
-        f = fs.open_file("/boot/grub/grub.conf")
+        if fs.file_exist("/boot/grub/menu.lst"):
+            grubfile = "/boot/grub/menu.lst"
+        elif fs.file_exist("/boot/grub/grub.conf"):
+            grubfile = "/boot/grub/grub.conf"
+        else:
+            raise RuntimeError, "We couldn't find /boot/grub/{menu.lst,grub.conf} " + \
+                                "in the image provided. halt!"
+        f = fs.open_file(grubfile)
         buf = f.read()
         f.close()
         fs.close()
@@ -141,11 +150,12 @@ def main(cf = None):
             
         fill_entries(win, cf, selected)
         c = stdscr.getch()
-        if mytime != -1:
-            mytime += 1
-#        if c == ord('q'):
-#            selected = -1
-#            break
+        if (c == -1):
+            if mytime != -1:
+                mytime += 1
+        if c == ord('q'):
+           selected = -1
+           break
         elif c == ord('c'):
             # FIXME: needs to go to command line mode
             continue
@@ -172,14 +182,14 @@ def main(cf = None):
         elif selected >= len(cf.images):
             selected = len(cf.images) - 1
 
-    if selected >= 0:
-        return selected
+    return selected
 
 if __name__ == "__main__":
     sel = None
     
     def run_main(scr, *args):
         global sel
+        global cf
         sel = main(cf)
 
     def usage():
@@ -221,13 +231,16 @@ if __name__ == "__main__":
     cf = get_config(file)
     if interactive:
         curses.wrapper(run_main)
+        if sel == -1:   # user has chosen to quit
+            curses.endwin()
+            sys.exit(1)
     else:
         sel = cf.default
 
     # set the entry to boot as requested
     if entry is not None:
         idx = get_entry_idx(cf, entry)
-        if idx is not None and idx > 0 and idx < len(cf.images):
+        if idx is not None and idx >= 0 and idx < len(cf.images):
             sel = idx
 
     img = cf.images[sel]
@@ -236,35 +249,31 @@ if __name__ == "__main__":
     if img.initrd:
         print "  initrd: %s" %(img.initrd[1],)
 
-    if is_disk_image(file):
-        raise RuntimeError, "unable to handle full disk images yet"
-
     # read the kernel and initrd onto the hostfs
     fs = None
     for fstype in grub.fsys.fstypes.values():
         if fstype.sniff_magic(file):
             fs = fstype.open_fs(file)
             break
-
-    if fs is None:
-        raise RuntimeError, "Unable to open filesystem"
+    else:
+        raise RuntimeError, "Unable to open filesystem or filesystem not supported"
 
     kernel = fs.open_file(img.kernel[1],).read()
     (tfd, fn) = tempfile.mkstemp(prefix="vmlinuz.")
     os.write(tfd, kernel)
     os.close(tfd)
-    sxp = "linux (kernel %s)" %(fn,)
+    sxp = "linux (kernel %s) " %(fn,)
 
     if img.initrd:
         initrd = fs.open_file(img.initrd[1],).read()
         (tfd, fn) = tempfile.mkstemp(prefix="initrd.")
         os.write(tfd, initrd)
         os.close(tfd)
-        sxp += "(ramdisk %s)" %(fn,)
-    else:
-        initrd = None
-    sxp += "(args '%s')" %(img.args,)
+        sxp += "(ramdisk %s) " %(fn,)
+
+    sxp += "(args '%s')\n" %(img.args,)
 
     sys.stdout.flush()
     os.write(fd, sxp)
     
+    curses.endwin()

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

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

Re: [PATCH] xen-booloader: pygrub improvement & bug fix

[Jeremy Katz]

On Fri, 2005-04-29 at 12:56 +0900, aq wrote:
> here is a patch to improve and fix few bugs in pygrub of xen-booloader. 

Cool, thanks.  Comments below

> List of changes:
> - temporarily remove usage of use_default_colors(), since python 2.3
> doesnt support this method.

It would be better to actually check for the method and call it if
available.. something like
  if hasattr(curses, 'use_default_colors') curses.use_default_colors()

> - allow user to press 'q' to quit pygrub

I had this at one point and then removed it -- the question is if you
quit, then what are you wanting to boot?  I guess it would be aborting
the domain boot (which is the result you get), but thinking further down
the road, what does that mean on a reboot?  So I just decided the best
thing to do was not to allow this.  

> - deinitialize curses before quitting.

curses should get de-initialized when you leave curses.wrapper -- are
you not seeing this?

Thanks,

Jeremy

Re: [PATCH] xen-booloader: pygrub improvement & bug fix

[aq]

On 4/30/05, Jeremy Katz <katzj@redhat.com> wrote:
> On Fri, 2005-04-29 at 12:56 +0900, aq wrote:
> > here is a patch to improve and fix few bugs in pygrub of xen-booloader.
> 
> Cool, thanks.  Comments below
> 
> > List of changes:
> > - temporarily remove usage of use_default_colors(), since python 2.3
> > doesnt support this method.
> 
> It would be better to actually check for the method and call it if
> available.. something like
>   if hasattr(curses, 'use_default_colors') curses.use_default_colors()

fine. but anyway i am implementing color feature, so perhaps we dont
need those default color.

> 
> > - allow user to press 'q' to quit pygrub
> 
> I had this at one point and then removed it -- the question is if you
> quit, then what are you wanting to boot?  I guess it would be aborting
> the domain boot (which is the result you get), but thinking further down
> the road, what does that mean on a reboot?  So I just decided the best
> thing to do was not to allow this.

for me, it is nice to allow pepole to cancel the tool. without this
option, how can you allow people to quit once they dont want to run
anymore? so i think it is fine to keep it that way.
 
> 
> > - deinitialize curses before quitting.
> 
> curses should get de-initialized when you leave curses.wrapper -- are
> you not seeing this?

right, thanks for pointing out this.

by the way, pygrub is an ugly name. the fact that this tool is written
in python is what users care least. how about renamming it to a more
make-sense name? like xengrub, or (even better) xenloader?

regards,
aq