[RFC] SuSE and ppc64/yaboot support

[Cleber Rosa]

This is an RFC for adding support for SuSE systems (running Grub) and other
Linux systems running on Power with yaboot (tested on RHEL 6).

Patch #3 is really a placeholder and it's the one I'm mostly interested
in getting feedback. The reasoning is that current grub2 checks are
really simplistic, and I'd like to know whether the same thing should be
OK for other bootloaders. In the inline comments are a proposal for a real
check of yaboot installation.

Series summary:

[PATCH 1/3] RFC: Fix grub detection on SuSE systems: lba and boot
[PATCH 2/3] RFC: Fix grub detection on SuSE systems: config file
[PATCH 3/3] RFC: ppc64/yaboot: add support for probing the currently

[PATCH 1/3] RFC: Fix grub detection on SuSE systems: lba and boot information

[Cleber Rosa]

This patch adds code that gets lba and boot device information on
SuSE systems, which is done by looking at the commands that were
used to install grub on that system, and match information with
grub's device.map file.

Signed-off-by: Cleber Rosa 
---
 grubby.c |  238 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 232 insertions(+), 6 deletions(-)

diff --git a/grubby.c b/grubby.c
index 601ba8d..c664387 100644
--- a/grubby.c
+++ b/grubby.c
@@ -56,6 +56,16 @@ int debug = 0;	/* Currently just for template debugging */
 #define NOOP_OPCODE 0x90
 #define JMP_SHORT_OPCODE 0xeb

+/*
+ * This SuSE grub configuration file at this location is not your average
+ * grub configuration file, but instead the grub commands used to setup
+ * grub on that system.
+ */
+#define SUSE_GRUB_CONF "/etc/grub.conf"
+#define SUSE_RELEASE_FILE "/etc/SuSE-release"
+#define GRUB_DEVICE_MAP "/boot/grub/device.map"
+
+
 /* comments get lumped in with indention */
 struct lineElement {
     char * item;
@@ -1938,6 +1948,202 @@ void displayEntry(struct singleEntry * entry, const char * prefix, int index) {
     }
 }

