On Wed, 27 Nov 2013 15:35:09 +0100
Markus Armbruster <arm...@redhat.com> wrote:
> 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?
replacing one one-liner with another might help a little but
won't change a thing in general. It will be essentially the same.
>
> 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?
I've replaced it with "mem_str" see
"[PATCH 04/28] vl: convert -m to QemuOpts"
>
> > [...]
> >> >> 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've dropped completely defaults setting in QemuOpts please see:
"[PATCH 04/28] vl: convert -m to QemuOpts"
"[PATCH 05/28] vl.c: extend -m option to support options for memory hotplug"
As for ">it only creates empty opts." I'm confused.
qemu_opt_get(qemu_get_machine_opts(), "foo") pattern showed by grep is the
same
as I use to get "slots/maxmem":
exec.c: if (!qemu_opt_get_bool(qemu_get_machine_opts(), "mem-merge", true)) {
hw/arm/boot.c: info->dtb_filename = qemu_opt_get(qemu_get_machine_opts(),
"dtb");
hw/ppc/spapr.c: const char *drivename =
qemu_opt_get(qemu_get_machine_opts(), "nvram");
hw/ppc/virtex_ml507.c: dtb_filename = qemu_opt_get(qemu_get_machine_opts(),
"dtb");
include/sysemu/sysemu.h:QemuOpts *qemu_get_machine_opts(void);
kvm-all.c: if (!qemu_opt_get_bool(qemu_get_machine_opts(), "kernel_irqchip",
true) ||
target-i386/kvm.c: shadow_mem = qemu_opt_get_size(qemu_get_machine_opts(),
probably it is there because passing them as C parameters is more intrusive
than
just using user supplied values directly.
> > * 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.
I can easily drop this hunk from "[PATCH 04/28] vl: convert -m to QemuOpts",
I've posted tonight as reply to this thread,
since ram_size is already passed to machine_init(), it's not worth arguing.
