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

[Markus Armbruster]

Igor Mammedov <address@hidden> writes:

> On Thu, 21 Nov 2013 11:12:43 +0100
> Markus Armbruster <address@hidden> wrote:
>
>> Igor Mammedov <address@hidden> writes:
>> 
>> > Along with conversion extend -m option to support following parameters:
>> 
>> Please split this into two patches: first conversion to QemuOpts, then
>> extension.
>> 
>> >   "mem" - startup memory amount
>> >   "slots" - total number of hotplug memory slots
>> >   "maxmem" - maximum possible memory
>> >
>> > "slots" and "maxmem" should go in pair and "maxmem" should be greater
>> > than "mem" for memory hotplug to be usable.
>> >
>> > v2:
>> >   make sure maxmem is not less than ram size
>> >
>> > Signed-off-by: Igor Mammedov <address@hidden>
>> > ---
>> >  qemu-options.hx |    9 +++++-
>> >  vl.c            |   73 
>> > ++++++++++++++++++++++++++++++++++++++++++++++---------
>> >  2 files changed, 68 insertions(+), 14 deletions(-)
>> >
>> > diff --git a/qemu-options.hx b/qemu-options.hx
>> > index 8b94264..fe4559b 100644
>> > --- a/qemu-options.hx
>> > +++ b/qemu-options.hx
>> > @@ -210,8 +210,13 @@ use is discouraged as it may be removed from future 
>> > versions.
>> >  ETEXI
>> >  
>> >  DEF("m", HAS_ARG, QEMU_OPTION_m,
>> > -    "-m megs         set virtual RAM size to megs MB [default="
>> > -    stringify(DEFAULT_RAM_SIZE) "]\n", QEMU_ARCH_ALL)
>> > +    "-m [mem=]megs[,slots=n,maxmem=size]\n"
>> > +    "                set virtual RAM size to megs MB [default="
>> > +    stringify(DEFAULT_RAM_SIZE) "]\n"
>> > +    "                mem=start-up memory amount\n"
>> > +    "                slots=maximum number of hotplug slots\n"
>> > +    "                maxmem=maximum total amount of memory\n",
>> > +    QEMU_ARCH_ALL)
>> 
>> Help text is confusing, because it discusses megs twice.  Fits right in,
>> as output of -help is generally confusing (to put it politely).
>> 
>> What about something like this:
>> 
>> -m [mem=]megs[,slots=n,maxmem=size]
>>                 configure guest RAM
>>                 mem: initial amount of guest memory (default: XXX)
>>                 slots: number of hotplug slots (default: none)
>>                 maxmem: maximum amount of guest memory (default: mem)
> Sure, I'll fix it.
>
>> >  STEXI
>> >  @item -m @var{megs}
>> >  @findex -m
>> > diff --git a/vl.c b/vl.c
>> > index f28674f..5974f0f 100644
>> > --- a/vl.c
>> > +++ b/vl.c
>> > @@ -529,6 +529,28 @@ static QemuOptsList qemu_msg_opts = {
>> >      },
>> >  };
>> >  
>> > +static QemuOptsList qemu_mem_opts = {
>> > +    .name = "memory-opts",
>> > +    .implied_opt_name = "mem",
>> > +    .head = QTAILQ_HEAD_INITIALIZER(qemu_mem_opts.head),
>> > +    .merge_lists = true,
>> 
>> Yes, because multiple -m need to accumulate.
>> 
>> > +    .desc = {
>> > +        {
>> > +            .name = "mem",
>> > +            .type = QEMU_OPT_SIZE,
>> > +        },
>> > +        {
>> > +            .name = "slots",
>> > +            .type = QEMU_OPT_NUMBER,
>> > +        },
>> > +        {
>> > +            .name = "maxmem",
>> > +            .type = QEMU_OPT_SIZE,
>> > +        },
>> > +        { /* end of list */ }
>> > +    },
>> > +};
>> > +
>> >  /**
>> >   * Get machine options
>> >   *
>> > @@ -2816,6 +2838,14 @@ static int object_create(QemuOpts *opts, void 
>> > *opaque)
>> >      return 0;
>> >  }
>> >  
>> > +static void qemu_init_default_mem_opts(uint64_t size)
>> > +{
>> > +    QemuOpts *opts = qemu_opts_create_nofail(&qemu_mem_opts);
>> > +    qemu_opt_set_number(opts, "mem", size);
>> > +    qemu_opt_set_number(opts, "maxmem", size);
>> > +    qemu_opt_set_number(opts, "slots", 0);
>> > +}
>> > +
>> 
>> We usually don't put defaults in QemuOpts.  Instead, the code querying
>> QemuOpts detects and handles absence of the parameter.  Can be as simple
>> as qemu_opt_get_size(opts, "mem", DEFAULT_RAM_SIZE * 1024 * 1024).
> It allows to make number of uses a bit simpler:
>
> for example v6:
>     QemuOpts *opts = qemu_opts_find(qemu_find_opts("memory-opts"), NULL);
>     if (!opts) { /* no -m x,... was passed to cmd line so no mem hotplug */
>         return;
>     }
>     mem_st->dev_count = qemu_opt_get_number(opts, "slots", 0);
>
> becomes"
>     QemuOpts *opts = qemu_opts_find(qemu_find_opts("memory-opts"), NULL);
>     state->dev_count = qemu_opt_get_number(opts, "slots", 0);
>     
> and eliminates need to pepper code with DEFAULT_RAM_SIZE * 1024 * 1024 or
> like constants wherever qemu_opt_get_..() is called, that was main reason
> to set defaults. i.e. set defaults only once instead of spreading them in
> every place qemu_opt_get_..() is called.

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?

   Drawback: you lose the ability to see whether the user gave a value.
   See below.

