Code Review - Add support for multiboot command line

[Davide Libenzi]

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:

  g...@github.com: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)

----------------------------------------------------------------

Davide Libenzi (1):

      Added support for multiboot protocol command line extraction

 kern/include/init.h   | 11 +++++++++++

 kern/include/string.h |  2 +-

 kern/src/init.c       | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++

 kern/src/strstr.c     |  4 ++--

 4 files changed, 69 insertions(+), 3 deletions(-)

 create mode 100644 kern/include/init.h

diff --git a/kern/include/init.h b/kern/include/init.h

new file mode 100644

index 0000000..ee364b5

--- /dev/null

+++ b/kern/include/init.h

@@ -0,0 +1,11 @@

+/* Copyright (c) 2015 Google Inc

+ * Davide Libenzi <dlib...@google.com>

+ * See LICENSE for details.

+ */

+

+#pragma once

+

+const char *get_boot_option(const char *base, const char *option, char *param,

+ size_t max_param);

+void _panic(const char *file, int line, const char *fmt, ...);

+void _warn(const char *file, int line, const char *fmt, ...);

diff --git a/kern/include/string.h b/kern/include/string.h

index efcc890..ab5ac5f 100644

--- a/kern/include/string.h

+++ b/kern/include/string.h

@@ -5,7 +5,7 @@

 

 int strlen(const char *s);

 int strnlen(const char *s, size_t size);

-char *strstr(char *s1, char *s2);

+const char *strstr(const char *s1, const char *s2);

 

 /* zra : These aren't being used, and they are dangerous, so I'm rm'ing them

 STRING strcpy(STRING dst, const STRING src);

diff --git a/kern/src/init.c b/kern/src/init.c

index ba88555..4d1c986 100644

--- a/kern/src/init.c

+++ b/kern/src/init.c

@@ -46,11 +46,62 @@

 #include <acpi.h>

 #include <coreboot_tables.h>

 

+#define MAX_BOOT_CMDLINE_SIZE 4096

+

 int booting = 1;

 struct sysinfo_t sysinfo;

+static char boot_cmdline[MAX_BOOT_CMDLINE_SIZE];

+

 static void run_linker_funcs(void);

 static int run_init_script(void);

 

+const char *get_boot_option(const char *base, const char *option, char *param,

+ size_t max_param)

+{

+ const char *opt;

+

+ if (!base)

+ base = boot_cmdline;

+ else

+ base++;

+ opt = strstr(base, option);

+ if (opt) {

+ const char *arg = opt;

+

+ for (; *arg && (strchr("= \t", *arg) == NULL); arg++);

+ if (*arg == '=') {

+ size_t psize;

+ const char *eoa;

+

+ arg++;

+ if (*arg == '\'') {

+ arg++;

+ for (eoa = arg; *eoa && (*eoa != '\''); eoa++);

+ } else {

+ for (eoa = arg; *eoa && (strchr(" \t", *eoa) == NULL); eoa++);

+ }

+ psize = eoa - arg;

+ psize = MIN(psize, max_param - 1);

+ memcpy(param, arg, psize);

+ param[psize] = 0;

+ } else if (max_param) {

+ *param = 0;

+ }

+ }

+

+ return opt;

+}

+

+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);

+

+ strncpy(boot_cmdline, cmdln, sizeof(boot_cmdline));

+ boot_cmdline[sizeof(boot_cmdline) - 1] = 0;

+ }

+}

+

 void kernel_init(multiboot_info_t *mboot_info)

 {

  extern char edata[], end[];

@@ -62,9 +113,13 @@ void kernel_init(multiboot_info_t *mboot_info)

  * memory, we may clobber it. */

  multiboot_kaddr = (struct multiboot_info*)((physaddr_t)mboot_info

                                                + KERNBASE);

+ extract_multiboot_cmdline(multiboot_kaddr);

+

  cons_init();

  print_cpuinfo();

 

+ printk("Boot Command Line: '%s'\n", boot_cmdline);

+

  exception_table_init();

  cache_init(); // Determine systems's cache properties

  pmem_init(multiboot_kaddr);

diff --git a/kern/src/strstr.c b/kern/src/strstr.c

index 10fbf3a..4904469 100644

--- a/kern/src/strstr.c

+++ b/kern/src/strstr.c

@@ -32,9 +32,9 @@

  * Return pointer to first occurrence of s2 in s1,

  * 0 if none

  */

-char *strstr(char *s1, char *s2)

