| Message ID | 88a1cc0775d3d4f5262b31b9452f8acccc6bbb41.1434458391.git.DirtY.iCE.hu@gmail.com |
|---|---|
| State | New |
| Headers | show |
Copying László because his fingerprints are on OptsVisitor. "Kővágó, Zoltán" <dirty.ice.hu@gmail.com> writes: > The current OptsVisitor flattens the whole structure, if there are same named > fields under different paths (like `in' and `out' in `Audiodev'), the current > visitor can't cope with them (for example setting `frequency=44100' will set the > in's frequency to 44100 and leave out's frequency unspecified). > > This patch fixes it, by the following changes: > 1) Specifying just the field name will apply to all fields that has the > specified name (this means it would set both in's and out's frequency to > 44100 in the above example). > 2) Optionally user can specify the path in the hierarchy. Names are separated by > a dot (e.g. `in.frequency', `foo.bar.something', etc). The user need not > specify the whole path, only the last few components (i.e. `bar.something' is > equivalent to `foo.bar.something' if only `foo' has a `bar' field). This way > 1) is just a special case of this when only the last component is specified. > 3) In case of an ambiguity (e.g `frequency=44100,in.frequency=8000') the longest > matching (the most specific) path wins (so in this example, in's frequency > would become 8000, because `in.frequency' is more specific that `frequency', > and out's frequency would become 44100, because only `frequency' matches it). Can you explain why the complexity is needed, i.e. why we can't just require full paths always?
On Mi, 2015-06-17 at 09:50 +0200, Markus Armbruster wrote: > Copying László because his fingerprints are on OptsVisitor. > > "Kővágó, Zoltán" <dirty.ice.hu@gmail.com> writes: > > > The current OptsVisitor flattens the whole structure, if there are same named > > fields under different paths (like `in' and `out' in `Audiodev'), the current > > visitor can't cope with them (for example setting `frequency=44100' will set the > > in's frequency to 44100 and leave out's frequency unspecified). > > > > This patch fixes it, by the following changes: > > 1) Specifying just the field name will apply to all fields that has the > > specified name (this means it would set both in's and out's frequency to > > 44100 in the above example). > > 2) Optionally user can specify the path in the hierarchy. Names are separated by > > a dot (e.g. `in.frequency', `foo.bar.something', etc). The user need not > > specify the whole path, only the last few components (i.e. `bar.something' is > > equivalent to `foo.bar.something' if only `foo' has a `bar' field). This way > > 1) is just a special case of this when only the last component is specified. > > 3) In case of an ambiguity (e.g `frequency=44100,in.frequency=8000') the longest > > matching (the most specific) path wins (so in this example, in's frequency > > would become 8000, because `in.frequency' is more specific that `frequency', > > and out's frequency would become 44100, because only `frequency' matches it). > > Can you explain why the complexity is needed, i.e. why we can't just > require full paths always? Keeping the short names is required for -netdev backward compatibility. Restricting to short or full (i.e. something= or foo.bar.something=, but disallow bar.something=) should not be a problem. I'm not sure this simplifies things much though. We have to build the full path anyway, and I think bar.something= is just a convenient thing we get almost for free ... cheers, Gerd
2015-06-17 10:41 keltezéssel, Gerd Hoffmann írta: > On Mi, 2015-06-17 at 09:50 +0200, Markus Armbruster wrote: >> Copying László because his fingerprints are on OptsVisitor. >> >> "Kővágó, Zoltán" <dirty.ice.hu@gmail.com> writes: >> >>> The current OptsVisitor flattens the whole structure, if there are same named >>> fields under different paths (like `in' and `out' in `Audiodev'), the current >>> visitor can't cope with them (for example setting `frequency=44100' will set the >>> in's frequency to 44100 and leave out's frequency unspecified). >>> >>> This patch fixes it, by the following changes: >>> 1) Specifying just the field name will apply to all fields that has the >>> specified name (this means it would set both in's and out's frequency to >>> 44100 in the above example). >>> 2) Optionally user can specify the path in the hierarchy. Names are separated by >>> a dot (e.g. `in.frequency', `foo.bar.something', etc). The user need not >>> specify the whole path, only the last few components (i.e. `bar.something' is >>> equivalent to `foo.bar.something' if only `foo' has a `bar' field). This way >>> 1) is just a special case of this when only the last component is specified. >>> 3) In case of an ambiguity (e.g `frequency=44100,in.frequency=8000') the longest >>> matching (the most specific) path wins (so in this example, in's frequency >>> would become 8000, because `in.frequency' is more specific that `frequency', >>> and out's frequency would become 44100, because only `frequency' matches it). >> >> Can you explain why the complexity is needed, i.e. why we can't just >> require full paths always? > > Keeping the short names is required for -netdev backward compatibility. > > Restricting to short or full (i.e. something= or foo.bar.something=, but > disallow bar.something=) should not be a problem. I'm not sure this > simplifies things much though. We have to build the full path anyway, > and I think bar.something= is just a convenient thing we get almost for > free ... With the current implementation you can specify (see my previous patch) in.try-poll=off in case of alsa. If we would need full paths, it would look like opts.data.in.try-poll=off, which is probably not something we want.
Copying Kevin because similar issues exist in the block layer. Gerd Hoffmann <kraxel@redhat.com> writes: > On Mi, 2015-06-17 at 09:50 +0200, Markus Armbruster wrote: >> Copying László because his fingerprints are on OptsVisitor. >> >> "Kővágó, Zoltán" <dirty.ice.hu@gmail.com> writes: >> >> > The current OptsVisitor flattens the whole structure, if there are >> > same named >> > fields under different paths (like `in' and `out' in `Audiodev'), >> > the current >> > visitor can't cope with them (for example setting >> > frequency=44100' will set the >> > in's frequency to 44100 and leave out's frequency unspecified). >> > >> > This patch fixes it, by the following changes: >> > 1) Specifying just the field name will apply to all fields that has the >> > specified name (this means it would set both in's and out's frequency to >> > 44100 in the above example). What if they have different types? What if one of them can't take the value? >> > 2) Optionally user can specify the path in the hierarchy. Names >> > are separated by >> > a dot (e.g. `in.frequency', `foo.bar.something', etc). The user need not >> > specify the whole path, only the last few components >> > (i.e. `bar.something' is >> > equivalent to `foo.bar.something' if only `foo' has a `bar' >> > field). This way >> > 1) is just a special case of this when only the last component >> > is specified. >> > 3) In case of an ambiguity (e.g >> > frequency=44100,in.frequency=8000') the longest >> > matching (the most specific) path wins (so in this example, >> > in's frequency >> > would become 8000, because `in.frequency' is more specific that >> > frequency', >> > and out's frequency would become 44100, because only >> > frequency' matches it). The current rule for multiple assignments is "last one wins". E.g. in -drive if=none,file=tmp.img,file=tmp.qcow2 file=tmp.qcow2 wins. If I understand correctly, this patch amends the rule to "last most specific one wins". Correct? >> Can you explain why the complexity is needed, i.e. why we can't just >> require full paths always? > > Keeping the short names is required for -netdev backward compatibility. I suspect mostly because NetLegacy and Netdev aren't flat unions. Could be self-inflicted pain. What about flattening them instead? Assuming that's possible; I'd have to try. > Restricting to short or full (i.e. something= or foo.bar.something=, but > disallow bar.something=) should not be a problem. I'm not sure this > simplifies things much though. We have to build the full path anyway, > and I think bar.something= is just a convenient thing we get almost for > free ... We've been bitten by convenience features before. Adding them tends to be cheap, but maintaining compatibility can become a terrible headache.
"Kővágó Zoltán" <dirty.ice.hu@gmail.com> writes: > 2015-06-17 10:41 keltezéssel, Gerd Hoffmann írta: >> On Mi, 2015-06-17 at 09:50 +0200, Markus Armbruster wrote: >>> Copying László because his fingerprints are on OptsVisitor. >>> >>> "Kővágó, Zoltán" <dirty.ice.hu@gmail.com> writes: >>> >>>> The current OptsVisitor flattens the whole structure, if there are >>>> same named >>>> fields under different paths (like `in' and `out' in `Audiodev'), >>>> the current >>>> visitor can't cope with them (for example setting >>>> frequency=44100' will set the >>>> in's frequency to 44100 and leave out's frequency unspecified). >>>> >>>> This patch fixes it, by the following changes: >>>> 1) Specifying just the field name will apply to all fields that has the >>>> specified name (this means it would set both in's and out's frequency to >>>> 44100 in the above example). >>>> 2) Optionally user can specify the path in the hierarchy. Names >>>> are separated by >>>> a dot (e.g. `in.frequency', `foo.bar.something', etc). The user need not >>>> specify the whole path, only the last few components >>>> (i.e. `bar.something' is >>>> equivalent to `foo.bar.something' if only `foo' has a `bar' >>>> field). This way >>>> 1) is just a special case of this when only the last component >>>> is specified. >>>> 3) In case of an ambiguity (e.g >>>> frequency=44100,in.frequency=8000') the longest >>>> matching (the most specific) path wins (so in this example, >>>> in's frequency >>>> would become 8000, because `in.frequency' is more specific >>>> that `frequency', >>>> and out's frequency would become 44100, because only >>>> frequency' matches it). >>> >>> Can you explain why the complexity is needed, i.e. why we can't just >>> require full paths always? >> >> Keeping the short names is required for -netdev backward compatibility. >> >> Restricting to short or full (i.e. something= or foo.bar.something=, but >> disallow bar.something=) should not be a problem. I'm not sure this >> simplifies things much though. We have to build the full path anyway, >> and I think bar.something= is just a convenient thing we get almost for >> free ... > > With the current implementation you can specify (see my previous > patch) in.try-poll=off in case of alsa. If we would need full paths, > it would look like opts.data.in.try-poll=off, which is probably not > something we want. I agree opts.data.in.try-poll would be an ugly user interface. But instead of hiding opts.data. with a convenience feature, I'd like us to investigate avoiding it altogether.
2015-06-17 13:18 keltezéssel, Markus Armbruster írta: > Copying Kevin because similar issues exist in the block layer. > > Gerd Hoffmann <kraxel@redhat.com> writes: > >> On Mi, 2015-06-17 at 09:50 +0200, Markus Armbruster wrote: >>> Copying László because his fingerprints are on OptsVisitor. >>> >>> "Kővágó, Zoltán" <dirty.ice.hu@gmail.com> writes: >>> >>>> The current OptsVisitor flattens the whole structure, if there are >>>> same named >>>> fields under different paths (like `in' and `out' in `Audiodev'), >>>> the current >>>> visitor can't cope with them (for example setting >>>> frequency=44100' will set the >>>> in's frequency to 44100 and leave out's frequency unspecified). >>>> >>>> This patch fixes it, by the following changes: >>>> 1) Specifying just the field name will apply to all fields that has the >>>> specified name (this means it would set both in's and out's frequency to >>>> 44100 in the above example). > > What if they have different types? > > What if one of them can't take the value? Currently it will error out, requiring the user to be more explicit. Probably not the best solution, but I can't really think of a better solution. (If we would ignore invalid values that would be very confusing imho.) >>>> 2) Optionally user can specify the path in the hierarchy. Names >>>> are separated by >>>> a dot (e.g. `in.frequency', `foo.bar.something', etc). The user need not >>>> specify the whole path, only the last few components >>>> (i.e. `bar.something' is >>>> equivalent to `foo.bar.something' if only `foo' has a `bar' >>>> field). This way >>>> 1) is just a special case of this when only the last component >>>> is specified. >>>> 3) In case of an ambiguity (e.g >>>> frequency=44100,in.frequency=8000') the longest >>>> matching (the most specific) path wins (so in this example, >>>> in's frequency >>>> would become 8000, because `in.frequency' is more specific that >>>> frequency', >>>> and out's frequency would become 44100, because only >>>> frequency' matches it). > > The current rule for multiple assignments is "last one wins". E.g. in > > -drive if=none,file=tmp.img,file=tmp.qcow2 > > file=tmp.qcow2 wins. > > If I understand correctly, this patch amends the rule to "last most > specific one wins". Correct? Yes. (But I didn't really checked that as I didn't know about the "last one win", and just thought it's an artifact of the current implementation.) >>> Can you explain why the complexity is needed, i.e. why we can't just >>> require full paths always? >> >> Keeping the short names is required for -netdev backward compatibility. > > I suspect mostly because NetLegacy and Netdev aren't flat unions. > Could be self-inflicted pain. > > What about flattening them instead? Assuming that's possible; I'd have > to try. > >> Restricting to short or full (i.e. something= or foo.bar.something=, but >> disallow bar.something=) should not be a problem. I'm not sure this >> simplifies things much though. We have to build the full path anyway, >> and I think bar.something= is just a convenient thing we get almost for >> free ... > > We've been bitten by convenience features before. Adding them tends to > be cheap, but maintaining compatibility can become a terrible headache. >
"Kővágó Zoltán" <dirty.ice.hu@gmail.com> writes: > 2015-06-17 13:18 keltezéssel, Markus Armbruster írta: >> Copying Kevin because similar issues exist in the block layer. >> >> Gerd Hoffmann <kraxel@redhat.com> writes: >> >>> On Mi, 2015-06-17 at 09:50 +0200, Markus Armbruster wrote: >>>> Copying László because his fingerprints are on OptsVisitor. >>>> >>>> "Kővágó, Zoltán" <dirty.ice.hu@gmail.com> writes: >>>> >>>>> The current OptsVisitor flattens the whole structure, if there are >>>>> same named >>>>> fields under different paths (like `in' and `out' in `Audiodev'), >>>>> the current >>>>> visitor can't cope with them (for example setting >>>>> frequency=44100' will set the >>>>> in's frequency to 44100 and leave out's frequency unspecified). >>>>> >>>>> This patch fixes it, by the following changes: >>>>> 1) Specifying just the field name will apply to all fields that has the >>>>> specified name (this means it would set both in's and out's >>>>> frequency to >>>>> 44100 in the above example). >> >> What if they have different types? >> >> What if one of them can't take the value? > > Currently it will error out, requiring the user to be more > explicit. Probably not the best solution, but I can't really think of > a better solution. (If we would ignore invalid values that would be > very confusing imho.) Yes, we clearly don't want foo=0 to set a.foo and b.foo, but foo=x set only a.foo, because the former can take any string, but the latter only a number. Can we require the LHS to be unambiguous? [...]
2015-06-17 15:41 keltezéssel, Markus Armbruster írta: > "Kővágó Zoltán" <dirty.ice.hu@gmail.com> writes: > >> 2015-06-17 13:18 keltezéssel, Markus Armbruster írta: >>> Copying Kevin because similar issues exist in the block layer. >>> >>> Gerd Hoffmann <kraxel@redhat.com> writes: >>> >>>> On Mi, 2015-06-17 at 09:50 +0200, Markus Armbruster wrote: >>>>> Copying László because his fingerprints are on OptsVisitor. >>>>> >>>>> "Kővágó, Zoltán" <dirty.ice.hu@gmail.com> writes: >>>>> >>>>>> The current OptsVisitor flattens the whole structure, if there are >>>>>> same named >>>>>> fields under different paths (like `in' and `out' in `Audiodev'), >>>>>> the current >>>>>> visitor can't cope with them (for example setting >>>>>> frequency=44100' will set the >>>>>> in's frequency to 44100 and leave out's frequency unspecified). >>>>>> >>>>>> This patch fixes it, by the following changes: >>>>>> 1) Specifying just the field name will apply to all fields that has the >>>>>> specified name (this means it would set both in's and out's >>>>>> frequency to >>>>>> 44100 in the above example). >>> >>> What if they have different types? >>> >>> What if one of them can't take the value? >> >> Currently it will error out, requiring the user to be more >> explicit. Probably not the best solution, but I can't really think of >> a better solution. (If we would ignore invalid values that would be >> very confusing imho.) > > Yes, we clearly don't want foo=0 to set a.foo and b.foo, but foo=x set > only a.foo, because the former can take any string, but the latter only > a number. > > Can we require the LHS to be unambiguous? Originally I designed it that way because it allows you to specify frequency=44100 and set both in.frequency and out.frequency. But this could also be the convenience feature that we not really need. I don't see any other downside of making it unambigous.
On 06/17/2015 05:01 AM, Kővágó Zoltán wrote: >>> Can you explain why the complexity is needed, i.e. why we can't just >>> require full paths always? >> >> Keeping the short names is required for -netdev backward compatibility. >> >> Restricting to short or full (i.e. something= or foo.bar.something=, but >> disallow bar.something=) should not be a problem. I'm not sure this >> simplifies things much though. We have to build the full path anyway, >> and I think bar.something= is just a convenient thing we get almost for >> free ... > > With the current implementation you can specify (see my previous patch) > in.try-poll=off in case of alsa. If we would need full paths, it would > look like opts.data.in.try-poll=off, which is probably not something we > want. Elsewhere in the thread, it was suggested that you use a flat union. That would simplify the full path to opts.in.try-poll, rather than opts.data.in.try-poll.
"Kővágó Zoltán" <dirty.ice.hu@gmail.com> writes: > 2015-06-17 15:41 keltezéssel, Markus Armbruster írta: >> "Kővágó Zoltán" <dirty.ice.hu@gmail.com> writes: >> >>> 2015-06-17 13:18 keltezéssel, Markus Armbruster írta: >>>> Copying Kevin because similar issues exist in the block layer. >>>> >>>> Gerd Hoffmann <kraxel@redhat.com> writes: >>>> >>>>> On Mi, 2015-06-17 at 09:50 +0200, Markus Armbruster wrote: >>>>>> Copying László because his fingerprints are on OptsVisitor. >>>>>> >>>>>> "Kővágó, Zoltán" <dirty.ice.hu@gmail.com> writes: >>>>>> >>>>>>> The current OptsVisitor flattens the whole structure, if there are >>>>>>> same named >>>>>>> fields under different paths (like `in' and `out' in `Audiodev'), >>>>>>> the current >>>>>>> visitor can't cope with them (for example setting >>>>>>> frequency=44100' will set the >>>>>>> in's frequency to 44100 and leave out's frequency unspecified). >>>>>>> >>>>>>> This patch fixes it, by the following changes: >>>>>>> 1) Specifying just the field name will apply to all fields that has the >>>>>>> specified name (this means it would set both in's and out's >>>>>>> frequency to >>>>>>> 44100 in the above example). >>>> >>>> What if they have different types? >>>> >>>> What if one of them can't take the value? >>> >>> Currently it will error out, requiring the user to be more >>> explicit. Probably not the best solution, but I can't really think of >>> a better solution. (If we would ignore invalid values that would be >>> very confusing imho.) >> >> Yes, we clearly don't want foo=0 to set a.foo and b.foo, but foo=x set >> only a.foo, because the former can take any string, but the latter only >> a number. >> >> Can we require the LHS to be unambiguous? > > Originally I designed it that way because it allows you to specify > frequency=44100 and set both in.frequency and out.frequency. Is it common to set both to the same value? We could also default one to the other. Not sure that's actually a good idea. > But this > could also be the convenience feature that we not really need. I don't > see any other downside of making it unambigous. Good to know. We've struggled with unforeseen aftereffects of cute convenience features too much, and by now I'm quite reluctant to support them without a really compelling reason.
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c index f2ad6d7..409d8b7 100644 --- a/qapi/opts-visitor.c +++ b/qapi/opts-visitor.c @@ -64,13 +64,14 @@ struct OptsVisitor /* Non-null iff depth is positive. Each key is a QemuOpt name. Each value * is a non-empty GQueue, enumerating all QemuOpt occurrences with that * name. */ - GHashTable *unprocessed_opts; + GHashTable *unprocessed_opts, *opts; /* The list currently being traversed with opts_start_list() / * opts_next_list(). The list must have a struct element type in the * schema, with a single mandatory scalar member. */ ListMode list_mode; GQueue *repeated_opts; + char *repeated_name; /* When parsing a list of repeating options as integers, values of the form * "a-b", representing a closed interval, are allowed. Elements in the @@ -86,6 +87,9 @@ struct OptsVisitor * not survive or escape the OptsVisitor object. */ QemuOpt *fake_id_opt; + + /* List of field names leading to the current structure. */ + GQueue *nested_names; }; @@ -97,11 +101,12 @@ destroy_list(gpointer list) static void -opts_visitor_insert(GHashTable *unprocessed_opts, const QemuOpt *opt) +opts_visitor_insert(OptsVisitor *ov, const QemuOpt *opt) { GQueue *list; + assert(opt); - list = g_hash_table_lookup(unprocessed_opts, opt->name); + list = g_hash_table_lookup(ov->opts, opt->name); if (list == NULL) { list = g_queue_new(); @@ -109,7 +114,8 @@ opts_visitor_insert(GHashTable *unprocessed_opts, const QemuOpt *opt) * "key_destroy_func" in opts_start_struct(). Thus cast away key * const-ness in order to suppress gcc's warning. */ - g_hash_table_insert(unprocessed_opts, (gpointer)opt->name, list); + g_hash_table_insert(ov->opts, (gpointer)opt->name, list); + g_hash_table_insert(ov->unprocessed_opts, (gpointer)opt->name, list); } /* Similarly, destroy_list() doesn't call g_queue_free_full(). */ @@ -127,17 +133,27 @@ opts_start_struct(Visitor *v, void **obj, const char *kind, if (obj) { *obj = g_malloc0(size > 0 ? size : 1); } + + /* assuming name is a statically allocated string (or at least it's lifetime + * is longer than the visitor's) */ + if (!name) { + name = ""; + } + g_queue_push_tail(ov->nested_names, (gpointer) name); + if (ov->depth++ > 0) { return; } - ov->unprocessed_opts = g_hash_table_new_full(&g_str_hash, &g_str_equal, - NULL, &destroy_list); + ov->opts = g_hash_table_new_full(&g_str_hash, &g_str_equal, + NULL, &destroy_list); + ov->unprocessed_opts = g_hash_table_new(&g_str_hash, &g_str_equal); + QTAILQ_FOREACH(opt, &ov->opts_root->head, next) { /* ensured by qemu-option.c::opts_do_parse() */ assert(strcmp(opt->name, "id") != 0); - opts_visitor_insert(ov->unprocessed_opts, opt); + opts_visitor_insert(ov, opt); } if (ov->opts_root->id != NULL) { @@ -145,7 +161,7 @@ opts_start_struct(Visitor *v, void **obj, const char *kind, ov->fake_id_opt->name = g_strdup("id"); ov->fake_id_opt->str = g_strdup(ov->opts_root->id); - opts_visitor_insert(ov->unprocessed_opts, ov->fake_id_opt); + opts_visitor_insert(ov, ov->fake_id_opt); } } @@ -163,6 +179,8 @@ opts_end_struct(Visitor *v, Error **errp) OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v); GQueue *any; + g_queue_pop_tail(ov->nested_names); + if (--ov->depth > 0) { return; } @@ -177,6 +195,8 @@ opts_end_struct(Visitor *v, Error **errp) } g_hash_table_destroy(ov->unprocessed_opts); ov->unprocessed_opts = NULL; + g_hash_table_destroy(ov->opts); + ov->opts = NULL; if (ov->fake_id_opt) { g_free(ov->fake_id_opt->name); g_free(ov->fake_id_opt->str); @@ -185,16 +205,56 @@ opts_end_struct(Visitor *v, Error **errp) ov->fake_id_opt = NULL; } +static void +sum_strlen(gpointer data, gpointer user_data) +{ + const char *str = data; + size_t *sum_len = user_data; + *sum_len += strlen(str) + 1; +} + +static void +append_str(gpointer data, gpointer user_data) +{ + strcat(user_data, data); + strcat(user_data, "."); +} + +/* lookup a name, trying from the most qualified version (e.g. foo.bar.asd) to + * least qualified ones (i.e. foo.bar.asd overrides bar.asd or asd) */ static GQueue * -lookup_distinct(const OptsVisitor *ov, const char *name, Error **errp) +lookup_distinct(const OptsVisitor *ov, const char *name, char **out_key, + Error **errp) { - GQueue *list; + GQueue *list = NULL; + char *key, *key2; + size_t sum_len = strlen(name); - list = g_hash_table_lookup(ov->unprocessed_opts, name); + g_queue_foreach(ov->nested_names, sum_strlen, &sum_len); + key = g_malloc(sum_len+1); + key[0] = 0; + g_queue_foreach(ov->nested_names, append_str, key); + strcat(key, name); + + key2 = key; + while (*key2) { + list = g_hash_table_lookup(ov->opts, key2); + if (list) { + if (out_key) { + *out_key = g_strdup(key2); + } + break; + } + + while (*key2 && *key2++ != '.') { + } + } if (!list) { error_set(errp, QERR_MISSING_PARAMETER, name); } + + g_free(key); return list; } @@ -206,7 +266,7 @@ opts_start_list(Visitor *v, const char *name, Error **errp) /* we can't traverse a list in a list */ assert(ov->list_mode == LM_NONE); - ov->repeated_opts = lookup_distinct(ov, name, errp); + ov->repeated_opts = lookup_distinct(ov, name, &ov->repeated_name, errp); if (ov->repeated_opts != NULL) { ov->list_mode = LM_STARTED; } @@ -242,11 +302,9 @@ opts_next_list(Visitor *v, GenericList **list, Error **errp) /* range has been completed, fall through in order to pop option */ case LM_IN_PROGRESS: { - const QemuOpt *opt; - - opt = g_queue_pop_head(ov->repeated_opts); + g_queue_pop_head(ov->repeated_opts); if (g_queue_is_empty(ov->repeated_opts)) { - g_hash_table_remove(ov->unprocessed_opts, opt->name); + g_hash_table_remove(ov->unprocessed_opts, ov->repeated_name); return NULL; } link = &(*list)->next; @@ -272,22 +330,28 @@ opts_end_list(Visitor *v, Error **errp) ov->list_mode == LM_SIGNED_INTERVAL || ov->list_mode == LM_UNSIGNED_INTERVAL); ov->repeated_opts = NULL; + + g_free(ov->repeated_name); + ov->repeated_name = NULL; + ov->list_mode = LM_NONE; } static const QemuOpt * -lookup_scalar(const OptsVisitor *ov, const char *name, Error **errp) +lookup_scalar(const OptsVisitor *ov, const char *name, char** out_key, + Error **errp) { if (ov->list_mode == LM_NONE) { GQueue *list; /* the last occurrence of any QemuOpt takes effect when queried by name */ - list = lookup_distinct(ov, name, errp); + list = lookup_distinct(ov, name, out_key, errp); return list ? g_queue_peek_tail(list) : NULL; } assert(ov->list_mode == LM_IN_PROGRESS); + assert(out_key == NULL || *out_key == NULL); return g_queue_peek_head(ov->repeated_opts); } @@ -309,13 +373,15 @@ opts_type_str(Visitor *v, char **obj, const char *name, Error **errp) { OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v); const QemuOpt *opt; + char *key = NULL; - opt = lookup_scalar(ov, name, errp); + opt = lookup_scalar(ov, name, &key, errp); if (!opt) { return; } *obj = g_strdup(opt->str ? opt->str : ""); - processed(ov, name); + processed(ov, key); + g_free(key); } @@ -325,8 +391,9 @@ opts_type_bool(Visitor *v, bool *obj, const char *name, Error **errp) { OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v); const QemuOpt *opt; + char *key = NULL; - opt = lookup_scalar(ov, name, errp); + opt = lookup_scalar(ov, name, &key, errp); if (!opt) { return; } @@ -343,13 +410,15 @@ opts_type_bool(Visitor *v, bool *obj, const char *name, Error **errp) } else { error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name, "on|yes|y|off|no|n"); + g_free(key); return; } } else { *obj = true; } - processed(ov, name); + processed(ov, key); + g_free(key); } @@ -361,13 +430,14 @@ opts_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp) const char *str; long long val; char *endptr; + char *key = NULL; if (ov->list_mode == LM_SIGNED_INTERVAL) { *obj = ov->range_next.s; return; } - opt = lookup_scalar(ov, name, errp); + opt = lookup_scalar(ov, name, &key, errp); if (!opt) { return; } @@ -381,11 +451,13 @@ opts_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp) if (errno == 0 && endptr > str && INT64_MIN <= val && val <= INT64_MAX) { if (*endptr == '\0') { *obj = val; - processed(ov, name); + processed(ov, key); + g_free(key); return; } if (*endptr == '-' && ov->list_mode == LM_IN_PROGRESS) { long long val2; + assert(key == NULL); str = endptr + 1; val2 = strtoll(str, &endptr, 0); @@ -406,6 +478,7 @@ opts_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp) error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name, (ov->list_mode == LM_NONE) ? "an int64 value" : "an int64 value or range"); + g_free(key); } @@ -417,13 +490,14 @@ opts_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp) const char *str; unsigned long long val; char *endptr; + char *key = NULL; if (ov->list_mode == LM_UNSIGNED_INTERVAL) { *obj = ov->range_next.u; return; } - opt = lookup_scalar(ov, name, errp); + opt = lookup_scalar(ov, name, &key, errp); if (!opt) { return; } @@ -435,11 +509,13 @@ opts_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp) if (parse_uint(str, &val, &endptr, 0) == 0 && val <= UINT64_MAX) { if (*endptr == '\0') { *obj = val; - processed(ov, name); + processed(ov, key); + g_free(key); return; } if (*endptr == '-' && ov->list_mode == LM_IN_PROGRESS) { unsigned long long val2; + assert(key == NULL); str = endptr + 1; if (parse_uint_full(str, &val2, 0) == 0 && @@ -458,6 +534,7 @@ opts_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp) error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name, (ov->list_mode == LM_NONE) ? "a uint64 value" : "a uint64 value or range"); + g_free(key); } @@ -468,8 +545,9 @@ opts_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp) const QemuOpt *opt; int64_t val; char *endptr; + char *key = NULL; - opt = lookup_scalar(ov, name, errp); + opt = lookup_scalar(ov, name, &key, errp); if (!opt) { return; } @@ -479,11 +557,13 @@ opts_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp) if (val < 0 || *endptr) { error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name, "a size value representible as a non-negative int64"); + g_free(key); return; } *obj = val; - processed(ov, name); + processed(ov, key); + g_free(key); } @@ -494,7 +574,7 @@ opts_optional(Visitor *v, bool *present, const char *name, Error **errp) /* we only support a single mandatory scalar field in a list node */ assert(ov->list_mode == LM_NONE); - *present = (lookup_distinct(ov, name, NULL) != NULL); + *present = (lookup_distinct(ov, name, NULL, NULL) != NULL); } @@ -505,6 +585,8 @@ opts_visitor_new(const QemuOpts *opts) ov = g_malloc0(sizeof *ov); + ov->nested_names = g_queue_new(); + ov->visitor.start_struct = &opts_start_struct; ov->visitor.end_struct = &opts_end_struct; @@ -545,6 +627,10 @@ opts_visitor_cleanup(OptsVisitor *ov) if (ov->unprocessed_opts != NULL) { g_hash_table_destroy(ov->unprocessed_opts); } + if (ov->opts != NULL) { + g_hash_table_destroy(ov->opts); + } + g_queue_free(ov->nested_names); g_free(ov->fake_id_opt); g_free(ov); } diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index c7eaa86..a818eff 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -81,6 +81,11 @@ { 'command': 'user_def_cmd3', 'data': {'a': 'int', '*b': 'int' }, 'returns': 'int' } +# For testing hierarchy support in opts-visitor +{ 'struct': 'UserDefOptionsSub', + 'data': { + '*nint': 'int' } } + # For testing integer range flattening in opts-visitor. The following schema # corresponds to the option format: # @@ -94,7 +99,9 @@ '*u64' : [ 'uint64' ], '*u16' : [ 'uint16' ], '*i64x': 'int' , - '*u64x': 'uint64' } } + '*u64x': 'uint64' , + 'sub0': 'UserDefOptionsSub', + 'sub1': 'UserDefOptionsSub' } } # testing event { 'struct': 'EventStructOne', diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c index ebeee5d..5862c7c 100644 --- a/tests/test-opts-visitor.c +++ b/tests/test-opts-visitor.c @@ -177,6 +177,34 @@ expect_u64_max(OptsVisitorFixture *f, gconstpointer test_data) g_assert(f->userdef->u64->value == UINT64_MAX); } +static void +expect_both(OptsVisitorFixture *f, gconstpointer test_data) +{ + expect_ok(f, test_data); + g_assert(f->userdef->sub0->has_nint); + g_assert(f->userdef->sub0->nint == 13); + g_assert(f->userdef->sub1->has_nint); + g_assert(f->userdef->sub1->nint == 13); +} + +static void +expect_sub0(OptsVisitorFixture *f, gconstpointer test_data) +{ + expect_ok(f, test_data); + g_assert(f->userdef->sub0->has_nint); + g_assert(f->userdef->sub0->nint == 13); + g_assert(!f->userdef->sub1->has_nint); +} + +static void +expect_sub1(OptsVisitorFixture *f, gconstpointer test_data) +{ + expect_ok(f, test_data); + g_assert(!f->userdef->sub0->has_nint); + g_assert(f->userdef->sub1->has_nint); + g_assert(f->userdef->sub1->nint == 13); +} + /* test cases */ int @@ -270,6 +298,12 @@ main(int argc, char **argv) add_test("/visitor/opts/i64/range/2big/full", &expect_fail, "i64=-0x8000000000000000-0x7fffffffffffffff"); + /* Test nested structs support */ + add_test("/visitor/opts/nested/unqualified", &expect_both, "nint=13"); + add_test("/visitor/opts/nested/both", &expect_both, + "sub0.nint=13,sub1.nint=13"); + add_test("/visitor/opts/nested/sub0", &expect_sub0, "sub0.nint=13"); + add_test("/visitor/opts/nested/sub1", &expect_sub1, "sub1.nint=13"); g_test_run(); return 0; }
The current OptsVisitor flattens the whole structure, if there are same named fields under different paths (like `in' and `out' in `Audiodev'), the current visitor can't cope with them (for example setting `frequency=44100' will set the in's frequency to 44100 and leave out's frequency unspecified). This patch fixes it, by the following changes: 1) Specifying just the field name will apply to all fields that has the specified name (this means it would set both in's and out's frequency to 44100 in the above example). 2) Optionally user can specify the path in the hierarchy. Names are separated by a dot (e.g. `in.frequency', `foo.bar.something', etc). The user need not specify the whole path, only the last few components (i.e. `bar.something' is equivalent to `foo.bar.something' if only `foo' has a `bar' field). This way 1) is just a special case of this when only the last component is specified. 3) In case of an ambiguity (e.g `frequency=44100,in.frequency=8000') the longest matching (the most specific) path wins (so in this example, in's frequency would become 8000, because `in.frequency' is more specific that `frequency', and out's frequency would become 44100, because only `frequency' matches it). Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com> --- qapi/opts-visitor.c | 144 +++++++++++++++++++++++++------- tests/qapi-schema/qapi-schema-test.json | 9 +- tests/test-opts-visitor.c | 34 ++++++++ 3 files changed, 157 insertions(+), 30 deletions(-)