[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH for-5.1 2/8] qemu-options: Factor out get_opt_name_value() he
From: |
Markus Armbruster |
Subject: |
Re: [PATCH for-5.1 2/8] qemu-options: Factor out get_opt_name_value() helper |
Date: |
Wed, 15 Apr 2020 09:03:29 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) |
Kevin Wolf <address@hidden> writes:
> Am 14.04.2020 um 11:42 hat Markus Armbruster geschrieben:
>> Eric Blake <address@hidden> writes:
>>
>> > On 4/9/20 10:30 AM, Markus Armbruster wrote:
>> >> The next commits will put it to use.
>> >>
>> >> Signed-off-by: Markus Armbruster <address@hidden>
>> >> ---
>> >> util/qemu-option.c | 102 +++++++++++++++++++++++++--------------------
>> >> 1 file changed, 56 insertions(+), 46 deletions(-)
>> >>
>> >
>> >> +static const char *get_opt_name_value(const char *params,
>> >> + const char *firstname,
>> >> + char **name, char **value)
>> >> +{
>> >> + const char *p, *pe, *pc;
>> >> +
>> >> + pe = strchr(params, '=');
>> >> + pc = strchr(params, ',');
>> >> +
>> >> + if (!pe || (pc && pc < pe)) {
>> >> + /* found "foo,more" */
>> >> + if (firstname) {
>> >> + /* implicitly named first option */
>> >> + *name = g_strdup(firstname);
>> >> + p = get_opt_value(params, value);
>> >
>> > Is this correct even when params is "foo,,more"? But...
>> >
>> >> static void opts_do_parse(QemuOpts *opts, const char *params,
>> >> const char *firstname, bool prepend,
>> >> bool *invalidp, Error **errp)
>> >> {
>> >> - char *option = NULL;
>> >> - char *value = NULL;
>> >> - const char *p,*pe,*pc;
>> >> Error *local_err = NULL;
>> >> + char *option, *value;
>> >> + const char *p;
>> >> - for (p = params; *p != '\0'; p++) {
>> >> - pe = strchr(p, '=');
>> >> - pc = strchr(p, ',');
>> >> - if (!pe || (pc && pc < pe)) {
>> >> - /* found "foo,more" */
>> >> - if (p == params && firstname) {
>> >> - /* implicitly named first option */
>> >> - option = g_strdup(firstname);
>> >> - p = get_opt_value(p, &value);
>> >
>> > ...in this patch, it is just code motion, so if it is a bug, it's
>> > pre-existing and worth a separate fix.
>>
>> If @p points to "foo,,more", possibly followed by additional characters,
>> then we have pc && pc < pe, and enter this conditional. This means that
>>
>> foo,,more=42
>>
>> gets parsed as two name=value, either as
>>
>> name value
>> -----------------------
>> @firstname "foo,more"
>> "" "42"
>>
>> if @firstname is non-null
>
> Hm, how that? get_opt_value() doesn't stop at '=', so shouldn't you get
> a single option @firstname with a value of "foo,more=42"?
>
> name value
> -----------------------
> @firstname "foo,more=42"
>
>> or else as
>>
>> name value
>> -----------------
>> "foo,,more" "on"
>> "" "42"
>
> Here get_opt_name() stops at the first comma. You get a positive flag
> "foo" and the remaing option string starts with a comma, so the
> condition will still be true for the next loop iteration. Now you get a
> positive flag with an empty name "". Finally, the rest is parsed as an
> option, so we get:
>
> name value
> -----------------------
> "foo" "on"
> "" "on"
> "more" "42"
>
> Actually, at this point I thought I could just check, and gdb confirms
> my reading for both cases.
You're right. I should know better than trying to predict what the
QemuOpts parser does.
> This is still crazy, but perhaps less so than the interpretations you
> suggested.
Permitting anti-social characters in the NAME part of NAME=VALUE is
crazy. To findout which kind of crazy exactly, run the code and
observe. Do not blindly trust the maintainer's explanations, because
he's quite possibly just as confused as everybody else.
- [PATCH for-5.1 8/8] qemu-option: Move is_valid_option_list() to qemu-img.c and rewrite, (continued)
[PATCH for-5.1 2/8] qemu-options: Factor out get_opt_name_value() helper, Markus Armbruster, 2020/04/09
Re: [PATCH for-5.1 2/8] qemu-options: Factor out get_opt_name_value() helper, Kevin Wolf, 2020/04/14
[PATCH for-5.1 7/8] qemu-img: Factor out accumulate_options() helper, Markus Armbruster, 2020/04/09
[PATCH for-5.1 5/8] qemu-option: Fix has_help_option()'s sloppy parsing, Markus Armbruster, 2020/04/09
[PATCH for-5.1 1/8] tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt(), Markus Armbruster, 2020/04/09