Igor Mammedov <imamm...@redhat.com> writes: > On Tue, 26 Nov 2013 15:49:05 +0100 > Markus Armbruster <arm...@redhat.com> wrote: > >> Igor Mammedov <imamm...@redhat.com> writes: >> >> > On Thu, 21 Nov 2013 11:12:43 +0100 >> > Markus Armbruster <arm...@redhat.com> wrote: >> > >> >> Igor Mammedov <imamm...@redhat.com> writes: >> >> > [...] >> Two separate issues here: >> >> 1. The "no qemu_mem_opts have been specified" case >> >> This is equivalent to "empty options". Therefore, the case can be >> eliminated by pre-creating empty options. No objection. >> >> The three existing merge_lists users don't do that. Perhaps they >> should. >> >> 2. How to provide default values >> >> Supplying defaults is left to the caller of qemu_opt_get_FOO() by >> design. >> >> Pre-creating option parameters deviates from that pattern. You >> justify it by saying it "eliminates need to pepper code with >> DEFAULT_RAM_SIZE * 1024 * 1024". How many occurrences? > beside of vl.c for: > mem & maxmem 1 in hw/i386/pc.c > slots - 6 in several files
Could the common code be factored out the old-fashioned way? Precedence: qemu_get_machine_opts() encapsulates some QemuOpts-related details, so its many users don't have to deal with them. > see below for continuation: > >> >> Drawback: you lose the ability to see whether the user gave a value. >> See below. >> > [...] >> >> Ugly. >> >> >> >> Why is the variable called 'end'? >> > would be 'suffix' better? >> >> end points to the whole value string, not the end of anything, and >> neither to a suffix of anything. > Any suggestions? What about val? > [...] >> >> If you refrain from putting defaults into opts, you can distinguish the >> >> cases "user didn't specify maxmem, so assume mem", and "user specified >> >> maxmem, so check it's >= mem". >> > So foar, there is no point in distinguishing above cases, >> > since maxmem <= mem is invalid value and hotplug should be disabled. >> > So setting default maxmem to mem or anything less effectively >> > disables hotplug. >> >> Yes, setting maxmem < mem is invalid and should be rejected, but not >> setting maxmem at all should be accepted even when you set mem. >> >> Your patch like this pseudo-code: >> >> mem = DEFAULT_RAM_SIZE * 1024 * 1024 >> maxmem = mem >> >> if user specifies mem: >> mem = user's mem >> if user specifes max-mem: >> mem = user's max-mem >> >> if max-mem < mem >> what now? >> should error our if max-mem and mem were specified by the user >> shouldn't if user didn't specify max-mem! >> but can't say whether he did >> >> I'd do it this way: >> >> mem = unset >> maxmem = unset >> >> if user specifies mem: >> mem = user's mem >> if user specifes max-mem: >> mem = user's max-mem >> >> if mem != unset && max-mem != unset && max-mem < mem >> error >> >> I'd use QemuOpts for the user's command line, and no more. For anything >> beyond that, I'd use ordinary variables, such as ram_size. > Ok, I'll revert to the old code where options users check for option > availability, it's not that much code. > > > As for using QemuOpts as global store for global variables: > > * using local variables would require changing of machine init or/and > QEMUMachine and changing functions signature pass them down the stack to > consumers. Extending QEMUMachineInitArgs should suffice. Once you're inside the board code, passing stuff around as C parameters is probably an improvement over passing around QemuOpts. > * adding "slots" readonly property to i440fx & q35 for consumption in > ACPI hotplug code and building ACPI tables. It would be essentially another > global lookup for i440fx & q35 object and pulling "slots" property, > which is much longer way/complex way to get global value. That's a lot of > boilerplate code for the same outcome. Can't say without seeing the code. > * about setting default for "mem" value: if default "mem" is not set and > no -m is provided on CLI, we get case where > ram_size = foo & "mem" unset > And if I recall correctly there was an effort to provide interface for > currently used QemuOpts to external users. So "mem" would get inconsistent > with what QEMU uses. QemuOpts do not record what QEMU uses. They record what the user asked for. > To sum up above said: > * I'd like to continue using QemuOpts as global constant value store, it > saves from adding a lot of boilerplate-code that would do the same. Keeping the user's configuration just in QemuOpts is fine. What I don't like is messing with it there. This includes storing defaults. Here's another reason: -writeconfig should write out exactly the user's configuration. If you mess with it, it may write out messed up configuration, depending on *when* you mess with it. > Doing > "git grep qemu_get_machine_opts" > gets us several precedents that already use it that way. Note that it does *not* store defaults in QemuOpts, it only creates empty opts. I'm not sure that was a good idea. > * I believe that setting default in QemuOpts for "mem" is a good thing that > leads to consistent meaning of "mem" with what QEMU actually uses. I'm not sure I got this argument.
