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:59, David Hildenbrand wrote:
> 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
> 

... hoping it's as simple as this:

diff --git a/hw/core/machine.c b/hw/core/machine.c
index f0d35c6401..09f40c7f07 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1352,6 +1352,7 @@ out:
  
  void machine_run_board_init(MachineState *machine, const char *mem_path, Error **errp)
  {
+    ERRP_GUARD();
      MachineClass *machine_class = MACHINE_GET_CLASS(machine);
      ObjectClass *oc = object_class_by_name(machine->cpu_type);
      CPUClass *cc;
@@ -1380,9 +1381,13 @@ void machine_run_board_init(MachineState *machine, const char *mem_path, Error *
                 numa_uses_legacy_mem()) {
          if (object_property_find(object_get_objects_root(),
                                   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",
+            error_setg(errp, "object's id '%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 setting"
+                " 'memory-backend=%s' with '-machine'.\n",
                  machine_class->default_ram_id);
              return;
          }


-- 
Cheers,

David / dhildenb