[Fedora-xen] Re: [patch 0/4] elilo multiboot support
Aron Griffis
aron at hp.com
Wed Jun 28 11:59:16 UTC 2006
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 at hp.com>
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)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 191 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/fedora-xen/attachments/20060628/40b050e7/attachment.sig>
More information about the Fedora-xen
mailing list