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
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?
[...]
> >> 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.
* 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.
* 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.
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.
Doing
"git grep qemu_get_machine_opts"
gets us several precedents that already use it that way.
* 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.
--
Regards,
Igor
