Patchwork Add assignment operation to config file parser..

login
register
mail settings
Submitter john cooper
Date Feb. 6, 2010, 6:59 p.m.
Message ID <4B6DBC01.4060307@redhat.com>
Download mbox | patch
Permalink /patch/44723/
State New
Headers show

Comments

john cooper - Feb. 6, 2010, 6:59 p.m.
This patch reworks support for both assignment and
append in the config file parser.  It was motivated
by comments received on the cpu model config file
format.

Commit dc9ca4ba27be4fe6a0284061b8f056c4364fb0d9
changed the behavior of "=" from assign to append.
This patch preserves the ability to append to an
option (however now via "+="), reverts "=" to its
previous behavior, and allows both to interoperate.

Signed-off-by: john cooper <john.cooper@redhat.com>
---
Anthony Liguori - Feb. 7, 2010, 4:24 p.m.
On 02/06/2010 12:59 PM, john cooper wrote:
> This patch reworks support for both assignment and
> append in the config file parser.  It was motivated
> by comments received on the cpu model config file
> format.
>
> Commit dc9ca4ba27be4fe6a0284061b8f056c4364fb0d9
> changed the behavior of "=" from assign to append.
> This patch preserves the ability to append to an
> option (however now via "+="), reverts "=" to its
> previous behavior, and allows both to interoperate.
>
> Signed-off-by: john cooper<john.cooper@redhat.com>
>    

This deviates from standard ini syntax which makes me a big 
uncomfortable with it.  Gerd, do you have an opinion?

Regards,

Anthony Liguori
Gerd Hoffmann - Feb. 8, 2010, 1:21 p.m.
On 02/07/10 17:24, Anthony Liguori wrote:
> On 02/06/2010 12:59 PM, john cooper wrote:
>> This patch reworks support for both assignment and
>> append in the config file parser. It was motivated
>> by comments received on the cpu model config file
>> format.
>>
>> Commit dc9ca4ba27be4fe6a0284061b8f056c4364fb0d9
>> changed the behavior of "=" from assign to append.
>> This patch preserves the ability to append to an
>> option (however now via "+="), reverts "=" to its
>> previous behavior, and allows both to interoperate.
>>
>> Signed-off-by: john cooper<john.cooper@redhat.com>
>
> This deviates from standard ini syntax which makes me a big
> uncomfortable with it. Gerd, do you have an opinion?

Also it the syntax change will break existing users of the append 
feature (host/guestfwd for slirp networking).

Any reason why you can't use the current append to avoid the overlong 
feature flag lines?

Another idea:  One could reference other processors "base", then you can 
define a -- say -- Opteron_G3 as "Opteron_G2 features plus these".

cheers,
   Gerd
john cooper - Feb. 8, 2010, 4 p.m.
Gerd Hoffmann wrote:
> On 02/07/10 17:24, Anthony Liguori wrote:
>> On 02/06/2010 12:59 PM, john cooper wrote:
>>> This patch reworks support for both assignment and
>>> append in the config file parser. It was motivated
>>> by comments received on the cpu model config file
>>> format.
>>>
>>> Commit dc9ca4ba27be4fe6a0284061b8f056c4364fb0d9
>>> changed the behavior of "=" from assign to append.
>>> This patch preserves the ability to append to an
>>> option (however now via "+="), reverts "=" to its
>>> previous behavior, and allows both to interoperate.
>>>
>>> Signed-off-by: john cooper<john.cooper@redhat.com>
>>
>> This deviates from standard ini syntax which makes me a big
>> uncomfortable with it. Gerd, do you have an opinion?
> 
> Also it the syntax change will break existing users of the append
> feature (host/guestfwd for slirp networking).
> 
> Any reason why you can't use the current append to avoid the overlong
> feature flag lines?

Impacting existing usage was my primary concern as well.
I'd run this by Mark who created the patch changing the
disposition of "=" to append and I didn't find any alarms
there.

The proposed scheme supports both semantics and arguably
seems more intuitive.  If that is the general consensus
and it won't unduly perturb existing usage the above
would be opportune before the current behavior becomes
more entrenched (e.g. by further dependencies such as the
proposed cpu model configuration).

But to answer the question, my prior patch can work with
either.

Thanks,

-john

Patch

