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

qapi/error.h tells you :)

 * = Creating errors =
 [...]
 * Create an error and add additional explanation:
 *     error_setg(errp, "invalid quark");
 *     error_append_hint(errp, "Valid quarks are up, down, strange, "
 *                       "charm, top, bottom.\n");
 * This may require use of ERRP_GUARD(); more on that below.
 [...]
 * = Why, when and how to use ERRP_GUARD() =
 *
 * Without ERRP_GUARD(), use of the @errp parameter is restricted:
 * - It must not be dereferenced, because it may be null.
 * - It should not be passed to error_prepend() or
 *   error_append_hint(), because that doesn't work with &error_fatal.
 * ERRP_GUARD() lifts these restrictions.
 *
 * To use ERRP_GUARD(), add it right at the beginning of the function.
 * @errp can then be used without worrying about the argument being
 * NULL or &error_fatal.
 *
 * Using it when it's not needed is safe, but please avoid cluttering
 * the source with useless code.
 *
 * = Converting to ERRP_GUARD() =
 *
 * To convert a function to use ERRP_GUARD():
 *
 * 0. If the Error ** parameter is not named @errp, rename it to
 *    @errp.
 *
 * 1. Add an ERRP_GUARD() invocation, by convention right at the
 *    beginning of the function.  This makes @errp safe to use.
 *
 * 2. Replace &err by errp, and err by *errp.  Delete local variable
 *    @err.
 *
 * 3. Delete error_propagate(errp, *errp), replace
 *    error_propagate_prepend(errp, *errp, ...) by error_prepend(errp, ...)
 *
 * 4. Ensure @errp is valid at return: when you destroy *errp, set
 *    *errp = NULL.
 *
 * Example:
 *
 *     bool fn(..., Error **errp)
 *     {
 *         Error *err = NULL;
 *
 *         foo(arg, &err);
 *         if (err) {
 *             handle the error...
 *             error_propagate(errp, err);
 *             return false;
 *         }
 *         ...
 *     }
 *
 * becomes
 *
 *     bool fn(..., Error **errp)
 *     {
 *         ERRP_GUARD();
 *
 *         foo(arg, errp);
 *         if (*errp) {
 *             handle the error...
 *             return false;
 *         }
 *         ...
 *     }
 *
 * For mass-conversion, use scripts/coccinelle/errp-guard.cocci.

Questions?

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