From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54581) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VjrME-0005sV-6l for qemu-devel@nongnu.org; Fri, 22 Nov 2013 09:05:24 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VjrM8-00020n-6V for qemu-devel@nongnu.org; Fri, 22 Nov 2013 09:05:18 -0500 Received: from mx1.redhat.com ([209.132.183.28]:62435) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VjrM7-0001zq-Up for qemu-devel@nongnu.org; Fri, 22 Nov 2013 09:05:12 -0500 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rAME58FF016430 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 22 Nov 2013 09:05:09 -0500 Message-ID: <528F64A9.9010008@redhat.com> Date: Fri, 22 Nov 2013 15:05:29 +0100 From: Max Reitz MIME-Version: 1.0 References: <1385060754-18821-1-git-send-email-mreitz@redhat.com> <1385060754-18821-4-git-send-email-mreitz@redhat.com> <528F10F1.5090300@redhat.com> In-Reply-To: <528F10F1.5090300@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 3/6] qemu-option: Add qemu_config_parse_qdict() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng , qemu-devel@nongnu.org Cc: Kevin Wolf , Stefan Hajnoczi On 22.11.2013 09:08, Fam Zheng wrote: > On 2013=E5=B9=B411=E6=9C=8822=E6=97=A5 03:05, Max Reitz wrote: >> This function basically parses command-line options given as a QDict >> replacing a config file. >> >> For instance, the QDict {"section.opt1": 42, "section.opt2": 23} >> corresponds to the config file: >> >> [section] >> opt1 =3D 42 >> opt2 =3D 23 >> >> It is possible to specify multiple sections and also multiple sections >> of the same type. On the command line, this looks like the following: >> >> inject-error.0.event=3Dreftable_load,\ >> inject-error.1.event=3Dl2_load,\ >> set-state.event=3Dl1_update >> >> This would correspond to the following config file: >> >> [inject-error "inject-error.0"] >> event =3D reftable_load >> >> [inject-error "inject-error.1"] >> event =3D l2_load >> >> [set-state] >> event =3D l1_update >> >> Signed-off-by: Max Reitz >> --- >> include/qemu/config-file.h | 6 +++ >> util/qemu-config.c | 111=20 >> +++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 117 insertions(+) >> >> diff --git a/include/qemu/config-file.h b/include/qemu/config-file.h >> index 508428f..dbd97c4 100644 >> --- a/include/qemu/config-file.h >> +++ b/include/qemu/config-file.h >> @@ -4,6 +4,7 @@ >> #include >> #include "qemu/option.h" >> #include "qapi/error.h" >> +#include "qapi/qmp/qdict.h" >> >> QemuOptsList *qemu_find_opts(const char *group); >> QemuOptsList *qemu_find_opts_err(const char *group, Error **errp); >> @@ -18,6 +19,11 @@ int qemu_config_parse(FILE *fp, QemuOptsList=20 >> **lists, const char *fname); >> >> int qemu_read_config_file(const char *filename); >> >> +/* Parse QDict options as a replacement for a config file (allowing=20 >> multiple >> + enumerated (0..(n-1)) configuration "sections") */ >> +void qemu_config_parse_qdict(QDict *options, QemuOptsList **lists, >> + Error **errp); >> + >> /* Read default QEMU config files >> */ >> int qemu_read_default_config_files(bool userconfig); >> diff --git a/util/qemu-config.c b/util/qemu-config.c >> index 04da942..80d82d4 100644 >> --- a/util/qemu-config.c >> +++ b/util/qemu-config.c >> @@ -356,3 +356,114 @@ int qemu_read_config_file(const char *filename) >> return -EINVAL; >> } >> } >> + >> +static void config_parse_qdict_section(QDict *options, QemuOptsList=20 >> *opts, >> + Error **errp) >> +{ >> + QemuOpts *subopts, *subopts_i; >> + QDict *subqdict, *subqdict_i =3D NULL; >> + char *prefix =3D g_strdup_printf("%s.", opts->name); >> + Error *local_err =3D NULL; >> + size_t orig_size, enum_size; >> + >> + qdict_extract_subqdict(options, &subqdict, prefix); >> + orig_size =3D qdict_size(subqdict); >> + if (!orig_size) { >> + goto out; >> + } >> + >> + subopts =3D qemu_opts_create_nofail(opts); >> + qemu_opts_absorb_qdict(subopts, subqdict, &local_err); >> + if (error_is_set(&local_err)) { >> + error_propagate(errp, local_err); >> + goto out; >> + } >> + >> + enum_size =3D qdict_size(subqdict); >> + if (enum_size < orig_size && enum_size) { >> + error_setg(errp, "Unknown option '%s' for '%s'", >> + qdict_first(subqdict)->key, opts->name); >> + goto out; >> + } >> + >> + if (enum_size) { >> + /* Multiple, enumerated rules */ >> + int i; >> + >> + /* Not required anymore */ >> + qemu_opts_del(subopts); >> + >> + for (i =3D 0; i < INT_MAX; i++) { >> + char i_prefix[32], opt_name[48]; >> + size_t snprintf_ret; >> + >> + snprintf_ret =3D snprintf(i_prefix, 32, "%i.", i); >> + assert(snprintf_ret < 32); >> + >> + snprintf_ret =3D snprintf(opt_name, 48, "%s.%i",=20 >> opts->name, i); >> + assert(snprintf_ret < 48); > > Is there a length limit of opts->name? I.e. is this an error case=20 > rather than an assertion case? Hm, yes, I had most of the code directly in block/blkdebug.c at first,=20 that's why there are some issues here which work fine just for blkdebug,=20 but are not that good in the general case. I'll use g_strdup_sprintf=20 instead. > >> + >> + qdict_extract_subqdict(subqdict, &subqdict_i, i_prefix); >> + if (!qdict_size(subqdict_i)) { >> + break; >> + } >> + >> + subopts_i =3D qemu_opts_create(opts, opt_name, 1, NULL); > > Could pass in errp and skip error_setg below. Oh, right, missed that, thanks. > >> + if (!subopts_i) { >> + error_setg(errp, "Option ID '%s' already in use",=20 >> opt_name); >> + goto out; >> + } >> + >> + qemu_opts_absorb_qdict(subopts_i, subqdict_i, &local_err)= ; >> + if (error_is_set(&local_err)) { >> + error_propagate(errp, local_err); >> + qemu_opts_del(subopts_i); >> + goto out; >> + } >> + >> + if (qdict_size(subqdict_i)) { >> + error_setg(errp, "[%s] rules don't support the=20 >> option '%s'", > > s/rules don't/section doesn't/ Yes, this one also is fine for blkdebug, but not so much for the general=20 case. > >> + opts->name, qdict_first(subqdict_i)->key); >> + qemu_opts_del(subopts_i); >> + goto out; >> + } >> + >> + /* >> + if (add_rule(subopts_i, (void *)ard) < 0) { >> + error_setg(errp, "Could not add [%s] rule %i",=20 >> opts->name, i); > > s/rule/section/ > Or just remove this hunk? Hm, I probably should have completely reviewed my own code before=20 sending it. Will do that in v2, promise. ;-) > >> + goto out; >> + } >> + */ >> + >> + QDECREF(subqdict_i); >> + subqdict_i =3D NULL; >> + } >> + >> + if (qdict_size(subqdict)) { >> + error_setg(errp, "Unused option '%s' for [%s]", >> + qdict_first(subqdict)->key, opts->name); >> + goto out; >> + } >> + } >> + >> +out: >> + QDECREF(subqdict); >> + QDECREF(subqdict_i); >> + >> + free(prefix); >> +} >> + >> +void qemu_config_parse_qdict(QDict *options, QemuOptsList **lists, >> + Error **errp) >> +{ >> + int i; >> + Error *local_err =3D NULL; >> + >> + for (i =3D 0; lists[i]; i++) { >> + config_parse_qdict_section(options, lists[i], &local_err); >> + if (error_is_set(&local_err)) { >> + error_propagate(errp, local_err); >> + return; >> + } >> + } >> +} >> > > This does the work, but also seems ad-hoc. Providing an array of dicts=20 > in the command line should be a general logic, which would also be=20 > very useful for Quorum driver. Yes, I thought about quorum, too. I don't think just using QemuOpts=20 there would be too bad, since the above function could do all the work=20 there as well. I could probably introduce a QDict function which splits=20 a QDict with keys enumerated in this way into a QList of QDicts. I'll=20 have a look into how much more complex that will be and, depending on=20 that, include it in v2 or not. Thanks for reviewing, Max