Hi - Looks good, just some minor stuff (below):
On 2015-11-02 at 19:19 "'Davide Libenzi' via Akaros" <[email protected]> wrote: > This CL adds support for multiboot command line, and also adds an API > to retrieve boot options. > Tested with qemu -append option. > > https://github.com/brho/akaros/compare/master...dlibenzi:boot_cmdline > > > The following changes since commit > 2a9b3cdc47dbde55f9d125fde3e11832ca4c0b91: > > Avoid double declarations, integer overflow, and use branch hints > (2015-10-30 16:02:29 -0400) > > are available in the git repository at: > > [email protected]:dlibenzi/akaros boot_cmdline > > for you to fetch changes up to > 51b5249fe747f5ecd0a2d4c21da3d33d2c3a7217: > > Added support for multiboot protocol command line extraction > (2015-11-02 19:14:32 -0800) Two checkpatch complaints: ../patches/0002-Added-kernel-test-case-for-command-line-parsing-code.patch -------------------------------------------------------------------------- WARNING: quoted string split across lines #47: FILE: kern/src/ktest/pb_ktests.c:2214: + "kernel -root=/foo -simple -num=123 -quoted='abc \\'' -dup=311 -dup='akaros' " + "-inner=-outer -outer=-inner=xyz"; This one is probably okay, though I got a little confused with the 'abc \\''. are \s the escapes, and if so, are we handling the '' correctly for the quoted case? WARNING: line over 80 characters #93: FILE: kern/src/ktest/pb_ktests.c:2260: + KT_ASSERT_M("Invalid -outer option value", strcmp(param, "-inner=xyz") == 0); > From 4d25c7595ca4018e12a536060af20dd34385c395 Mon Sep 17 00:00:00 2001 > From: Davide Libenzi <[email protected]> > Date: Mon, 2 Nov 2015 19:14:32 -0800 > Subject: Added support for multiboot protocol command line extraction > +static void extract_multiboot_cmdline(struct multiboot_info *mbi) > +{ > + if (mbi && (mbi->flags & MULTIBOOT_INFO_CMDLINE) && mbi->cmdline) { > + const char *cmdln = (const char *) KADDR(mbi->cmdline); > + > + /* We need to copy the command line in a permanent buffer, > since the > + * multiboot memory where it is currently residing will be part > of the > + * free boot memory later on in the boot process. > + */ More of an FYI, but we have no plans to ever try and free boot memory. > + strncpy(boot_cmdline, cmdln, sizeof(boot_cmdline)); > + boot_cmdline[sizeof(boot_cmdline) - 1] = 0; Please use strlcpy for this, which Dan recently added. We're trying to cut down on the strncpys. Barret -- You received this message because you are subscribed to the Google Groups "Akaros" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. For more options, visit https://groups.google.com/d/optout.
