On Wed, Sep 05, 2012 at 01:44:45PM +0800, Dave Young wrote: > > In case efi booting, kdump need kernel parameter noefi and acpi_rsdp= > to disable efi reinit and retrieve the acpi root table physical address. > > Add a function cmdline_add_efi to get the address from > /sys/firmware/efi/systab > If there's no such file or read fail the function will just do nothing. > > Tested efi boot Fedora 17 on thinkpad T420.
Hi Dave, conceptually I am fine with this patch. However, I do have a few nits in relation to the string handling as explained below. > > Signed-off-by: Dave Young <[email protected]> > --- > kexec/arch/i386/crashdump-x86.c | 47 > ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 47 insertions(+) > > --- kexec-tools.orig/kexec/arch/i386/crashdump-x86.c > +++ kexec-tools/kexec/arch/i386/crashdump-x86.c > @@ -27,6 +27,7 @@ > #include <sys/types.h> > #include <sys/stat.h> > #include <unistd.h> > +#include <fcntl.h> > #include "../../kexec.h" > #include "../../kexec-elf.h" > #include "../../kexec-syscall.h" > @@ -658,6 +659,51 @@ static int cmdline_add_memmap_acpi(char > return 0; > } > > +/* Appends 'noefi acpi_rsdp=' commandline for efi boot crash dump */ > +static void cmdline_add_efi(char *cmdline) > +{ > + int cmdlen, len, fd, ret, i = 0; > + char str_efi[64], str_tmp[32]; > + > + memset(str_efi, 0x0, sizeof(str_efi)); > + memset(str_tmp, 0x0, sizeof(str_tmp)); > + snprintf(str_efi, 18, "%s", " noefi acpi_rsdp="); strncpy() might be be easier on the eyes. > + > + fd = open("/sys/firmware/efi/systab", O_RDONLY); > + if (fd < 0) > + return; > + > + while(1) { > + ret = read(fd, &str_tmp[i], 1); > + if (ret <= 0) > + goto out_close; > + if (str_tmp[i] == '\n') { > + str_tmp[i] = '\0'; > + break; > + } > + i++; > + } It seems to me that the above code may overflow str_tmp. Also, I wonder if read(fd, str_tmp, sizeof str_tmp) might make things a bit easier over all. Finally, I have a personal preference for "a + i" over "&a[i]". Though I'm not going to get excited about it. > + > + if (!(strncmp(str_tmp, "ACPI20=", 7))) > + strcat(str_efi, &str_tmp[7]); > + else if (!(strncmp(str_tmp, "ACPI=", 5))) > + strcat(str_efi, &str_tmp[5]); > + else > + goto out_close; I wonder if strncat() would be better above, though I don't think an overflow can actually occur if str_tmp is null-terminated. > + > + len = strlen(str_efi); > + cmdlen = strlen(cmdline) + len; > + if (cmdlen > (COMMAND_LINE_SIZE - 1)) > + die("Command line overflow\n"); > + strcat(cmdline, str_efi); > + > + dbgprintf("Command line after adding efi\n"); > + dbgprintf("%s\n", cmdline); > + > +out_close: > + close(fd); > +} > + > static void get_backup_area(unsigned long *start, unsigned long *end) > { > const char *iomem = proc_iomem(); > @@ -830,6 +876,7 @@ int load_crashdump_segments(struct kexec > if (delete_memmap(memmap_p, elfcorehdr, memsz) < 0) > return -1; > cmdline_add_memmap(mod_cmdline, memmap_p); > + cmdline_add_efi(mod_cmdline); > cmdline_add_elfcorehdr(mod_cmdline, elfcorehdr); > > /* Inform second kernel about the presence of ACPI tables. */ > _______________________________________________ kexec mailing list [email protected] http://lists.infradead.org/mailman/listinfo/kexec