+int isSuseGrubConf(const char * path) {
+    FILE * grubConf;
+    char * line = NULL;
+    size_t len = 0;
+
+    grubConf = fopen(path, "r");
+    if (!grubConf) {
+        dbgPrintf("Could not open SuSE configuration file '%s'\n",
+		  SUSE_GRUB_CONF);
+	return 1;
+    }
+
+    if (getline(&line, &len, grubConf) == -1) {
+	dbgPrintf("Could not read line from file '%s'\n",
+		  SUSE_GRUB_CONF);
+        return 1;
+    }
+
+    if (strncmp(line, "setup", 5)) {
+	dbgPrintf("SuSE configuration file '%s' does not appear to be valid\n",
+		  SUSE_GRUB_CONF);
+	return 1;
+    }
+
+    free(line);
+    fclose(grubConf);
+    return 0;
+}
+
+int suseGrubConfGetLba(const char * path, int * lbaPtr) {
+    FILE * grubConf;
+    char * line = NULL;
+    size_t len = 0;
+
+    if (!path) return 1;
+    if (!lbaPtr) return 1;
+
+    grubConf = fopen(path, "r");
+    if (!grubConf) return 1;
+
+    while (getline(&line, &len, grubConf) != -1) {
+        if (!strncmp(line, "setup", 5)) {
+	    if (strstr(line, "--force-lba")) {
+	        *lbaPtr = 1;
+	    } else {
+	        *lbaPtr = 0;
+	    }
+	    dbgPrintf("lba: %i\n", *lbaPtr);
+	    break;
+	}
+    }
+
+    free(line);
+    fclose(grubConf);
+    return 0;
+}
+
+int suseGrubConfGetInstallDevice(const char * path, char ** devicePtr) {
+    FILE * grubConf;
+    char * line = NULL;
+    size_t len = 0;
+    char * lastParamPtr = NULL;
+    char * secLastParamPtr = NULL;
+    char installDeviceNumber = '\0';
+
+    if (!path) return 1;
+    if (!devicePtr) return 1;
+
+    grubConf = fopen(path, "r");
+    if (!grubConf) return 1;
+
+    while (getline(&line, &len, grubConf) != -1) {
+      /*
+       * If this is a line with a valid setup command, it will contain
+       * the device grub will be installed to.
+       */
+      if (!strncmp(line, "setup", 5)) {
+
+	  lastParamPtr = line + strlen(line);
+
+	  if (*--lastParamPtr == '\n')
+	    *lastParamPtr = '\0';
+
+	  /* Last parameter in grub may be an optional IMAGE_DEVICE */
+	  while (!isspace(*lastParamPtr))
+	    lastParamPtr--;
+	  lastParamPtr++;
+
+	  secLastParamPtr = lastParamPtr - 2;
+	  dbgPrintf("lastParamPtr: %s\n", lastParamPtr);
+	  if (!strncmp(lastParamPtr, "(hd", 3))
+	    lastParamPtr += 3;
+	  dbgPrintf("lastParamPtr: %c\n", *lastParamPtr);
+
+	  /*
+	   * Second last parameter will point out if last parameter is
+	   * either an IMAGE_DEVICE or INSTALL_DEVICE
+	   */
+	  while (!isspace(*secLastParamPtr))
+	    secLastParamPtr--;
+	  secLastParamPtr++;
+
+	  dbgPrintf("secLastParamPtr: %s\n", secLastParamPtr);
+	  if (!strncmp(secLastParamPtr, "(hd", 3)) {
+	    secLastParamPtr += 3;
+	    dbgPrintf("secLastParamPtr: %c\n", *secLastParamPtr);
+
+	    installDeviceNumber = *secLastParamPtr;
+	  } else {
+	    installDeviceNumber = *lastParamPtr;
+	  }
+
+	  *devicePtr = malloc(6);
+	  snprintf(*devicePtr, 6, "(hd%c)", installDeviceNumber);
+	  dbgPrintf("installDeviceNumber: %c\n", installDeviceNumber);
+	  break;
+      }
+    }
+
+    free(line);
+    fclose(grubConf);
+    return 0;
+}
+
+int grubGetBootFromDeviceMap(const char * path,
+			     const char * device,
+			     char ** bootPtr) {
+    FILE * deviceMap;
+    char * line = NULL;
+    size_t len = 0;
+    char * devicePtr;
+    char * end;
+
+    if (!path) return 1;
+    if (!device) return 1;
+    if (!bootPtr) return 1;
+
+    deviceMap = fopen(path, "r");
+    if (!deviceMap) return 1;
+
+    while (getline(&line, &len, deviceMap) != -1) {
+        if (strncmp(line, "#", 1)) {
+	    devicePtr = line;
+	    while (isspace(*line))
+	        devicePtr++;
+	    dbgPrintf("device: %s\n", devicePtr);
+
+	    if (!strncmp(devicePtr, device, strlen(device))) {
+	    devicePtr += strlen(device);
+	    while (isspace(*devicePtr))
+	        devicePtr++;
+
+	    end = strchr(devicePtr, '\n');
+	    if (end)
+	        *end = '\0';
+
+	    *bootPtr = strdup(devicePtr);
+	    break;
+	  }
+	}
+    }
+
+    free(line);
+    fclose(deviceMap);
+    return 0;
+}
+
+int suseGrubConfGetBoot(const char * path, char ** bootPtr) {
+    char * grubDevice;
+
+    suseGrubConfGetInstallDevice(path, &grubDevice);
+    dbgPrintf("grubby installation device: %s\n", grubDevice);
+
+    grubGetBootFromDeviceMap(GRUB_DEVICE_MAP, grubDevice, bootPtr);
+    dbgPrintf("boot: %s\n", *bootPtr);
+
+    return 0;
+}
+
+int parseSuseGrubConf(int * lbaPtr, char ** bootPtr) {
+    if (isSuseGrubConf(SUSE_GRUB_CONF)) return 1;
+
+    if (lbaPtr) {
+        *lbaPtr = 0;
+        if (suseGrubConfGetLba(SUSE_GRUB_CONF, lbaPtr))
+            return 1;
+    }
+
+    if (bootPtr) {
+        *bootPtr = NULL;
+        suseGrubConfGetBoot(SUSE_GRUB_CONF, bootPtr);
+    }
+
+    return 0;
+}
+
 int parseSysconfigGrub(int * lbaPtr, char ** bootPtr) {
     FILE * in;
     char buf[1024];
@@ -1988,11 +2194,19 @@ int parseSysconfigGrub(int * lbaPtr, char ** bootPtr) {
 void dumpSysconfigGrub(void) {
     char * boot;
     int lba;
+    int notOnSuse;

-    if (!parseSysconfigGrub(&lba, &boot)) {
-	if (lba) printf("lba\n");
-	if (boot) printf("boot=%s\n", boot);
+    notOnSuse = access(SUSE_RELEASE_FILE, R_OK);
+    if (notOnSuse) {
+        if (parseSysconfigGrub(&lba, &boot))
+	  return;
+    } else {
+        if (parseSuseGrubConf(&lba, &boot))
+	  return;
     }
+
+    if (lba) printf("lba\n");
+    if (boot) printf("boot=%s\n", boot);
 }

int displayInfo(struct grubConfig * config, char * kernel,
@@ -2812,9 +3026,14 @@ int checkForGrub(struct grubConfig * config) {
     int fd;
     unsigned char bootSect[512];
     char * boot;
+    int notOnSuse;

-    if (parseSysconfigGrub(NULL, &boot))
-	return 0;
+    notOnSuse = access(SUSE_RELEASE_FILE, R_OK);
+    if (notOnSuse) {
+	if (parseSysconfigGrub(NULL, &boot)) return 0;
+    } else {
+	if (parseSuseGrubConf(NULL, &boot)) return 0;
+    }

/* assume grub is not installed -- not an error condition */
     if (!boot)
@@ -2833,7 +3052,14 @@ int checkForGrub(struct grubConfig * config) {
     }
     close(fd);

-    return checkDeviceBootloader(boot, bootSect);
+    if (notOnSuse)
+      return checkDeviceBootloader(boot, bootSect);
+    else
+      /*
+       * The more elaborate checks do not work on SuSE. The checks done
+       * seem to be reasonble (at least for now), so just return success
+       */
+      return 2;
 }

int checkForExtLinux(struct grubConfig * config) {

[PATCH 1/3] RFC: Fix grub detection on SuSE systems: lba and boot information

[Peter Jones]

On 01/30/2012 03:41 PM, Cleber Rosa wrote:
> This patch adds code that gets lba and boot device information on
> SuSE systems, which is done by looking at the commands that were
> used to install grub on that system, and match information with
> grub's device.map file.
>
> Signed-off-by: Cleber Rosa

In principle, I'm okay with the idea of this patch. In practice, let's talk
about the code inline.
> grubby.c |  238 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>   1 files changed, 232 insertions(+), 6 deletions(-)
>
> diff --git a/grubby.c b/grubby.c
> index 601ba8d..c664387 100644
> --- a/grubby.c
> +++ b/grubby.c
> @@ -56,6 +56,16 @@ int debug = 0;	/* Currently just for template debugging */
>   #define NOOP_OPCODE 0x90
>   #define JMP_SHORT_OPCODE 0xeb
>
> +/*
> This SuSE grub configuration file at this location is not your average
> grub configuration file, but instead the grub commands used to setup
> grub on that system.
> */
>
> +#define SUSE_GRUB_CONF "/etc/grub.conf"
> +#define SUSE_RELEASE_FILE "/etc/SuSE-release"
> +#define GRUB_DEVICE_MAP "/boot/grub/device.map"

I'd rather see whichever of these is truly needed (which I suspect is just
SUSE_GRUB_CONF) defined near the function it gets used in.
>

One newline is fine.
> /* comments get lumped in with indention */
>   struct lineElement {
>       char * item;
> @@ -1938,6 +1948,202 @@ void displayEntry(struct singleEntry * entry, const char * prefix, int index) {
>       }
>   }
>
> +int isSuseGrubConf(const char * path) {
> FILE * grubConf;
> char * line = NULL;
> size_t len = 0;
> grubConf = fopen(path, "r");
> if (!grubConf) {
>    dbgPrintf("Could not open SuSE configuration file '%s'\n",
>   SUSE_GRUB_CONF);

If you're going to rely on the passed in path for opening the file,
use it for the debug messages here too.
> return 1;

I realize there's some inconsistency in the current source tree, but
if a function is "isFoo()", 1 should be truth.
> }
> if (getline(&line,&len, grubConf) == -1) {
> dbgPrintf("Could not read line from file '%s'\n",
>   SUSE_GRUB_CONF);
>    return 1;

This leaks grubConf, as well as using SUSE_GRUB_CONF instead of path.
> }
> if (strncmp(line, "setup", 5)) {
> dbgPrintf("SuSE configuration file '%s' does not appear to be valid\n",
>   SUSE_GRUB_CONF);
> return 1;

This leaks line and grubConf as well as using SUSE_GRUB_CONF instead of path.
> }
> free(line);
> fclose(grubConf);
> return 0;
>
> +}
>
> +int suseGrubConfGetLba(const char * path, int * lbaPtr) {
> FILE * grubConf;
> char * line = NULL;
> size_t len = 0;
> if (!path) return 1;
> if (!lbaPtr) return 1;
> grubConf = fopen(path, "r");
> if (!grubConf) return 1;
> while (getline(&line,&len, grubConf) != -1) {
>    if (!strncmp(line, "setup", 5)) {
>    if (strstr(line, "--force-lba")) {

1) Please indent 4 spaces each time.
2) line is not necessarily null terminated here - strstr() can segfault it.
>        *lbaPtr = 1;
>    } else {
>        *lbaPtr = 0;
>    }
>    dbgPrintf("lba: %i\n", *lbaPtr);
>    break;
> }
> }
> free(line);
> fclose(grubConf);
> return 0;
>
> +}
>
> +int suseGrubConfGetInstallDevice(const char * path, char ** devicePtr) {
> FILE * grubConf;
> char * line = NULL;
> size_t len = 0;
> char * lastParamPtr = NULL;
> char * secLastParamPtr = NULL;
> char installDeviceNumber = '\0';
> if (!path) return 1;
> if (!devicePtr) return 1;
> grubConf = fopen(path, "r");
> if (!grubConf) return 1;
> while (getline(&line,&len, grubConf) != -1) {
>  /*
>   * If this is a line with a valid setup command, it will contain
>   * the device grub will be installed to.
>   */
>  if (!strncmp(line, "setup", 5)) {
>  lastParamPtr = line + strlen(line);
>  if (*--lastParamPtr == '\n')
>    *lastParamPtr = '\0';

getline() doesn't guarantee that the string is terminated, only that it's
of length it dumped into len.  So strlen isn't usable here.  But also you
don't need it - just use len!
>  /* Last parameter in grub may be an optional IMAGE_DEVICE */
>  while (!isspace(*lastParamPtr))
>    lastParamPtr--;
>  lastParamPtr++;
>  secLastParamPtr = lastParamPtr - 2;
>  dbgPrintf("lastParamPtr: %s\n", lastParamPtr);

Again, it hasn't been terminated; you can't give it to printf-like functions.
>  if (!strncmp(lastParamPtr, "(hd", 3))
>    lastParamPtr += 3;

lastParamPtr needs bounds checking here as well - if it's 2 characters long,
this again could segfault.
>  dbgPrintf("lastParamPtr: %c\n", *lastParamPtr);
>  /*
>   * Second last parameter will point out if last parameter is
>   * either an IMAGE_DEVICE or INSTALL_DEVICE
>   */
>  while (!isspace(*secLastParamPtr))
>    secLastParamPtr--;
>  secLastParamPtr++;
>  dbgPrintf("secLastParamPtr: %s\n", secLastParamPtr);
>  if (!strncmp(secLastParamPtr, "(hd", 3)) {
>    secLastParamPtr += 3;
>    dbgPrintf("secLastParamPtr: %c\n", *secLastParamPtr);
>    installDeviceNumber = *secLastParamPtr;
>  } else {
>    installDeviceNumber = *lastParamPtr;
>  }
>  *devicePtr = malloc(6);
>  snprintf(*devicePtr, 6, "(hd%c)", installDeviceNumber);
>  dbgPrintf("installDeviceNumber: %c\n", installDeviceNumber);
>  break;
>  }
> }

This all has those same problems, of course.
> free(line);
> fclose(grubConf);
> return 0;
>
> +}
>
> +int grubGetBootFromDeviceMap(const char * path,
> 	     const char * device,
> 	     char ** bootPtr) {
> FILE * deviceMap;
> char * line = NULL;
> size_t len = 0;
> char * devicePtr;
> char * end;
> if (!path) return 1;
> if (!device) return 1;
> if (!bootPtr) return 1;
> deviceMap = fopen(path, "r");
> if (!deviceMap) return 1;
> while (getline(&line,&len, deviceMap) != -1) {
>    if (strncmp(line, "#", 1)) {
>    devicePtr = line;
>    while (isspace(*line))
>        devicePtr++;

How does this work?  Since line is never advanced, you're going to keep on
finding the same whitespace.  If it were advanced, you'd need to keep check of 
the bounds of "line" here to avoid segfaulting.
>    dbgPrintf("device: %s\n", devicePtr);

This too can segfault.
>    if (!strncmp(devicePtr, device, strlen(device))) {
>    devicePtr += strlen(device);
>    while (isspace(*devicePtr))
>        devicePtr++;

This one segfaults as well.
>    end = strchr(devicePtr, '\n');

if the end of file doesn't have a newline, this is bad, too.
>    if (end)
>        *end = '\0';
>    *bootPtr = strdup(devicePtr);

Need to bounds check here as well.
>    break;
>  }
> }
> }
> free(line);
> fclose(deviceMap);
> return 0;
>
> +}
>
> +int suseGrubConfGetBoot(const char * path, char ** bootPtr) {
> char * grubDevice;
> suseGrubConfGetInstallDevice(path,&grubDevice);
> dbgPrintf("grubby installation device: %s\n", grubDevice);
> grubGetBootFromDeviceMap(GRUB_DEVICE_MAP, grubDevice, bootPtr);
> dbgPrintf("boot: %s\n", *bootPtr);
> return 0;
>
> +}

There's no point in #defining GRUB_DEVICE_MAP if this is the only place it's
going to get used.
> +int parseSuseGrubConf(int * lbaPtr, char ** bootPtr) {
> if (isSuseGrubConf(SUSE_GRUB_CONF)) return 1;
> if (lbaPtr) {
>    *lbaPtr = 0;
>    if (suseGrubConfGetLba(SUSE_GRUB_CONF, lbaPtr))
>        return 1;
> }
> if (bootPtr) {
>    *bootPtr = NULL;
>    suseGrubConfGetBoot(SUSE_GRUB_CONF, bootPtr);
> }
> return 0;
>
> +}

Okay.
> int parseSysconfigGrub(int * lbaPtr, char ** bootPtr) {
>     FILE * in;
>     char buf[1024];
>
> @@ -1988,11 +2194,19 @@ int parseSysconfigGrub(int * lbaPtr, char ** bootPtr) {
>   void dumpSysconfigGrub(void) {
>       char * boot;
>       int lba;
> int notOnSuse;
> if (!parseSysconfigGrub(&lba,&boot)) {
> if (lba) printf("lba\n");
> if (boot) printf("boot=%s\n", boot);
> notOnSuse = access(SUSE_RELEASE_FILE, R_OK);

I'd really rather this was a function, isSuse(), that returns 1 if it is.
> if (notOnSuse) {
>    if (parseSysconfigGrub(&lba,&boot))
>  return;

This needs a value.  Did you build this code?
> } else {
>    if (parseSuseGrubConf(&lba,&boot))
>  return;

Same here.
>   }
>
> if (lba) printf("lba\n");
>
> if (boot) printf("boot=%s\n", boot);
> }
>
> int displayInfo(struct grubConfig * config, char * kernel,
>
> @@ -2812,9 +3026,14 @@ int checkForGrub(struct grubConfig * config) {
>       int fd;
>       unsigned char bootSect[512];
>       char * boot;
> int notOnSuse;
> if (parseSysconfigGrub(NULL,&boot))
> return 0;
>
> notOnSuse = access(SUSE_RELEASE_FILE, R_OK);
>
> if (notOnSuse) {
>
> if (parseSysconfigGrub(NULL,&boot)) return 0;
>
> } else {
>
> if (parseSuseGrubConf(NULL,&boot)) return 0;
>
> }
>
> /* assume grub is not installed -- not an error condition */
>  if (!boot)
>
> @@ -2833,7 +3052,14 @@ int checkForGrub(struct grubConfig * config) {
>       }
>       close(fd);
> return checkDeviceBootloader(boot, bootSect);
>
> if (notOnSuse)
>  return checkDeviceBootloader(boot, bootSect);
>
> else
>  /*
>   * The more elaborate checks do not work on SuSE. The checks done
>   * seem to be reasonble (at least for now), so just return success
>   */
>  return 2;
>
> }
>
> int checkForExtLinux(struct grubConfig * config) {

I'll leave it to you to revise all three of your patches with this feedback in
mind.

Thanks!

[PATCH 1/3] RFC: Fix grub detection on SuSE systems: lba and boot information

[Cleber Rosa]

On 02/27/2012 12:03 PM, Peter Jones wrote:
> On 01/30/2012 03:41 PM, Cleber Rosa wrote:
> > This patch adds code that gets lba and boot device information on
> > SuSE systems, which is done by looking at the commands that were
> > used to install grub on that system, and match information with
> > grub's device.map file.
> >
> > Signed-off-by: Cleber Rosa
>
> In principle, I'm okay with the idea of this patch. In practice, let's 
> talk
> about the code inline.
> > grubby.c |  238 
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> >   1 files changed, 232 insertions(+), 6 deletions(-)
> >
> > diff --git a/grubby.c b/grubby.c
> > index 601ba8d..c664387 100644
> > --- a/grubby.c
> > +++ b/grubby.c
> > @@ -56,6 +56,16 @@ int debug = 0;    /* Currently just for template 
> > debugging */
> >   #define NOOP_OPCODE 0x90
> >   #define JMP_SHORT_OPCODE 0xeb
> >
> > +/*
> > This SuSE grub configuration file at this location is not your
> >
> > average
> > grub configuration file, but instead the grub commands used to setup
> > grub on that system.
> > */
> >
> > +#define SUSE_GRUB_CONF "/etc/grub.conf"
> > +#define SUSE_RELEASE_FILE "/etc/SuSE-release"
> > +#define GRUB_DEVICE_MAP "/boot/grub/device.map"
>
> I'd rather see whichever of these is truly needed (which I suspect is 
> just
> SUSE_GRUB_CONF) defined near the function it gets used in.
> >
>
> One newline is fine.
> > /* comments get lumped in with indention */
> >   struct lineElement {
> >       char * item;
> > @@ -1938,6 +1948,202 @@ void displayEntry(struct singleEntry * entry, 
> > const char * prefix, int index) {
> >       }
> >   }
> >
> > +int isSuseGrubConf(const char * path) {
> > FILE * grubConf;
> > char * line = NULL;
> > size_t len = 0;
> > grubConf = fopen(path, "r");
> > if (!grubConf) {
> >    dbgPrintf("Could not open SuSE configuration file '%s'\n",
> >      SUSE_GRUB_CONF);
>
> If you're going to rely on the passed in path for opening the file,
> use it for the debug messages here too.
> > return 1;
>
> I realize there's some inconsistency in the current source tree, but
> if a function is "isFoo()", 1 should be truth.
> > }
> > if (getline(&line,&len, grubConf) == -1) {
> > dbgPrintf("Could not read line from file '%s'\n",
> >      SUSE_GRUB_CONF);
> >    return 1;
>
> This leaks grubConf, as well as using SUSE_GRUB_CONF instead of path.
> > }
> > if (strncmp(line, "setup", 5)) {
> > dbgPrintf("SuSE configuration file '%s' does not appear to be
> >
> > valid\n",
> >      SUSE_GRUB_CONF);
> > return 1;
>
> This leaks line and grubConf as well as using SUSE_GRUB_CONF instead 
> of path.
> > }
> > free(line);
> > fclose(grubConf);
> > return 0;
> >
> > +}
> >
> > +int suseGrubConfGetLba(const char * path, int * lbaPtr) {
> > FILE * grubConf;
> > char * line = NULL;
> > size_t len = 0;
> > if (!path) return 1;
> > if (!lbaPtr) return 1;
> > grubConf = fopen(path, "r");
> > if (!grubConf) return 1;
> > while (getline(&line,&len, grubConf) != -1) {
> >    if (!strncmp(line, "setup", 5)) {
> >    if (strstr(line, "--force-lba")) {
> Please indent 4 spaces each time.
> line is not necessarily null terminated here - strstr() can
>
> segfault it.
> >        *lbaPtr = 1;
> >    } else {
> >        *lbaPtr = 0;
> >    }
> >    dbgPrintf("lba: %i\n", *lbaPtr);
> >    break;
> > }
> > }
> > free(line);
> > fclose(grubConf);
> > return 0;
> >
> > +}
> >
> > +int suseGrubConfGetInstallDevice(const char * path, char ** 
> > devicePtr) {
> > FILE * grubConf;
> > char * line = NULL;
> > size_t len = 0;
> > char * lastParamPtr = NULL;
> > char * secLastParamPtr = NULL;
> > char installDeviceNumber = '\0';
> > if (!path) return 1;
> > if (!devicePtr) return 1;
> > grubConf = fopen(path, "r");
> > if (!grubConf) return 1;
> > while (getline(&line,&len, grubConf) != -1) {
> >  /*
> >   * If this is a line with a valid setup command, it will contain
> >   * the device grub will be installed to.
> >   */
> >  if (!strncmp(line, "setup", 5)) {
> >  lastParamPtr = line + strlen(line);
> >  if (*--lastParamPtr == '\n')
> >    *lastParamPtr = '\0';
>
> getline() doesn't guarantee that the string is terminated, only that it's
> of length it dumped into len.  So strlen isn't usable here.  But also you
> don't need it - just use len!
> >  /* Last parameter in grub may be an optional IMAGE_DEVICE */
> >  while (!isspace(*lastParamPtr))
> >    lastParamPtr--;
> >  lastParamPtr++;
> >  secLastParamPtr = lastParamPtr - 2;
> >  dbgPrintf("lastParamPtr: %s\n", lastParamPtr);
>
> Again, it hasn't been terminated; you can't give it to printf-like 
> functions.
> >  if (!strncmp(lastParamPtr, "(hd", 3))
> >    lastParamPtr += 3;
>
> lastParamPtr needs bounds checking here as well - if it's 2 characters 
> long,
> this again could segfault.
> >  dbgPrintf("lastParamPtr: %c\n", *lastParamPtr);
> >  /*
> >   * Second last parameter will point out if last parameter is
> >   * either an IMAGE_DEVICE or INSTALL_DEVICE
> >   */
> >  while (!isspace(*secLastParamPtr))
> >    secLastParamPtr--;
> >  secLastParamPtr++;
> >  dbgPrintf("secLastParamPtr: %s\n", secLastParamPtr);
> >  if (!strncmp(secLastParamPtr, "(hd", 3)) {
> >    secLastParamPtr += 3;
> >    dbgPrintf("secLastParamPtr: %c\n", *secLastParamPtr);
> >    installDeviceNumber = *secLastParamPtr;
> >  } else {
> >    installDeviceNumber = *lastParamPtr;
> >  }
> >  *devicePtr = malloc(6);
> >  snprintf(*devicePtr, 6, "(hd%c)", installDeviceNumber);
> >  dbgPrintf("installDeviceNumber: %c\n", installDeviceNumber);
> >  break;
> >  }
> > }
>
> This all has those same problems, of course.
> > free(line);
> > fclose(grubConf);
> > return 0;
> >
> > +}
> >
> > +int grubGetBootFromDeviceMap(const char * path,
> >             const char * device,
> >             char ** bootPtr) {
> > FILE * deviceMap;
> > char * line = NULL;
> > size_t len = 0;
> > char * devicePtr;
> > char * end;
> > if (!path) return 1;
> > if (!device) return 1;
> > if (!bootPtr) return 1;
> > deviceMap = fopen(path, "r");
> > if (!deviceMap) return 1;
> > while (getline(&line,&len, deviceMap) != -1) {
> >    if (strncmp(line, "#", 1)) {
> >    devicePtr = line;
> >    while (isspace(*line))
> >        devicePtr++;
>
> How does this work?  Since line is never advanced, you're going to 
> keep on
> finding the same whitespace.  If it were advanced, you'd need to keep 
> check of the bounds of "line" here to avoid segfaulting.
> >    dbgPrintf("device: %s\n", devicePtr);
>
> This too can segfault.
> >    if (!strncmp(devicePtr, device, strlen(device))) {
> >    devicePtr += strlen(device);
> >    while (isspace(*devicePtr))
> >        devicePtr++;
>
> This one segfaults as well.
> >    end = strchr(devicePtr, '\n');
>
> if the end of file doesn't have a newline, this is bad, too.
> >    if (end)
> >        *end = '\0';
> >    *bootPtr = strdup(devicePtr);
>
> Need to bounds check here as well.
> >    break;
> >  }
> > }
> > }
> > free(line);
> > fclose(deviceMap);
> > return 0;
> >
> > +}
> >
> > +int suseGrubConfGetBoot(const char * path, char ** bootPtr) {
> > char * grubDevice;
> > suseGrubConfGetInstallDevice(path,&grubDevice);
> > dbgPrintf("grubby installation device: %s\n", grubDevice);
> > grubGetBootFromDeviceMap(GRUB_DEVICE_MAP, grubDevice, bootPtr);
> > dbgPrintf("boot: %s\n", *bootPtr);
> > return 0;
> >
> > +}
>
> There's no point in #defining GRUB_DEVICE_MAP if this is the only 
> place it's
> going to get used.
> > +int parseSuseGrubConf(int * lbaPtr, char ** bootPtr) {
> > if (isSuseGrubConf(SUSE_GRUB_CONF)) return 1;
> > if (lbaPtr) {
> >    *lbaPtr = 0;
> >    if (suseGrubConfGetLba(SUSE_GRUB_CONF, lbaPtr))
> >        return 1;
> > }
> > if (bootPtr) {
> >    *bootPtr = NULL;
> >    suseGrubConfGetBoot(SUSE_GRUB_CONF, bootPtr);
> > }
> > return 0;
> >
> > +}
>
> Okay.
> > int parseSysconfigGrub(int * lbaPtr, char ** bootPtr) {
> >     FILE * in;
> >     char buf[1024];
> >
> > @@ -1988,11 +2194,19 @@ int parseSysconfigGrub(int * lbaPtr, char ** 
> > bootPtr) {
> >   void dumpSysconfigGrub(void) {
> >       char * boot;
> >       int lba;
> > int notOnSuse;
> > if (!parseSysconfigGrub(&lba,&boot)) {
> > if (lba) printf("lba\n");
> > if (boot) printf("boot=%s\n", boot);
> > notOnSuse = access(SUSE_RELEASE_FILE, R_OK);
>
> I'd really rather this was a function, isSuse(), that returns 1 if it is.
> > if (notOnSuse) {
> >    if (parseSysconfigGrub(&lba,&boot))
> >  return;
>
> This needs a value.  Did you build this code?
> > } else {
> >    if (parseSuseGrubConf(&lba,&boot))
> >  return;
>
> Same here.
> >   }
> >
> > if (lba) printf("lba\n");
> >
> > if (boot) printf("boot=%s\n", boot);
> > }
> >
> > int displayInfo(struct grubConfig * config, char * kernel,
> >
> > @@ -2812,9 +3026,14 @@ int checkForGrub(struct grubConfig * config) {
> >       int fd;
> >       unsigned char bootSect[512];
> >       char * boot;
> > int notOnSuse;
> > if (parseSysconfigGrub(NULL,&boot))
> > return 0;
> >
> > notOnSuse = access(SUSE_RELEASE_FILE, R_OK);
> >
> > if (notOnSuse) {
> >
> > if (parseSysconfigGrub(NULL,&boot)) return 0;
> >
> > } else {
> >
> > if (parseSuseGrubConf(NULL,&boot)) return 0;
> >
> > }
> >
> > /* assume grub is not installed -- not an error condition */
> >  if (!boot)
> >
> > @@ -2833,7 +3052,14 @@ int checkForGrub(struct grubConfig * config) {
> >       }
> >       close(fd);
> > return checkDeviceBootloader(boot, bootSect);
> > if (notOnSuse)
> >  return checkDeviceBootloader(boot, bootSect);
> > else
> >  /*
> >   * The more elaborate checks do not work on SuSE. The checks done
> >   * seem to be reasonble (at least for now), so just return 
> >
> > success
> >   */
> >  return 2;
> >
> > }
> >
> > int checkForExtLinux(struct grubConfig * config) {
>
> I'll leave it to you to revise all three of your patches with this 
> feedback in
> mind.
>
> Thanks!

Peter,

Thanks for the feedback. I'll address all those issues that you have 
pointed out and get back to you with an updated version.

One thing that was not clear, though, is your opinion on patch #3, that 
is, how far should we go to check that a bootloader (yaboot in this 
particular case) is indeed installed on the system. Can you shed some 
like on that subject?

Thanks again!
CR.

[PATCH 1/3] RFC: Fix grub detection on SuSE systems: lba and boot information

[Peter Jones]

On 02/28/2012 08:20 AM, Cleber Rosa wrote:
> Peter,
>
> Thanks for the feedback. I'll address all those issues that you have
> pointed out and get back to you with an updated version.
>
> One thing that was not clear, though, is your opinion on patch #3,
> that is, how far should we go to check that a bootloader (yaboot in
> this particular case) is indeed installed on the system. Can you shed
> some like on that subject?

In general checking for the file is fine in grubby - in new-kernel-pkg, we
*always* call it with a config file (and format) manually specified, and if you're calling it from the command line and it has trouble, you can do the
same. So that's sufficient in my view.

[PATCH 2/3] RFC: Fix grub detection on SuSE systems: config file order

[Cleber Rosa]

SuSE sytems ship with a /etc/grub.conf that is not your regular grub
configuration file, but instead a sequence of grub commands. Example:

setup --stage2=/boot/grub/stage2 --force-lba (hd0,1) (hd0,1)
quit

Because of that, the parsing of the config fails. So, try to first
use the grub config file at /boot/grub/menu.lst.

Signed-off-by: Cleber Rosa 
---
 grubby.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/grubby.c b/grubby.c
index c664387..39d7846 100644
--- a/grubby.c
+++ b/grubby.c
@@ -174,8 +174,8 @@ struct keywordTypes grubKeywords[] = {

const char *grubFindConfig(struct configFileInfo *cfi) {
     static const char *configFiles[] = {
-	"/etc/grub.conf",
    "/boot/grub/menu.lst",
+	"/etc/grub.conf",
    NULL
     };
     static int i = -1;

[PATCH 3/3] RFC: ppc64/yaboot: add support for probing the currently installed bootloader

[Cleber Rosa]

Signed-off-by: Cleber Rosa 
---
 grubby.c |   37 ++++++++++++++++++++++++++++++++-----
 1 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/grubby.c b/grubby.c
index 39d7846..3abdc94 100644
--- a/grubby.c
+++ b/grubby.c
@@ -3092,6 +3092,22 @@ int checkForExtLinux(struct grubConfig * config) {
     return checkDeviceBootloader(boot, bootSect);
 }

+int checkForYaboot(struct grubConfig * config) {
+    /*
+     * This is a simplistic check, trully a placeholder. To properly check if
+     * yaboot is *installed* we'd need to:
+     * 1) get the system boot device (LT_BOOT)
+     * 2) considering it's a raw filesystem, check if the yaboot binary matches
+     *    the content on the boot device
+     * 3) if not, copy the binary to a temporary file and run "addnote" on it
+     * 4) check again if binary and boot device contents match
+     */
+    if (!access("/etc/yaboot.conf", R_OK))
+	return 2;
+
+    return 1;
+}
+
 static char * getRootSpecifier(char * str) {
     char * idx, * rootspec = NULL;

@@ -3579,9 +3595,9 @@ int main(int argc, const char ** argv) {
    { "boot-filesystem", 0, POPT_ARG_STRING, &bootPrefix, 0,
        _("filestystem which contains /boot directory (for testing only)"),
        _("bootfs") },
-#if defined(__i386__) || defined(__x86_64__)
+#if defined(__i386__) || defined(__x86_64__) || defined (__powerpc64__)
    { "bootloader-probe", 0, POPT_ARG_NONE, &bootloaderProbe, 0,
-	    _("check if lilo is installed on lilo.conf boot sector") },
+	    _("check which bootloader is installed on boot sector") },
 #endif
    { "config-file", 'c', POPT_ARG_STRING, &grubConfig, 0,
        _("path to grub config file to update ("-" for stdin)"), 
@@ -3821,8 +3837,8 @@ int main(int argc, const char ** argv) {
     }

if (bootloaderProbe) {
-	int lrc = 0, grc = 0, gr2c = 0, erc = 0;
-	struct grubConfig * lconfig, * gconfig;
+      int lrc = 0, grc = 0, gr2c = 0, erc = 0, yrc = 0;
+      struct grubConfig * lconfig, * gconfig, * yconfig;

const char *grub2config = grub2FindConfig(&grub2ConfigType);
    if (grub2config) {
@@ -3858,12 +3874,23 @@ int main(int argc, const char ** argv) {
    	erc = checkForExtLinux(lconfig);
    }

-	if (lrc == 1 || grc == 1 || gr2c == 1) return 1;
+
+	if (!access(yabootConfigType.defaultConfig, F_OK)) {
+	    yconfig = readConfig(yabootConfigType.defaultConfig,
+				 &yabootConfigType);
+	    if (!yconfig)
+		yrc = 1;
+	    else
+	      yrc = checkForYaboot(lconfig);
+	}
+
+	if (lrc == 1 || grc == 1 || gr2c == 1 || yrc == 1) return 1;

if (lrc == 2) printf("lilo\n");
    if (gr2c == 2) printf("grub2\n");
    if (grc == 2) printf("grub\n");
    if (erc == 2) printf("extlinux\n");
+	if (yrc == 2) printf("yaboot\n");

return 0;
     }

[Lucas Meneghel Rodrigues]

On 01/30/2012 06:41 PM, Cleber Rosa wrote:
> This is an RFC for adding support for SuSE systems (running Grub) and other
> Linux systems running on Power with yaboot (tested on RHEL 6).
>
> Patch #3 is really a placeholder and it's the one I'm mostly interested
> in getting feedback. The reasoning is that current grub2 checks are
> really simplistic, and I'd like to know whether the same thing should be
> OK for other bootloaders. In the inline comments are a proposal for a real
> check of yaboot installation.

Ping?
> Series summary:
>
> [PATCH 1/3] RFC: Fix grub detection on SuSE systems: lba and boot
> [PATCH 2/3] RFC: Fix grub detection on SuSE systems: config file
> [PATCH 3/3] RFC: ppc64/yaboot: add support for probing the currently
>
> Anaconda-devel-list mailing list
> [e-mail]
> https://www.redhat.com/mailman/listinfo/anaconda-devel-list