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:56, Markus Armbruster wrote:
> 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?

Thanks for the pointer!

... hopefully I'm done with that error-handling pain in QEMU soon and 
can continue focusing on things that make me feel more productive :P

-- 
Cheers,

David / dhildenb