Re: [PATCH v2 7/9] hw/sd/sdcard: Do not allow invalid SD card sizes

[Markus Armbruster <armbru@redhat.com>]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 13 Jul 2020 at 19:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> QEMU allows to create SD card with unrealistic sizes. This could
>> work, but some guests (at least Linux) consider sizes that are not
>> a power of 2 as a firmware bug and fix the card size to the next
>> power of 2.
>>
>
>> +            error_setg(errp, "Invalid SD card size: %s", blk_size_str);
>> +            g_free(blk_size_str);
>> +
>> +            blk_size_str = size_to_str(blk_size_aligned);
>> +            error_append_hint(errp,
>> +                              "SD card size has to be a power of 2, e.g. %s.\n"
>> +                              "You can resize disk images with "
>> +                              "'qemu-img resize <imagefile> <new-size>'\n"
>> +                              "(note that this will lose data if you make the "
>> +                              "image smaller than it currently is).\n",
>> +                              blk_size_str);
>> +            g_free(blk_size_str);
>
> Some places that create multi-line hints with error_append_hint()
> do it by calling it once per line, eg in target/arm/cpu64.c:
>                 error_setg(errp, "cannot disable sve128");
>                 error_append_hint(errp, "Disabling sve128 results in all "
>                                   "vector lengths being disabled.\n");
>                 error_append_hint(errp, "With SVE enabled, at least one "
>                                   "vector length must be enabled.\n");
>
> Some places don't, eg in block/vhdx-log.c:
>             error_append_hint(errp,  "To replay the log, run:\n"
>                               "qemu-img check -r all '%s'\n",
>                               bs->filename);

Both fine.

> Most places terminate with a '\n', but some places don't, eg
> crypto/block-luks.c:
>        error_append_hint(errp, "Failed to write to keyslot %i", keyslot);
>        return -1;

This is a bug.

> The documentation says
>  * May be called multiple times.  The resulting hint should end with a
>  * newline.
>
> which isn't very clear -- you can call it multiple times, but
> must you, if it's multiline?

If that was required, my doc comment would demand it.

> I assume that "should end with a newline" means "must end
> with a newline", and places like block-luks.c are bugs.

If you construct a hint with multiple calls, individual calls need not
terminate with a newline, but the resulting hint still should.

I readily admit that my doc comments sometimes require a "lawyerly"
reading :)

> Markus, do you know what the intended API here is?
>
> It looks like the implementation just tacks the hint
> string onto the end of any existing hint string, in
> which case multiple-line strings are fine and the same
> behaviour as calling the function multiple times.

That's the intended behavior.

> (I had assumed we might be accumulating an array of strings,
> or requiring multiline strings to be multiple calls so we
> could have the argument not need to be \n-terminated,
> to match error_setg(), but both those assumptions
> are obviously wrong.)

Would also be workable, if slightly less flexible.

Yes, error_setg() and error_append_hint() differ on newlines.

error_setg() constructs an error message.  These get reported in a rigid
format that does not afford line breaks in the middle.

error_append_hint() builds up a hint.  Hints are free-form text.
Embedded line breaks are fine.

In both cases, I decided not to treat newline at the end any different
from newlines anywhere else.  Thus, no newlines at all with
error_setg(), and newlines after every line including the last one with
error_append_hint().

> Anyway, I guess this multiline-message usage is something
> we do already and it will DTRT, so

Correct.

Hope this helps!

> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
>
> thanks
> -- PMM