>> See also below.
>> 
>> >  int main(int argc, char **argv, char **envp)
>> >  {
>> >      int i;
>> > @@ -2887,6 +2917,7 @@ int main(int argc, char **argv, char **envp)
>> >      qemu_add_opts(&qemu_tpmdev_opts);
>> >      qemu_add_opts(&qemu_realtime_opts);
>> >      qemu_add_opts(&qemu_msg_opts);
>> > +    qemu_add_opts(&qemu_mem_opts);
>> >  
>> >      runstate_init();
>> >  
>> > @@ -2901,7 +2932,8 @@ int main(int argc, char **argv, char **envp)
>> >      module_call_init(MODULE_INIT_MACHINE);
>> >      machine = find_default_machine();
>> >      cpu_model = NULL;
>> > -    ram_size = 0;
>> > +    ram_size = DEFAULT_RAM_SIZE * 1024 * 1024;
>> > +    qemu_init_default_mem_opts(ram_size);
>> >      snapshot = 0;
>> >      cyls = heads = secs = 0;
>> >      translation = BIOS_ATA_TRANSLATION_AUTO;
>> > @@ -3178,21 +3210,43 @@ int main(int argc, char **argv, char **envp)
>> >                  exit(0);
>> >                  break;
>> >              case QEMU_OPTION_m: {
>> > -                int64_t value;
>> >                  uint64_t sz;
>> > -                char *end;
>> > +                const char *end;
>> > +                char *s;
>> >  
>> > -                value = strtosz(optarg, &end);
>> > -                if (value < 0 || *end) {
>> > -                    fprintf(stderr, "qemu: invalid ram size: %s\n", 
>> > optarg);
>> > +                opts = qemu_opts_parse(qemu_find_opts("memory-opts"),
>> > +                                       optarg, 1);
>> > +                if (!opts) {
>> >                      exit(1);
>> >                  }
>> > -                sz = QEMU_ALIGN_UP((uint64_t)value, 8192);
>> > +
>> > +                /* fixup legacy sugffix-less format */
>> 
>> /* fix up legacy suffix-less format */
>> 
>> The problem here is that OPT_SIZE treats values without a size suffix as
>> bytes, but we need to default to MiB for backward compatibility.
>> 
>> > +                end = qemu_opt_get(opts, "mem");
>> > +                if (g_ascii_isdigit(end[strlen(end) - 1])) {
>> > +                    s = g_strconcat(end, "M", NULL);
>> > +                    qemu_opt_set(opts, "mem", s);
>> > +                    g_free(s);
>> > +                }
>> 
>> 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.

>> qemu_opt_set() appends to the list of options.  The un-fixed-up option
>> remains in the list.  qemu_opt_unset() could fix that, but it asserts
>> opts_accepts_any() for unclear reasons.  git-blame points to Kevin.
> it would be cleaner to unset it but it works event without unsetting it,
> since qemu_opt_set...() adds to tail and qemu_opt_get...() finds options
> from tail to head.
>
> Nevertheless, Kevin do you recall reasons for assert in 0dd6c526:
>
> int qemu_opt_unset(QemuOpts *opts, const char *name)
> ...
>     assert(opts_accepts_any(opts));
>
> would it be ok if I remove it?
>
>> 
>> Have you considered qemu_opt_set_number()?
> it was code left from v6 when I didn't know about it. Sorry, I'll
> use it instead.
>
>> 
>> If this "need a default suffix" pattern exists elsewhere, we should
>> consider extending QemuOptDesc to cover it.
> I haven't seen/don't recall need for it anywhere else, but it would be
> cleanest solution. But it would introduce temptation for users
> to shrug off suffixes which is wrong in my opinion. -m 1024 is confusing
> unless you know that it's in Mb.
>
> it would be better not to introduce mechanism, that would allow to do such
> thing in favor of explicit suffixes.

I'm afraid that horse as left the barn long ago:

* -numa mem=VAL accepts an optional suffix, defaulting it to 'M'.

* Likewise, HMP commands block_resize, block_stream,
  block_job_set_speed, migrate_set_cache_size, migrate_set_speed.

But point taken.

>> > +
>> > + sz = QEMU_ALIGN_UP(qemu_opt_get_size(opts, "mem", ram_size),
>> > +                                   8192);
>> > +                /* compatibility behaviour for case "-m 0" */
>> > +                if (sz == 0) {
>> > +                    sz = DEFAULT_RAM_SIZE * 1024 * 1024;
>> > +                }
>> > +
>> 
>> Yes, this is needed.  Our command line is bonkers.
>> 
>> >                  ram_size = sz;
>> >                  if (ram_size != sz) {
>> >                      fprintf(stderr, "qemu: ram size too large\n");
>> >                      exit(1);
>> >                  }
>> > +                /* store aligned value for future use */
>> > +                qemu_opt_set_number(opts, "mem", ram_size);
>> 
>> Here, you use qemu_opt_set_number().
>> 
>> Again, this appends to the list, and leaves the non-aligned value in.
> it's not an issue since the last appended opt is used in qemu_opt_get_size().
>
>> 
>> > +
>> > +                sz = qemu_opt_get_size(opts, "maxmem", ram_size);
>> > +                if (sz < ram_size) {
>> > +                    qemu_opt_set_number(opts, "maxmem", ram_size);
>> > +                }
>> >                  break;
>> >              }
>> 
>> Looks like you want to fix up something like "-m 1024", so that maxmem
>> stays equal to mem.  I'm afraid you also "fix" user errors like "-m
>> mem=1024M,maxmem=512M".
> Perhaps it would be better to bail out with error here.
>
>> 
>> 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.

>> >  #ifdef CONFIG_TPM
>> > @@ -4029,11 +4083,6 @@ int main(int argc, char **argv, char **envp)
>> >          exit(1);
>> >      }
>> >  
>> > -    /* init the memory */
>> > -    if (ram_size == 0) {
>> > -        ram_size = DEFAULT_RAM_SIZE * 1024 * 1024;
>> > -    }
>> > -
>> >      if (qemu_opts_foreach(qemu_find_opts("device"), device_help_func, 
>> > NULL, 0)
>> >          != 0) {
>> >          exit(0);
>
> Thanks for review!