Re: [PATCH v3 11/11] machine: Improve error message when using default RAM backend id

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> On 25.08.23 08:57, ThinerLogoer wrote:
>> Hello,
>> 
>> At 2023-08-23 23:34:11, "David Hildenbrand" <david@redhat.com> wrote:
>>> For migration purposes, users might want to reuse the default RAM
>>> backend id, but specify a different memory backend.
>>>
>>> For example, to reuse "pc.ram" on q35, one has to set
>>>     -machine q35,memory-backend=pc.ram
>>> Only then, can a memory backend with the id "pc.ram" be created
>>> manually.
>>>
>>> Let's improve the error message.
>>>
>>> Unfortuantely, we cannot use error_append_hint(), because the caller
>>> passes &error_fatal.
>>>
>>> Suggested-by: ThinerLogoer <logoerthiner1@163.com>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>> hw/core/machine.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>>> index f0d35c6401..dbcd124d45 100644
>>> --- a/hw/core/machine.c
>>> +++ b/hw/core/machine.c
>>> @@ -1382,7 +1382,9 @@ void machine_run_board_init(MachineState *machine, 
>>> const char *mem_path, Error *
>>>                                   machine_class->default_ram_id)) {
>>>              error_setg(errp, "object name '%s' is reserved for the default"
>>>                  " RAM backend, it can't be used for any other purposes."
>>> -                " Change the object's 'id' to something else",
>>> +                " Change the object's 'id' to something else or disable"
>>> +                " automatic creation of the default RAM backend by setting"
>>> +                " the 'memory-backend' machine property",
>>>                  machine_class->default_ram_id);
>>>              return;
>>>          }
>> 
>> I'd suggest a more explicit version:
>> 
>>                  " Change the object's 'id' to something else or disable"
>>                  " automatic creation of the default RAM backend by 
>> appending"
>>                  "  'memory-backend={machine_class->default_ram_id}' in 
>> '-machine' arguments",
>
>
> Thanks, I'll do:
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index f0d35c6401..cd0fd6cdd1 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -1382,8 +1382,10 @@ void machine_run_board_init(MachineState *machine, 
> const char *mem_path, Error *
>                                    machine_class->default_ram_id)) {
>               error_setg(errp, "object name '%s' is reserved for the default"
>                   " RAM backend, it can't be used for any other purposes."
> -                " Change the object's 'id' to something else",
> -                machine_class->default_ram_id);
> +                " Change the object's 'id' to something else or disable"
> +                " automatic creation of the default RAM backend by appending"
> +                " 'memory-backend=%s' in '-machine' arguments",
> +                machine_class->default_ram_id, 
> machine_class->default_ram_id);
>               return;
>           }
>           if (!create_default_memdev(current_machine, mem_path, errp)) {

error_setg()'s function comment specifies:

 * The resulting message should be a single phrase, with no newline or
 * trailing punctuation.

Please use error_append_hint(), like so

             error_setg(errp, "object name '%s' is reserved for the default"
                 " RAM backend, it can't be used for any other purposes",
                 machine_class->default_ram_id);
             error_append_hint(errp,
                 "Change the object's 'id' to something else or disable"
                 " automatic creation of the default RAM backend by appending"
                 " 'memory-backend=%s' in '-machine' arguments\n",
                 machine_class->default_ram_id);

Moreover:

* "object name" feels off, we're talking about IDs, aren't we?

* "appending X in Y" should be "appending X to Y".  Consider "setting
  'memory-backend=%s' with -machine".