+const char *strstr(const char *s1, const char *s2)

 {

- char *p;

+ const char *p;

  int f, n;

 

  f = s2[0];

[Davide Libenzi]

I changed the implementation a little, and added a ktest test case, in the same branch.

[Barret Rhoden]

Hi -

Looks good, just some minor stuff (below):

On 2015-11-02 at 19:19 "'Davide Libenzi' via Akaros"

<aka...@googlegroups.com> 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:
>
> g...@github.com: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 <dlib...@google.com>
> 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

[Davide Libenzi]

On Tue, Nov 10, 2015 at 7:59 AM, Barret Rhoden <br...@cs.berkeley.edu> wrote:

Two checkpatch complaints:

Fixed those. Same branch.

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?

Yes, it handles that correctly. Added a test case for that.

> +             /* 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.

The memory where the mboot cmdline resides, is part of the one that gets handed over as free.

Ask me how I know ... 😀

> +             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.

Fixed as well, same branch.

[ron minnich]

apropos the no plans to free boot memory comment, here's a reason. I found a very nice case recently in linux where a pointer to free memory was being used. I suspect these are more common than we can know. I also suspect new such cases are generated over time.

There are cases where freeing that little bit of memory makes a lot of sense, but I doubt they fit the Akaros use case.

This being C, where you never know where your pointers are sleeping at night, the only reasonable thing I can think of for boot memory would be to make it page-aligned, page-sized, and blow the PTEs away for it when done booting, so you can catch attempts to use it later. Or, just accept that you'll never get to use the 4M you can save by freeing it, and treat it like any other statically initialized variable you get to use for any purpose. 

4M? it's a rounding error in the GNU runtime alone :-) 

ron

[Davide Libenzi]

The Linux recycle of __init memory comes from a time where memory was a bit tighter.

Also, there are companies running Linux on devices where memory is very tight.

We are talking about different things here though.

I was not suggesting we do the discard of __init memory like Linux does.

But the multiboot memory where the command line resides, is overwritten as soon as Akaros starts.

This is why I had to copy it out immediately wihtin kernel_init, to a safe BSS buffer.

--
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 akaros+un...@googlegroups.com.
To post to this group, send email to aka...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Davide Libenzi]

On Tue, Nov 10, 2015 at 8:11 AM, Davide Libenzi <dlib...@google.com> wrote:

On Tue, Nov 10, 2015 at 7:59 AM, Barret Rhoden <br...@cs.berkeley.edu> wrote:

Two checkpatch complaints:

Fixed those. Same branch.

I had also inadvertently fscked up strstr() proto. Fixed now.

[Barret Rhoden]

On 2015-11-10 at 08:11 "'Davide Libenzi' via Akaros"

<aka...@googlegroups.com> wrote:
> On Tue, Nov 10, 2015 at 7:59 AM, Barret Rhoden <br...@cs.berkeley.edu>
> wrote:
>
> > Two checkpatch complaints:
> >
>
> Fixed those. Same branch.

Thanks for the changes.

+ strlcpy(boot_cmdline, cmdln, sizeof(boot_cmdline));

+ boot_cmdline[sizeof(boot_cmdline) - 1] = 0;

we don't need to set boot_cmdline[last] = 0, since strlcpy does that.
I can touch that up, so no need to worry about it.

But... sorry, I missed something else the first time around:

> -char *strstr(char *s1, char *s2)
> +const char *strstr(const char *s1, const char *s2)
> {
> - char *p;
> + const char *p;
> int f, n;
>
> f = s2[0];

This throws some warnings when compiling:

kern/src/net/ethermedium.c: In function 'etherbind':
kern/src/net/ethermedium.c:242:6: warning: assignment discards 'const' qualifier from pointer target type
ptr = strstr(buf, "addr: ");
^
CC kern/src/process.o
kern/src/net/ethermedium.c:248:6: warning: assignment discards 'const' qualifier from pointer target type
ptr = strstr(buf, "mbps: ");
^
kern/src/net/ethermedium.c:256:6: warning: assignment discards 'const' qualifier from pointer target type
ptr = strstr(buf, "feat: ");
^

From glancing at the man page, I think strstr isn't supposed to return
a const char*. Any particular need for that?

As a side note, we can separate out the strstr change into another
commit, since it's not really part of the "logical change" that is the
multiboot stuff.

Barret

[Barret Rhoden]

On 2015-11-10 at 08:22 "'Davide Libenzi' via Akaros"

<aka...@googlegroups.com> wrote:
> The Linux recycle of __init memory comes from a time where memory was
> a bit tighter.
> Also, there are companies running Linux on devices where memory is
> very tight.
> We are talking about different things here though.
> I was not suggesting we do the discard of __init memory like Linux
> does. But the multiboot memory where the command line resides, is
> overwritten as soon as Akaros starts.
> This is why I had to copy it out immediately wihtin kernel_init, to a
> safe BSS buffer.

Thanks for the clarification, my bad there. =)

Barret

[Davide Libenzi]

The strstr fix is in already. You can fix up the 0 ending thing.

Barret

[Barret Rhoden]

On 2015-11-10 at 08:30 "'Davide Libenzi' via Akaros"

Cool, thanks. Pulling it now.

[Barret Rhoden]

Thanks, merged to staging at 4402cbebfec6..f30d87bf34f2 (from, to]

You can see the entire diff with 'git diff' or at
https://github.com/brho/akaros/compare/4402cbebfec6...f30d87bf34f2

--------

And I have a fancy command line now:

Boot Command Line: '/kernel Giraffes Inside'

Barret


On 2015-11-02 at 19:19 "'Davide Libenzi' via Akaros"