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

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 26 Nov 2013 15:49:05 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Igor Mammedov <imammedo@redhat.com> writes:
> 
> > On Thu, 21 Nov 2013 11:12:43 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Igor Mammedov <imammedo@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