Vladimir Sementsov-Ogievskiy <vsement...@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.
