Re: [patch 0/4] elilo multiboot support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Jeremy Katz wrote:  [Mon Jun 26 2006, 03:21:18PM EDT]
> * dprintf is already included in the standard library as a GNU
> extension, but with a different functionality -- something like
> dbgprintf would be better just to avoid problems there

renamed to dbgPrintf to match the style of the rest of the program.

> * Instead of having the getLineByType2 thing, it would probably be a
> little cleaner to just change the types to be a normal int instead of an
> enum and then be able to determine matches via bitwise operators

I left this an enum with explicit bit values.  So you retain whatever
advantage you gained using an enum, and only lose it when they're OR'd
together for getLineByType.

> In the bigger realm, there seems to be a bug or two lingering.  Running
> the test suite ('make test' from the toplevel) seems to fail for
> x86/x86_64 with the patches applied -- it looks like we're gaining an
> initrd in cases where it's unexpected (copy-default shouldn't be copying
> the initrd, but it looks like it might be).  Also, it looks like kernel
> arguments might be getting lost in the multiboot case.  I'll try to look
> at this a little more later in the afternoon, but given how the past two
> weeks have been "later in the afternoon" could end up being Friday :/

All of the test suite issues are fixed in the patch attached to this
message.

addNewKernel:
- don't allow fall-through for LT_INITRD
- don't corrupt the template, use a copy instead
- don't forget to remove the prefix from the initrd
- when converting a grub multiboot template to a normal entry, use the
  module template lines to preserve kernel parameters

addLineTmpl:
- set val=NULL for a bracketed title to prevent elements[1] being set

isBracketedTitle:
- change order of tests for correctness (not related to a failure)

grubby/test/results/updargs/g3.7:
- remove trailing spaces, they were only there because previous
  versions of grubby didn't quite handle EOL correctly

grubby/test.sh:
- current arch doesn't affect what tests can be run, so run them all

