qemu-ppc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 11/11] machine: Improve error message when using default R


From: David Hildenbrand
Subject: Re: [PATCH v3 11/11] machine: Improve error message when using default RAM backend id
Date: Fri, 25 Aug 2023 12:10:38 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]