Re: [PATCH] print memory in MB units in initrd-too-large errmsg

[Markus Armbruster]

Jim Cromie <jim.cromie@gmail.com> writes:

> Change 2 error messages to display sizes in MB, not bytes.
>
> qemu: initrd is too large, cannot support this. (max: 2047 MB, need 5833 MB)
>
> Also, distinguish 2 sites by adding "it" and "this" respectively.
> This tells a careful reader that the error above is from the 2nd size
> check.

I don't like this part.

> With MB displayed, I have to ask: is it coincidence that max == 2048-1 ?

I don't know.

> Signed-off-by: Jim Cromie <jim.cromie@gmail.com>
> ---
>  hw/i386/x86.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index a88a126123..0677fe2fd1 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -878,9 +878,9 @@ void x86_load_linux(X86MachineState *x86ms,
>                  initrd_size = g_mapped_file_get_length(mapped_file);
>                  initrd_max = x86ms->below_4g_mem_size - acpi_data_size - 1;
>                  if (initrd_size >= initrd_max) {

Not this patch's problem, but here goes anyway: why reject initrd_size
== initrd_max?

> -                    fprintf(stderr, "qemu: initrd is too large, cannot 
> support."
> -                            "(max: %"PRIu32", need %"PRId64")\n",
> -                            initrd_max, (uint64_t)initrd_size);
> +                    fprintf(stderr, "qemu: initrd is too large, cannot 
> support it. "
> +                            "(max: %"PRIu32" MB, need %"PRId64" MB)\n",
> +                            initrd_max>>20, (uint64_t)initrd_size>>20);
>                      exit(1);
>                  }
>  
> @@ -1023,9 +1023,9 @@ void x86_load_linux(X86MachineState *x86ms,
>          initrd_data = g_mapped_file_get_contents(mapped_file);
>          initrd_size = g_mapped_file_get_length(mapped_file);
>          if (initrd_size >= initrd_max) {
> -            fprintf(stderr, "qemu: initrd is too large, cannot support."
> -                    "(max: %"PRIu32", need %"PRId64")\n",
> -                    initrd_max, (uint64_t)initrd_size);
> +            fprintf(stderr, "qemu: initrd is too large, cannot support this. 
> "
> +                    "(max: %"PRIu32" MB, need %"PRId64" MB)\n",
> +                    initrd_max>>20, (uint64_t)initrd_size>>20);
>              exit(1);
>          }

You're rounding both values down.  Consider

    initrd_max = 1000.5MiB - 1 exactly
    initrd_size = 1000.5MiB + 4096

Before the patch, we report

    (max: 1049100287, need 1049104384)

Unfriendly, but at least it's correct.  Afterwards

    (max: 1000 MB, need 1000 MB)

Wat?  Long-suffering users may guess the rounding issue.  But let's not
make users guess.

Better would be something like

    size of initrd exceeds the limit of X MiB by Y Bytes

*if* X is actually a multiple of 1 MiB.  Else, we need to consider
showing a suitably rounded fractional part, or stick to Bytes after all.