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

[David Hildenbrand <david@redhat.com>]

On 25.08.23 11:10, Markus Armbruster wrote:
> 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)) {
> 

Hi Markus,

> 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

Please see the patch description: "Unfortunately, we cannot use 
error_append_hint(), because the caller passes &error_fatal."

How should I deal with that?

> 
>               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?

Yes, I think so.

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

Can do, thanks.

-- 
Cheers,

David / dhildenb