On 09/05/2012 03:45 PM, Simon Horman wrote:
> 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.
Thanks for review.
>
>>
>> Signed-off-by: Dave Young <dyoung at redhat.com>
>> ---
>> 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.
Will switch to use strncpy
>
>> +
>> + 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.
Oops, will fix
>
> Also, I wonder if read(fd, str_tmp, sizeof str_tmp) might make
> things a bit easier over all.
This is fine to me as well. Just also need to end with '\n'
As Khalid said in another reply, the ACPI20= might not be the first
line. will resolve that issue as well.
>
> Finally, I have a personal preference for "a + i" over "&a[i]".
> Though I'm not going to get excited about it.
Both are fine to me, will convert.
>
>> +
>> + 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.
Will do
>
>> +
>> + 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
> kexec at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
--
Thanks
Dave
