Re: [Qemu-devel] [PATCH 04/27] vl: convert -m to qemu_opts_parse()

[Markus Armbruster]

Igor Mammedov <address@hidden> writes:

> On Wed, 27 Nov 2013 15:35:09 +0100
> Markus Armbruster <address@hidden> wrote:
>
>> Igor Mammedov <address@hidden> writes:
>> 
>> > On Tue, 26 Nov 2013 15:49:05 +0100
>> > Markus Armbruster <address@hidden> wrote:
>> >
>> >> Igor Mammedov <address@hidden> writes:
>> >> 
>> >> > On Thu, 21 Nov 2013 11:12:43 +0100
>> >> > Markus Armbruster <address@hidden> wrote:
>> >> >
>> >> >> Igor Mammedov <address@hidden> 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.

Need to review your latest to have an opinion here :)

>> 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"

Works for me.

>> > [...]
>> >> >> 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.

qemu_get_machine_opts() is precedent for factoring out common
opts-querying code.

It has one aspect that may have been a bad idea: it creates empty
machine opts when the user didn't specify any option creating machine
opts.

Hope this unconfuses you.  If not, I guess we should just move on to
discussing your revised patch :)

>> >  * 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.

Okay.