David Hildenbrand <da...@redhat.com> writes:
> On 25.08.23 11:10, Markus Armbruster wrote:
>> David Hildenbrand <da...@redhat.com> writes:
>>
>>> On 25.08.23 08:57, ThinerLogoer wrote:
>>>> Hello,
>>>>
>>>> At 2023-08-23 23:34:11, "David Hildenbrand" <da...@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 <logoerthin...@163.com>
>>>>> Signed-off-by: David Hildenbrand <da...@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.