The only test suite issue I didn't fix was related to symlink handling
since I didn't touch that code.  I think the code is attempting to
resolve the symlink and create the temporary file next to the target,
but the implementation is completely bogus at the moment (the output
of readlink() doesn't change depending on cwd).

> Also, it'd be nice to have some test cases to add to the test suite just
> so that we can more easily ensure that minor bugfixes don't cause
> regressions.

Not included in this message, but I'll follow up with these too.

Signed-off-by: Aron Griffis <aron@xxxxxx>

diff -r 57dd3b104446 -r 6964eee46f24 grubby/grubby.c
--- a/grubby/grubby.c	Tue Jun 27 19:09:26 2006 -0400
+++ b/grubby/grubby.c	Wed Jun 28 07:44:20 2006 -0400
@@ -34,7 +34,7 @@
 #define DEBUG 0
 
 #if DEBUG
-#define dbgPrintf(format, args...) printf(format , ## args)
+#define dbgPrintf(format, args...) fprintf(stderr, format , ## args)
 #else
 #define dbgPrintf(format, args...)
 #endif
@@ -89,10 +89,10 @@ struct singleEntry {
 
 /* These defines are (only) used in addNewKernel() */
 #define NEED_KERNEL  (1 << 0)
-#define NEED_INITRD  (1 << 2)
-#define NEED_TITLE   (1 << 3)
-#define NEED_ARGS    (1 << 4)
-#define NEED_MB      (1 << 5)
+#define NEED_INITRD  (1 << 1)
+#define NEED_TITLE   (1 << 2)
+#define NEED_ARGS    (1 << 3)
+#define NEED_MB      (1 << 4)
 
 #define MAIN_DEFAULT	    (1 << 0)
 #define DEFAULT_SAVED       -2
@@ -321,6 +321,7 @@ struct singleEntry * findEntryByPath(str
 				     int * index);
 static int readFile(int fd, char ** bufPtr);
 static void lineInit(struct singleLine * line);
+struct singleLine * lineDup(struct singleLine * line);
 static void lineFree(struct singleLine * line);
 static int lineWrite(FILE * out, struct singleLine * line,
 		     struct configFileInfo * cfi);
@@ -403,7 +404,7 @@ static struct singleLine * getLineByType
 }
 
 static int isBracketedTitle(struct singleLine * line) {
-    if ((*line->elements[0].item == '[') && (line->numElements == 1)) {
+    if (line->numElements == 1 && *line->elements[0].item == '[') {
         int len = strlen(line->elements[0].item);
         if (*(line->elements[0].item + len - 1) == ']') {
             /* FIXME: this is a hack... */
@@ -467,6 +468,25 @@ static void lineInit(struct singleLine *
     line->elements = NULL;
     line->numElements = 0;
     line->next = NULL;
+}
+
+struct singleLine * lineDup(struct singleLine * line) {
+    int i;
+    struct singleLine * newLine = malloc(sizeof(*newLine));
+
+    newLine->indent = strdup(line->indent);
+    newLine->next = NULL;
+    newLine->type = line->type;
+    newLine->numElements = line->numElements;
+    newLine->elements = malloc(sizeof(*newLine->elements) * 
+			       newLine->numElements);
+
+    for (i = 0; i < newLine->numElements; i++) {
+	newLine->elements[i].indent = strdup(line->elements[i].indent);
+	newLine->elements[i].item = strdup(line->elements[i].item);
+    }
+
+    return newLine;
 }
 
 static void lineFree(struct singleLine * line) {
@@ -692,15 +712,8 @@ static struct grubConfig * readConfig(co
 	     * lines came earlier in the template, make sure to use LT_HYPER 
 	     * instead of LT_KERNEL now
 	     */
-	    if (entry->multiboot) {
-		struct singleLine * l;
-		for (l = entry->lines; l; l = l->next) {
-		    if (l->type == LT_MBMODULE) {
-			line->type = LT_HYPER;	/* caught it! */
-			break;
-		    }
-		}
-	    }
+	    if (entry->multiboot)
+		line->type = LT_HYPER;
 
         } else if (line->type == LT_MBMODULE) {
 	    /* go back and fix the LT_KERNEL line to indicate LT_HYPER
@@ -1540,20 +1553,7 @@ struct singleLine * addLineTmpl(struct s
 				struct singleLine * tmplLine,
 				struct singleLine * prevLine,
 				const char * val) {
-    int i;
-    struct singleLine * newLine = malloc(sizeof(*newLine));
-
-    newLine->indent = strdup(tmplLine->indent);
-    newLine->next = NULL;
-    newLine->type = tmplLine->type;
-    newLine->numElements = tmplLine->numElements;
-    newLine->elements = malloc(sizeof(*newLine->elements) * 
-			       newLine->numElements);
-
-    for (i = 0; i < newLine->numElements; i++) {
-	newLine->elements[i].indent = strdup(tmplLine->elements[i].indent);
-	newLine->elements[i].item = strdup(tmplLine->elements[i].item);
-    }
+    struct singleLine * newLine = lineDup(tmplLine);
 
     if (val) {
 	/* override the inherited value with our own.
@@ -1599,8 +1599,8 @@ struct singleLine *  addLine(struct sing
     struct keywordTypes * kw;
     struct singleLine tmpl;
 
-    /* NB: This function shouldn't allocation items on the heap, but rather on
-     * the stack since it calls addLineTmpl which will make copies.
+    /* NB: This function shouldn't allocate items on the heap, rather on the
+     * stack since it calls addLineTmpl which will make copies.
      */
 
     if (type == LT_TITLE && cfi->titleBracketed) {
@@ -1611,6 +1611,7 @@ struct singleLine *  addLine(struct sing
 	tmpl.elements[0].item = alloca(strlen(val)+3);
 	sprintf(tmpl.elements[0].item, "[%s]", val);
 	tmpl.elements[0].indent = "";
+	val = NULL;
     } else {
 	kw = getKeywordByType(type, cfi);
 	if (!kw) abort();
@@ -2199,7 +2200,7 @@ int addNewKernel(struct grubConfig * con
 		 char * newKernelArgs, char * newKernelInitrd,
                  char * newMBKernel, char * newMBKernelArgs) {
     struct singleEntry * new;
-    struct singleLine * newLine = NULL, * tmplLine = NULL;
+    struct singleLine * newLine = NULL, * tmplLine = NULL, * masterLine = NULL;
     int needs;
     char * chptr;
 
@@ -2240,7 +2241,10 @@ int addNewKernel(struct grubConfig * con
     }
 
     if (template) {
-	for (tmplLine = template->lines; tmplLine; tmplLine = tmplLine->next) {
+	for (masterLine = template->lines; 
+	     masterLine && (tmplLine = lineDup(masterLine)); 
+	     lineFree(tmplLine), masterLine = masterLine->next) 
+	{
 	    /* skip comments */
 	    chptr = tmplLine->indent;
 	    while (*chptr && isspace(*chptr)) chptr++;
@@ -2295,23 +2299,10 @@ int addNewKernel(struct grubConfig * con
 
 	    } else if (tmplLine->type == LT_HYPER && 
 		       tmplLine->numElements >= 2) {
-		if (new->multiboot) {
-		    if (needs & NEED_MB) {
-			newLine = addLineTmpl(new, tmplLine, newLine, 
-					      newMBKernel + strlen(prefix));
-			needs &= ~NEED_MB;
-		    }
-		} else if (needs & NEED_KERNEL) {
-		    /* template is multi but new is not,
-		     * insert the kernel where the hypervisor was before
-		     */
-		    tmplLine->type = LT_KERNEL;
-		    free(tmplLine->elements[0].item);
-		    tmplLine->elements[0].item = 
-			strdup(getKeywordByType(LT_KERNEL, config->cfi)->key);
+		if (needs & NEED_MB) {
 		    newLine = addLineTmpl(new, tmplLine, newLine, 
-					  newKernelPath + strlen(prefix));
-		    needs &= ~NEED_KERNEL;
+					  newMBKernel + strlen(prefix));
+		    needs &= ~NEED_MB;
 		}
 
 	    } else if (tmplLine->type == LT_MBMODULE && 
@@ -2325,23 +2316,38 @@ int addNewKernel(struct grubConfig * con
 		    } else if (config->cfi->mbInitRdIsModule &&
 			       (needs & NEED_INITRD)) {
 			newLine = addLineTmpl(new, tmplLine, newLine,
-					      newKernelInitrd);
+					      newKernelInitrd + 
+					      strlen(prefix));
 			needs &= ~NEED_INITRD;
 		    }
+		} else if (needs & NEED_KERNEL) {
+		    /* template is multi but new is not, 
+		     * insert the kernel in the first module slot
+		     */
+		    tmplLine->type = LT_KERNEL;
+		    free(tmplLine->elements[0].item);
+		    tmplLine->elements[0].item = 
+			strdup(getKeywordByType(LT_KERNEL, config->cfi)->key);
+		    newLine = addLineTmpl(new, tmplLine, newLine, 
+					  newKernelPath + strlen(prefix));
+		    needs &= ~NEED_KERNEL;
 		} else if (needs & NEED_INITRD) {
 		    /* template is multi but new is not,
-		     * insert the initrd where the module was before
+		     * insert the initrd in the second module slot
 		     */
-		    newLine = addLine(new, config->cfi, LT_INITRD,
-				      config->secondaryIndent, 
-				      newKernelInitrd + strlen(prefix));
+		    tmplLine->type = LT_INITRD;
+		    free(tmplLine->elements[0].item);
+		    tmplLine->elements[0].item = 
+			strdup(getKeywordByType(LT_INITRD, config->cfi)->key);
+		    newLine = addLineTmpl(new, tmplLine, newLine, 
+					  newKernelInitrd + strlen(prefix));
 		    needs &= ~NEED_INITRD;
 		}
 
 	    } else if (tmplLine->type == LT_INITRD && 
-		       tmplLine->numElements >= 2 && 
-		       (needs & NEED_INITRD)) {
-		if (new->multiboot && !template->multiboot &&
+		       tmplLine->numElements >= 2) {
+		if (needs & NEED_INITRD &&
+		    new->multiboot && !template->multiboot &&
 		    config->cfi->mbInitRdIsModule) {
 		    /* make sure we don't insert the module initrd
 		     * before the module kernel... if we don't do it here,
@@ -2353,7 +2359,7 @@ int addNewKernel(struct grubConfig * con
 					  newKernelInitrd + strlen(prefix));
 			needs &= ~NEED_INITRD;
 		    }
-		} else {
+		} else if (needs & NEED_INITRD) {
 		    newLine = addLineTmpl(new, tmplLine, newLine,
 					  newKernelInitrd + strlen(prefix));
 		    needs &= ~NEED_INITRD;
diff -r 57dd3b104446 -r 6964eee46f24 grubby/test.sh
--- a/grubby/test.sh	Tue Jun 27 19:09:26 2006 -0400
+++ b/grubby/test.sh	Wed Jun 28 07:44:20 2006 -0400
@@ -1,32 +1,4 @@
 #!/bin/bash
-
-ARCH=$(uname -m)
-
-elilotest=""
-lilotest=""
-grubtest=""
-zipltest=""
-yaboottest=""
-
-case "$ARCH" in
-    i?86)
-	lilotest="yes"
-	grubtest="yes"
-	;;
-    x86_64)
-	lilotest="yes"
-	grubtest="yes"
-	;;
-    ppc*)
-	yaboottest="yes"
-	;;
-    s390*)
-	zipltest="yes"
-	;;
-    *)
-	echo "Not running any tests for $ARCH"
-	exit 0
-esac
 
 export MALLOC_CHECK_=2
 
@@ -47,35 +19,16 @@ oneTest () {
 	    echo -n " \"$arg\""
 	done
 	echo ""
-	./grubby $mode --bad-image-okay -c $cfg -o - "$@" | diff -u $correct -; 
+	./grubby $mode --bad-image-okay -c $cfg -o - "$@" | diff -u $correct -
 	RESULT=1
     fi 
 }
 
-liloTest() {
-    if [ -z "$lilotest" ]; then echo "skipping LILO test" ; return; fi
-    oneTest --lilo "$@"
-}
-
-eliloTest() {
-    if [ -z "$elilotest" ]; then echo "skipping ELILO test" ; return; fi
-    oneTest --elilo "$@"
-}
-
-grubTest() {
-    if [ -z "$grubtest" ]; then echo "skipping GRUB test" ; return; fi
-    oneTest --grub "$@"
-}
-
-yabootTest() {
-    if [ -z "$yaboottest" ]; then echo "skipping YABOOT test" ; return; fi
-    oneTest --yaboot "$@"
-}
-
-ziplTest() {
-    if [ -z "$zipltest" ]; then echo "skipping Z/IPL test" ; return; fi
-    oneTest --zipl "$@"
-}
+liloTest()   { oneTest --${FUNCNAME%Test} "$@"; }
+eliloTest()  { oneTest --${FUNCNAME%Test} "$@"; }
+grubTest()   { oneTest --${FUNCNAME%Test} "$@"; }
+yabootTest() { oneTest --${FUNCNAME%Test} "$@"; }
+ziplTest()   { oneTest --${FUNCNAME%Test} "$@"; }
 
 echo "Parse/write comparison..."
 for n in $(cd test; echo grub.[0-9]*); do
@@ -120,7 +73,7 @@ if [ ! -L mytest ]; then
 if [ ! -L mytest ]; then
     echo " failed (not a symlink)"
 fi
-target=$(ls -l mytest | awk '{ print $11 }')
+target=$(readlink mytest)
 if [ "$target" != grub-test ]; then
     echo "  failed (wrong target)"
 fi
diff -r 57dd3b104446 -r 6964eee46f24 grubby/test/results/updargs/g3.7
--- a/grubby/test/results/updargs/g3.7	Tue Jun 27 19:09:26 2006 -0400
+++ b/grubby/test/results/updargs/g3.7	Wed Jun 28 07:44:20 2006 -0400
@@ -3,11 +3,11 @@ splashimage=(hd0,1)/grub/splash.xpm.gz
 splashimage=(hd0,1)/grub/splash.xpm.gz
 title Red Hat Linux (2.4.7-2smp)
 	root (hd0,1)
-	kernel /vmlinuz-2.4.7-2smp 
+	kernel /vmlinuz-2.4.7-2smp
 	initrd /initrd-2.4.7-2smp.img
 title Red Hat Linux-up (2.4.7-2)
 	root (hd0,1)
-	kernel /vmlinuz-2.4.7-2 
+	kernel /vmlinuz-2.4.7-2
 	initrd /initrd-2.4.7-2.img
 title DOS
 	rootnoverify (hd0,0)

Attachment: pgpaZqJJjLQmg.pgp
Description: PGP signature

--

Fedora-xen@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/fedora-xen

[Index of Archives]     [Fedora General]     [Fedora Music]     [Linux Kernel]     [Fedora Desktop]     [Fedora Directory]     [PAM]     [Big List of Linux Books]     [Gimp]     [Yosemite News]

  Powered by Linux