| Message ID | 20230823153412.832081-12-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | memory-backend-file related improvements and VM templating support | expand |
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)) {
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.
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
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; }
This 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 > >
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>
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!
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; }
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(-)