Re: [PATCH 15/46] qemu-option: Tidy up opt_set() not to free arguments on failure

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 24.06.2020 19:43, Markus Armbruster wrote:
>> opt_set() frees its argument @value on failure.  Slightly unclean;
>> functions ideally do nothing on failure.
>>
>> To tidy this up, move opt_create() from opt_set() into its callers,
>> along with the cleanup.
>
> Hmm, let me think a bit..
>
> So, prior to this patch:
>
> opt_set gets name/value pair and sets the option in opts object, it
> seems absolutely obvious and standard behavior for Map-like object.
>
> The fact that for setting an option we create a QemuOpt object, and
> somehow register it inside opts object is an implementation detail.

You explain behavior on success.  The issue is behavior on failure.

> after the patch:
>
> opt_set gets opt object, which is already registered in opts. So,
> it seems like option is "partly" set already, and opt_set only
> finalize the processing.

Yes, opt_set() becomes a bit of a misnomer: it doesn't add a QemuOpt to
@opts, it validates a QemuOpt against its key's description, if @opts
has descriptions.

Hmm, opt_set() is now almost identical to qemu_opts_validate()'s loop
body.  Perhaps I can de-duplicate.

> And, as opt_set() only finalize the "set" operation, on opt_set
> failure we need additional roll-back of "set" operation first step.
>
> Additional fact, indirectly showing that something is unclear here
> is that we pass "opts" to opt_set twice: as "opts" parameter and
> inside opt: (opt->opts must be the same, assertion won't hurt if
> you decide to keep the patch).

Valid point.

> =====
>
> Semantics before the patch seems clearer to me.
>
> To improve the situation around "value", we can just g_strdup it
> in opt_create as well as "name" argument (and use const char*
> type for "value" argument as well)

We don't strdup() there because opts_do_parse() extracts the value with
get_opt_name(), which strdup()s it.  strdup()ing it some more isn't
wrong, I just dislike it.

Let me try the de-duplication, and then we'll see whether you still
dislike the patch.