[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 05/17] qapi: introduce OptsVisitor
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [PATCH v2 05/17] qapi: introduce OptsVisitor |
Date: |
Sat, 14 Jul 2012 00:48:58 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:10.0.5) Gecko/20120601 Thunderbird/10.0.5 |
On 07/13/12 18:51, Luiz Capitulino wrote:
> On Wed, 13 Jun 2012 10:22:36 +0200
> Laszlo Ersek <address@hidden> wrote:
>> Repeating an optarg is supported;
>
> I see that the current code supports this too, but why? Something
> like this should fail:
>
> -netdev type=tap,vhost=on,vhost=off,id=guest1,script=qemu-ifup-switch
> Also, you're using a queue to support the repeating of optargs,
> right? I think this could be simplified if we just don't support
> that.
I hate repeated options with a passion, but SLIRP's hostfwd and guestfwd
depend on repetition.
When the outermost opts_start_struct() is invoked and I shovel the
optargs into the queues, I can't yet know what's going to be used in
repeated form and what not.
If you prefer I can change lookup_scalar() as follows. For reference:
>> +static GQueue *
>> +lookup_distinct(const OptsVisitor *ov, const char *name, Error **errp)
>> +{
>> + GQueue *list;
>> +
>> + list = g_hash_table_lookup(ov->unprocessed_opts, name);
>> + if (!list) {
>> + error_set(errp, QERR_MISSING_PARAMETER, name);
>> + }
>> + return list;
>> +}
>> +static void
>> +opts_start_optional(Visitor *v, bool *present, const char *name,
>> + Error **errp)
>> +{
>> + OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
>> +
>> + /* we only support a single mandatory scalar field in a list node */
>> + assert(ov->repeated_opts == NULL);
>> + *present = (lookup_distinct(ov, name, NULL) != NULL);
>> +}
>> +static const QemuOpt *
>> +lookup_scalar(const OptsVisitor *ov, const char *name, Error **errp)
>> +{
>> + if (ov->repeated_opts == NULL) {
>> + GQueue *list;
>> +
>> + /* the last occurrence of any QemuOpt takes effect when queried by
>> name
>> + */
>> + list = lookup_distinct(ov, name, errp;
>> + return list ? g_queue_peek_tail(list) : NULL;
We're outside of list traversal in this branch, meaning the optarg is
allowed exactly once. (Optional optargs are first handled by
opts_start_optional().) If lookup_distinct() succeeds here, then rather
than returning the last occurrence, I could check the depth of the queue
(== 1 or > 1), and set an error for > 1.
However QemuOpts definitely supports repeated optargs now (otherwise
slirp hostfwd/guestfwd wouldn't work). qemu_opt_foreach() is used for
iteration (with QTAILQ_FOREACH()), while qemu_opt_find() -- and thus its
direct callers -- rely on QTAILQ_FOREACH_REVERSE() and the first match.
Optargs of an option are apparently chained like this:
qemu_opts_parse() [qemu-option.c]
opts_parse(..., defaults=false)
opts_do_parse(..., prepend=false)
opt_set(..., prepend=false, ...)
QTAILQ_INSERT_TAIL()
"-option arg=val1,arg=val2,arg=val3" is therefore linked into the
corresponding QemuOpts instance in the same order, and qemu_opt_find()
will return "arg=val3". I also use g_queue_push_tail() and
g_queue_peek_tail(), so I think we're compatible.
>> + }
>> + return g_queue_peek_head(ov->repeated_opts);
>> +}
Continuing slightly out of order:
>> +/* mimics qemu-option.c::parse_option_bool() */
>> +static void
>> +opts_type_bool(Visitor *v, bool *obj, const char *name, Error **errp)
>> +{
>> + OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
>> + const QemuOpt *opt;
>> +
>> + opt = lookup_scalar(ov, name, errp);
>> + if (!opt) {
>> + return;
>> + }
>> +
>> + if (opt->str) {
>> + if (strcmp(opt->str, "on") == 0 ||
>> + strcmp(opt->str, "yes") == 0 ||
>> + strcmp(opt->str, "y") == 0) {
>> + *obj = true;
>> + } else if (strcmp(opt->str, "off") == 0 ||
>> + strcmp(opt->str, "no") == 0 ||
>> + strcmp(opt->str, "n") == 0) {
>> + *obj = false;
>
> The current code only accepts 'on' or 'off', no reason to change that.
>
>> + } else {
>> + error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
>> + "on|yes|y|off|no|n");
>> + return;
>> + }
>> + } else {
>> + *obj = true;
>> + }
>> +
>> + processed(ov, name);
>> +}
This function is used for "bool" generally. The following optargs were
all unified as "bool":
- slirp/restrict: originally QEMU_OPT_STRING, net_init_slirp() accepting
all of "on|yes|y|off|no|n"
- tap/vnet_hdr: originally QEMU_OPT_BOOL, parse_option_bool() accepting
"on|off".
- tap/vhost: ditto
- tap/vhostforce: ditto
So I took the union (nothing should break that used to work).
The leading comment rather means that the structure of
parse_option_bool() is followed:
- optarg values meaning "true": true
- optarg values meaning "false": false
- other optarg values: error
- no optarg value at all: true
>> +static void
>> +opts_start_struct(Visitor *v, void **obj, const char *kind,
>> + const char *name, size_t size, Error **errp)
>> +{
>> + OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
>> + const QemuOpt *opt;
>> +
>> + *obj = g_malloc0(size);
>> + if (ov->depth++ > 0) {
>> + return;
>> + }
>> +
>> + ov->unprocessed_opts = g_hash_table_new_full(&g_str_hash, &g_str_equal,
>> + NULL, &destroy_list);
>> + QTAILQ_FOREACH(opt, &ov->opts_root->head, next) {
>> + GQueue *list;
>> +
>> + list = g_hash_table_lookup(ov->unprocessed_opts, opt->name);
>> + if (list == NULL) {
>> + list = g_queue_new();
>> +
>> + /* GHashTable will never try to free the keys -- we supplied
>> NULL
>> + * as "key_destroy_func" above. Thus cast away key const-ness in
>> + * order to suppress gcc's warning. */
>> + g_hash_table_insert(ov->unprocessed_opts, (gpointer)opt->name,
>> + list);
>> + }
>> +
>> + /* Similarly, destroy_list() doesn't call g_queue_free_full(). */
>> + g_queue_push_tail(list, (gpointer)opt);
>> + }
>> +}
>
> This doesn't insert the opts id into the hash, so opts_type_str()
> will fail to find the id when the generated code visits it here:
>
> void visit_type_Netdev(Visitor *m, Netdev ** obj, const char *name, Error
> **errp)
> {
> if (!error_is_set(errp)) {
> Error *err = NULL;
> visit_start_struct(m, (void **)obj, "Netdev", name, sizeof(Netdev),
> &err);
> if (!err) {
> assert(!obj || *obj);
> visit_type_str(m, obj ? &(*obj)->id : NULL, "id", &err);
> <---------
> [...]
>
*groan*
You're right. opts_do_parse() makes an exception with "id" and doesn't
call opt_set() for any occurrence of it. Would you accept the attached
fix, split up and squashed into previous parts appropriately?
Thanks!
Laszlo
0001-support-NetLegacy-id-and-clean-up-QemuOpts-id-usage.patch
Description: Text document