[Qemu-devel] [PATCH] multiboot: Support quotable commas in module list

[Qemu-devel] [PATCH] multiboot: Support quotable commas in module list

[Adam Lackorzynski]

Support quoting of ',' (and '\') to allow commas in the parameter list of
modules.

Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de>
---
 hw/multiboot.c |   33 +++++++++++++++++++++++++++++----
 1 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/hw/multiboot.c b/hw/multiboot.c
index 394ed01..73f01aa 100644
--- a/hw/multiboot.c
+++ b/hw/multiboot.c
@@ -95,13 +95,26 @@ typedef struct {
     int mb_mods_count;
 } MultibootState;
 
+static int mb_is_cmdline_quote(const char *c)
+{
+    return c[0] == '\\' && (c[1] == '\\' || c[1] == ',');
+}
+
 static uint32_t mb_add_cmdline(MultibootState *s, const char *cmdline)
 {
     int len = strlen(cmdline) + 1;
+    int ci, bi;
     target_phys_addr_t p = s->offset_cmdlines;
+    char *b = (char *)s->mb_buf + p;
 
-    pstrcpy((char *)s->mb_buf + p, len, cmdline);
-    s->offset_cmdlines += len;
+    for (ci = 0, bi = 0; ci < len - 1; ci++, bi++) {
+        if (mb_is_cmdline_quote(&cmdline[ci])) {
+            ci++;
+        }
+        b[bi] = cmdline[ci];
+    }
+    b[bi] = 0;
+    s->offset_cmdlines += bi + 1;
     return s->mb_buf_phys + p;
 }
 
@@ -124,6 +137,18 @@ static void mb_add_mod(MultibootState *s,
     s->mb_mods_count++;
 }
 
+static const char *mb_cmdline_next_module(const char *c)
+{
+    for (; *c; c++) {
+        if (mb_is_cmdline_quote(c)) {
+            c++;
+        } else if (c[0] == ',') {
+            return c;
+        }
+    }
+    return 0;
+}
+
 int load_multiboot(void *fw_cfg,
                    FILE *f,
                    const char *kernel_filename,
@@ -238,7 +263,7 @@ int load_multiboot(void *fw_cfg,
         const char *r = initrd_filename;
         mbs.mb_buf_size += strlen(r) + 1;
         mbs.mb_mods_avail = 1;
-        while ((r = strchr(r, ','))) {
+        while ((r = mb_cmdline_next_module(r))) {
            mbs.mb_mods_avail++;
            r++;
         }
@@ -261,7 +286,7 @@ int load_multiboot(void *fw_cfg,
             int mb_mod_length;
             uint32_t offs = mbs.mb_buf_size;
 
-            next_initrd = strchr(initrd_filename, ',');
+            next_initrd = (char *)mb_cmdline_next_module(initrd_filename);
             if (next_initrd)
                 *next_initrd = '\0';
             /* if a space comes after the module filename, treat everything
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH] multiboot: Support quotable commas in module list

[Stefan Hajnoczi]

On Fri, Apr 15, 2011 at 8:56 AM, Adam Lackorzynski
<adam@os.inf.tu-dresden.de> wrote:
> Support quoting of ',' (and '\') to allow commas in the parameter list of
> modules.
>
> Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de>
> ---
>  hw/multiboot.c |   33 +++++++++++++++++++++++++++++----
>  1 files changed, 29 insertions(+), 4 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

Re: [Qemu-devel] [PATCH] multiboot: Support quotable commas in module list

[Kevin Wolf]

Am 15.04.2011 09:56, schrieb Adam Lackorzynski:
> Support quoting of ',' (and '\') to allow commas in the parameter list of
> modules.
> 
> Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de>

Other options in qemu use double commas for escaping. So maybe reusing
get_opt_value() would make things more consistent. It also has the
advantage that double commas don't need additional escape characters for
the shell.

On the other hand, using backslashes for escaping is probably more
familiar for most people, so I don't have a very strong opinion on it.

Kevin

Re: [Qemu-devel] [PATCH] multiboot: Support quotable commas in module list

[Adam Lackorzynski]

On Fri Apr 15, 2011 at 15:17:28 +0200, Kevin Wolf wrote:
> Am 15.04.2011 09:56, schrieb Adam Lackorzynski:
> > Support quoting of ',' (and '\') to allow commas in the parameter list of
> > modules.
> > 
> > Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de>
> 
> Other options in qemu use double commas for escaping. So maybe reusing
> get_opt_value() would make things more consistent. It also has the
> advantage that double commas don't need additional escape characters for
> the shell.
> 
> On the other hand, using backslashes for escaping is probably more
> familiar for most people, so I don't have a very strong opinion on it.

Same for me. I like the fact with the double-commas and easier shell
quoting. On the other side using backslashes is more common. However, I
construct the overall command via scripts anyway, so I'll only very
seldom actually type this myself.

Here's how it would look like. Diff is smaller.
More opinions very welcome.


diff --git a/hw/multiboot.c b/hw/multiboot.c
index 394ed01..7d5cb22 100644
--- a/hw/multiboot.c
+++ b/hw/multiboot.c
@@ -97,11 +97,11 @@ typedef struct {
 
 static uint32_t mb_add_cmdline(MultibootState *s, const char *cmdline)
 {
-    int len = strlen(cmdline) + 1;
     target_phys_addr_t p = s->offset_cmdlines;
+    char *b = (char *)s->mb_buf + p;
 
-    pstrcpy((char *)s->mb_buf + p, len, cmdline);
-    s->offset_cmdlines += len;
+    get_opt_value(b, strlen(cmdline) + 1, cmdline);
+    s->offset_cmdlines += strlen(b) + 1;
     return s->mb_buf_phys + p;
 }
 
@@ -238,7 +238,7 @@ int load_multiboot(void *fw_cfg,
         const char *r = initrd_filename;
         mbs.mb_buf_size += strlen(r) + 1;
         mbs.mb_mods_avail = 1;
-        while ((r = strchr(r, ','))) {
+        while (*(r = get_opt_value(NULL, 0, r))) {
            mbs.mb_mods_avail++;
            r++;
         }
@@ -252,7 +252,7 @@ int load_multiboot(void *fw_cfg,
     mbs.offset_cmdlines = mbs.offset_mbinfo + mbs.mb_mods_avail * MB_MOD_SIZE;
 
     if (initrd_filename) {
-        char *next_initrd;
+        char *next_initrd, not_last;
 
         mbs.offset_mods = mbs.mb_buf_size;
 
@@ -261,9 +261,9 @@ int load_multiboot(void *fw_cfg,
             int mb_mod_length;
             uint32_t offs = mbs.mb_buf_size;
 
-            next_initrd = strchr(initrd_filename, ',');
-            if (next_initrd)
-                *next_initrd = '\0';
+            next_initrd = (char *)get_opt_value(NULL, 0, initrd_filename);
+            not_last = *next_initrd;
+            *next_initrd = '\0';
             /* if a space comes after the module filename, treat everything
                after that as parameters */
             target_phys_addr_t c = mb_add_cmdline(&mbs, initrd_filename);
@@ -287,7 +287,7 @@ int load_multiboot(void *fw_cfg,
                      (char *)mbs.mb_buf + offs,
                      (char *)mbs.mb_buf + offs + mb_mod_length, c);
             initrd_filename = next_initrd+1;
-        } while (next_initrd);
+        } while (not_last);
     }
 
     /* Commandline support */



Adam
-- 
Adam                 adam@os.inf.tu-dresden.de
  Lackorzynski         http://os.inf.tu-dresden.de/~adam/

Re: [Qemu-devel] [PATCH] multiboot: Support quotable commas in module list

[Stefan Hajnoczi]

On Sat, Apr 16, 2011 at 10:42 AM, Adam Lackorzynski
<adam@os.inf.tu-dresden.de> wrote:
>
> On Fri Apr 15, 2011 at 15:17:28 +0200, Kevin Wolf wrote:
>> Am 15.04.2011 09:56, schrieb Adam Lackorzynski:
>> > Support quoting of ',' (and '\') to allow commas in the parameter list of
>> > modules.
>> >
>> > Signed-off-by: Adam Lackorzynski <adam@os.inf.tu-dresden.de>
>>
>> Other options in qemu use double commas for escaping. So maybe reusing
>> get_opt_value() would make things more consistent. It also has the
>> advantage that double commas don't need additional escape characters for
>> the shell.
>>
>> On the other hand, using backslashes for escaping is probably more
>> familiar for most people, so I don't have a very strong opinion on it.
>
> Same for me. I like the fact with the double-commas and easier shell
> quoting. On the other side using backslashes is more common. However, I
> construct the overall command via scripts anyway, so I'll only very
> seldom actually type this myself.
>
> Here's how it would look like. Diff is smaller.
> More opinions very welcome.

I like this more because it is more consistent with QEMU syntax and reuses code.

Stefan