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<crosa@xxxxxxxxxx>
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!
--
Peter
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
