Re: [Qemu-devel] [PATCH] vl.c: fix regression when reading memory size from config file

[Paolo Bonzini]

On 11/01/2015 11:38, Marcel Apfelbaum wrote:
> This is happening because an actual logic is performed on the memory
> arguments inside the main's switch, disregarding the config file content.
> 
> Solved by extracting the logic on a separate function and calling it
> after the switch.
> 
> Signed-off-by: Marcel Apfelbaum <address@hidden>
> ---
>  vl.c | 176 
> +++++++++++++++++++++++++++++++++++--------------------------------
>  1 file changed, 92 insertions(+), 84 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index adfdbfd..4877a47 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2644,6 +2644,93 @@ out:
>      return 0;
>  }
>  
> +static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size)
> +{
> +    uint64_t sz;
> +    const char *mem_str;
> +    const char *maxmem_str, *slots_str;
> +    const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
> +                                        1024 * 1024;
> +    QemuOpts *opts = qemu_find_opts_singleton("memory");
> +
> +    ram_size = default_ram_size;
> +    *maxram_size = default_ram_size;
> +
> +    mem_str = qemu_opt_get(opts, "size");
> +    if (!mem_str) {
> +        error_report("invalid -m option, missing 'size' option");
> +        exit(EXIT_FAILURE);
> +    }
> +    if (!*mem_str) {
> +        error_report("missing 'size' option value");
> +        exit(EXIT_FAILURE);
> +    }
> +
> +    sz = qemu_opt_get_size(opts, "size", ram_size);
> +
> +    /* Fix up legacy suffix-less format */
> +    if (g_ascii_isdigit(mem_str[strlen(mem_str) - 1])) {
> +        uint64_t overflow_check = sz;
> +
> +        sz <<= 20;
> +        if ((sz >> 20) != overflow_check) {
> +            error_report("too large 'size' option value");
> +            exit(EXIT_FAILURE);
> +        }
> +    }
> +
> +    /* backward compatibility behaviour for case "-m 0" */
> +    if (sz == 0) {
> +        sz = default_ram_size;
> +    }
> +
> +    sz = QEMU_ALIGN_UP(sz, 8192);
> +    ram_size = sz;
> +    if (ram_size != sz) {
> +        error_report("ram size too large");
> +        exit(EXIT_FAILURE);
> +    }
> +    *maxram_size = ram_size;
> +
> +    maxmem_str = qemu_opt_get(opts, "maxmem");
> +    slots_str = qemu_opt_get(opts, "slots");
> +    if (maxmem_str && slots_str) {
> +        uint64_t slots;
> +
> +        sz = qemu_opt_get_size(opts, "maxmem", 0);
> +        if (sz < ram_size) {
> +            error_report("invalid -m option value: maxmem "
> +                    "(0x%" PRIx64 ") <= initial memory (0x"
> +                    RAM_ADDR_FMT ")", sz, ram_size);
> +            exit(EXIT_FAILURE);
> +        }
> +
> +        slots = qemu_opt_get_number(opts, "slots", 0);
> +        if ((sz > ram_size) && !slots) {
> +            error_report("invalid -m option value: maxmem "
> +                    "(0x%" PRIx64 ") more than initial memory (0x"
> +                    RAM_ADDR_FMT ") but no hotplug slots where "
> +                    "specified", sz, ram_size);
> +            exit(EXIT_FAILURE);
> +        }
> +
> +        if ((sz <= ram_size) && slots) {
> +            error_report("invalid -m option value:  %"
> +                    PRIu64 " hotplug slots where specified but "
> +                    "maxmem (0x%" PRIx64 ") <= initial memory (0x"
> +                    RAM_ADDR_FMT ")", slots, sz, ram_size);
> +            exit(EXIT_FAILURE);
> +        }
> +        *maxram_size = sz;
> +        *ram_slots = slots;
> +    } else if ((!maxmem_str && slots_str) ||
> +            (maxmem_str && !slots_str)) {
> +        error_report("invalid -m option value: missing "
> +                "'%s' option", slots_str ? "maxmem" : "slots");
> +        exit(EXIT_FAILURE);
> +    }
> +}
> +
>  int main(int argc, char **argv, char **envp)
>  {
>      int i;
> @@ -2679,9 +2766,7 @@ int main(int argc, char **argv, char **envp)
>      };
>      const char *trace_events = NULL;
>      const char *trace_file = NULL;
> -    const ram_addr_t default_ram_size = (ram_addr_t)DEFAULT_RAM_SIZE *
> -                                        1024 * 1024;
> -    ram_addr_t maxram_size = default_ram_size;
> +    ram_addr_t maxram_size;
>      uint64_t ram_slots = 0;
>      FILE *vmstate_dump_file = NULL;
>      Error *main_loop_err = NULL;
> @@ -2732,7 +2817,6 @@ int main(int argc, char **argv, char **envp)
>      module_call_init(MODULE_INIT_MACHINE);
>      machine_class = find_default_machine();
>      cpu_model = NULL;
> -    ram_size = default_ram_size;
>      snapshot = 0;
>      cyls = heads = secs = 0;
>      translation = BIOS_ATA_TRANSLATION_AUTO;
> @@ -3022,92 +3106,13 @@ int main(int argc, char **argv, char **envp)
>                  version();
>                  exit(0);
>                  break;
> -            case QEMU_OPTION_m: {
> -                uint64_t sz;
> -                const char *mem_str;
> -                const char *maxmem_str, *slots_str;
> -
> +            case QEMU_OPTION_m:
>                  opts = qemu_opts_parse(qemu_find_opts("memory"),
>                                         optarg, 1);
>                  if (!opts) {
>                      exit(EXIT_FAILURE);
>                  }
> -
> -                mem_str = qemu_opt_get(opts, "size");
> -                if (!mem_str) {
> -                    error_report("invalid -m option, missing 'size' option");
> -                    exit(EXIT_FAILURE);
> -                }
> -                if (!*mem_str) {
> -                    error_report("missing 'size' option value");
> -                    exit(EXIT_FAILURE);
> -                }
> -
> -                sz = qemu_opt_get_size(opts, "size", ram_size);
> -
> -                /* Fix up legacy suffix-less format */
> -                if (g_ascii_isdigit(mem_str[strlen(mem_str) - 1])) {
> -                    uint64_t overflow_check = sz;
> -
> -                    sz <<= 20;
> -                    if ((sz >> 20) != overflow_check) {
> -                        error_report("too large 'size' option value");
> -                        exit(EXIT_FAILURE);
> -                    }
> -                }
> -
> -                /* backward compatibility behaviour for case "-m 0" */
> -                if (sz == 0) {
> -                    sz = default_ram_size;
> -                }
> -
> -                sz = QEMU_ALIGN_UP(sz, 8192);
> -                ram_size = sz;
> -                if (ram_size != sz) {
> -                    error_report("ram size too large");
> -                    exit(EXIT_FAILURE);
> -                }
> -                maxram_size = ram_size;
> -
> -                maxmem_str = qemu_opt_get(opts, "maxmem");
> -                slots_str = qemu_opt_get(opts, "slots");
> -                if (maxmem_str && slots_str) {
> -                    uint64_t slots;
> -
> -                    sz = qemu_opt_get_size(opts, "maxmem", 0);
> -                    if (sz < ram_size) {
> -                        error_report("invalid -m option value: maxmem "
> -                                "(0x%" PRIx64 ") <= initial memory (0x"
> -                                RAM_ADDR_FMT ")", sz, ram_size);
> -                        exit(EXIT_FAILURE);
> -                    }
> -
> -                    slots = qemu_opt_get_number(opts, "slots", 0);
> -                    if ((sz > ram_size) && !slots) {
> -                        error_report("invalid -m option value: maxmem "
> -                                "(0x%" PRIx64 ") more than initial memory 
> (0x"
> -                                RAM_ADDR_FMT ") but no hotplug slots where "
> -                                "specified", sz, ram_size);
> -                        exit(EXIT_FAILURE);
> -                    }
> -
> -                    if ((sz <= ram_size) && slots) {
> -                        error_report("invalid -m option value:  %"
> -                                PRIu64 " hotplug slots where specified but "
> -                                "maxmem (0x%" PRIx64 ") <= initial memory 
> (0x"
> -                                RAM_ADDR_FMT ")", slots, sz, ram_size);
> -                        exit(EXIT_FAILURE);
> -                    }
> -                    maxram_size = sz;
> -                    ram_slots = slots;
> -                } else if ((!maxmem_str && slots_str) ||
> -                           (maxmem_str && !slots_str)) {
> -                    error_report("invalid -m option value: missing "
> -                            "'%s' option", slots_str ? "maxmem" : "slots");
> -                    exit(EXIT_FAILURE);
> -                }
>                  break;
> -            }
>  #ifdef CONFIG_TPM
>              case QEMU_OPTION_tpmdev:
>                  if (tpm_config_parse(qemu_find_opts("tpmdev"), optarg) < 0) {
> @@ -3752,6 +3757,9 @@ int main(int argc, char **argv, char **envp)
>              }
>          }
>      }
> +
> +    set_memory_options(&ram_slots, &maxram_size);
> +
>      loc_set_none();
>  
>      os_daemonize();
> 

Looks good, thanks.

Paolo