diff --git a/qemu-config.c b/qemu-config.c
index 246fae6..4e53250 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -429,6 +429,7 @@  int qemu_config_parse(FILE *fp)
     char line[1024], group[64], id[64], arg[64], value[1024];
     QemuOptsList *list = NULL;
     QemuOpts *opts = NULL;
+    char append;
 
     while (fgets(line, sizeof(line), fp) != NULL) {
         if (line[0] == '\n') {
@@ -455,13 +456,16 @@  int qemu_config_parse(FILE *fp)
             opts = qemu_opts_create(list, NULL, 0);
             continue;
         }
-        if (sscanf(line, " %63s = \"%1023[^\"]\"", arg, value) == 2) {
-            /* arg = value */
+        append = 0;
+        if (sscanf(line, " %63s = \"%1023[^\"]\"", arg, value) == 2 ||
+            (sscanf(line, " %63s += \"%1023[^\"]\"", arg, value) == 2 ?
+                append = 1 : 0)) {
+            /* arg = value, arg += value */
             if (opts == NULL) {
                 fprintf(stderr, "no group defined\n");
                 return -1;
             }
-            if (qemu_opt_set(opts, arg, value) != 0) {
+            if (_qemu_opt_set(opts, arg, value, append) != 0) {
                 fprintf(stderr, "failed to set \"%s\" for %s\n",
                         arg, group);
                 return -1;
diff --git a/qemu-option.c b/qemu-option.c
index a52a4c4..7c0faed 100644
--- a/qemu-option.c
+++ b/qemu-option.c
@@ -562,7 +562,11 @@  static void qemu_opt_del(QemuOpt *opt)
     qemu_free(opt);
 }
 
-int qemu_opt_set(QemuOpts *opts, const char *name, const char *value)
+/* add option *name,*value to group *opts.  if append add to tail of option
+ * list, else set as sole element (overwrite any existing entries of *name).
+ */
+int _qemu_opt_set(QemuOpts *opts, const char *name, const char *value,
+                  char append)
 {
     QemuOpt *opt;
     QemuOptDesc *desc = opts->list->desc;
@@ -582,13 +586,27 @@  int qemu_opt_set(QemuOpts *opts, const char *name, const char *value)
             return -1;
         }
     }
-
-    opt = qemu_mallocz(sizeof(*opt));
-    opt->name = qemu_strdup(name);
-    opt->opts = opts;
-    QTAILQ_INSERT_TAIL(&opts->head, opt, next);
-    if (desc[i].name != NULL) {
-        opt->desc = desc+i;
+    if (append || !(opt = qemu_opt_find(opts, name))) {
+        opt = qemu_mallocz(sizeof(*opt));
+        opt->name = qemu_strdup(name);
+        opt->opts = opts;
+        QTAILQ_INSERT_TAIL(&opts->head, opt, next);
+        if (desc[i].name != NULL) {
+            opt->desc = desc+i;
+        }
+    } else if (!append) {
+        QemuOpt *p, *next;
+
+        /* assign to reclaimed *opt, remove all other *name defs */
+        QTAILQ_FOREACH_SAFE(p, &opts->head, next, next) {
+            if (p != opt && !strcmp(name, p->name)) {
+                qemu_free((char *)p->str);
+                QTAILQ_REMOVE(&opts->head, p, next);
+                qemu_free((char *)p);
+            }
+        }
+        qemu_free((char *)opt->str);
+        opt->str = NULL;
     }
     if (value) {
         opt->str = qemu_strdup(value);
diff --git a/qemu-option.h b/qemu-option.h
index 666b666..2385b1a 100644
--- a/qemu-option.h
+++ b/qemu-option.h
@@ -104,7 +104,14 @@  const char *qemu_opt_get(QemuOpts *opts, const char *name);
 int qemu_opt_get_bool(QemuOpts *opts, const char *name, int defval);
 uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval);
 uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval);
-int qemu_opt_set(QemuOpts *opts, const char *name, const char *value);
+int _qemu_opt_set(QemuOpts *opts, const char *name, const char *value,
+                  char append);
+static inline int qemu_opt_set(QemuOpts *opts, const char *name,
+                               const char *value)
+{
+    return (_qemu_opt_set(opts, name, value, 0));
+}
+
 typedef int (*qemu_opt_loopfunc)(const char *name, const char *value, void *opaque);
 int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
                      int abort_on_failure);