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;
}
--
2.41.0This series has been successfully tested by QE. Start a vm using
pc.ram id but specifying a different memory-backend from the default
one. Check the error message has been improved.
Tested-by: Mario Casquero <mcasquer@redhat.com>
On Wed, Aug 23, 2023 at 5:38 PM 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;
> }
> --
> 2.41.0
>
>
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",
All other patches are good on my environment, applicable on 8.1.0.
--
Regards,
logoerthiner
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)) {
--
Cheers,
David / dhildenb
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)) {
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
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?
* "appending X in Y" should be "appending X to Y". Consider "setting
'memory-backend=%s' with -machine".
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
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.
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
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
David Hildenbrand <david@redhat.com> writes:
> On 25.08.23 11:59, David Hildenbrand wrote:
[...]
>> ... 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
I'm afraid you'll be done with error handling right when you're done
with developing software.
> ... 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;
> }
Looks good to me!
On 25/8/23 12:10, David Hildenbrand wrote:
> 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..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;
> }
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
© 2016 - 2024 Red Hat, Inc.