| Message ID | 20210929025816.21076-17-wangyanan55@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | machine: smp parsing fixes and improvement | expand |
On 9/29/21 04:58, Yanan Wang wrote: > Quoting one of the Rules described in include/qapi/error.h: > " > Whenever practical, also return a value that indicates success / > failure. This can make the error checking more concise, and can > avoid useless error object creation and destruction. Note that > we still have many functions returning void. We recommend > • bool-valued functions return true on success / false on failure, > • pointer-valued functions return non-null / null pointer, and > • integer-valued functions return non-negative / negative. > " > > So make smp_parse() return true on success and false on failure, > so that we can more laconically check whether the parsing has > succeeded without touching the errp. > > Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Signed-off-by: Yanan Wang <wangyanan55@huawei.com> > --- > hw/core/machine.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) Thank you! Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On Wed, Sep 29, 2021 at 10:58:16AM +0800, Yanan Wang wrote: > Quoting one of the Rules described in include/qapi/error.h: > " > Whenever practical, also return a value that indicates success / > failure. This can make the error checking more concise, and can > avoid useless error object creation and destruction. Note that > we still have many functions returning void. We recommend > • bool-valued functions return true on success / false on failure, > • pointer-valued functions return non-null / null pointer, and > • integer-valued functions return non-negative / negative. > " > > So make smp_parse() return true on success and false on failure, > so that we can more laconically check whether the parsing has > succeeded without touching the errp. > > Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Signed-off-by: Yanan Wang <wangyanan55@huawei.com> > --- > hw/core/machine.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel
Yanan Wang <wangyanan55@huawei.com> writes: > Quoting one of the Rules described in include/qapi/error.h: > " > Whenever practical, also return a value that indicates success / > failure. This can make the error checking more concise, and can > avoid useless error object creation and destruction. Note that > we still have many functions returning void. We recommend > • bool-valued functions return true on success / false on failure, > • pointer-valued functions return non-null / null pointer, and > • integer-valued functions return non-negative / negative. > " > > So make smp_parse() return true on success and false on failure, > so that we can more laconically check whether the parsing has > succeeded without touching the errp. > > Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Signed-off-by: Yanan Wang <wangyanan55@huawei.com> > --- > hw/core/machine.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 4dc936732e..3e3a2707af 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c [...] > @@ -933,8 +935,7 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name, { MachineState *ms = MACHINE(obj); SMPConfiguration *config; ERRP_GUARD(); I believe ERRP_GUARD() is now redundant and should be dropped. if (!visit_type_SMPConfiguration(v, name, &config, errp)) { > return; > } > > - smp_parse(ms, config, errp); > - if (*errp) { > + if (!smp_parse(ms, config, errp)) { > qapi_free_SMPConfiguration(config); > } > }
On 29/09/21 04:58, Yanan Wang wrote: > @@ -933,8 +935,7 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name, > return; > } > > - smp_parse(ms, config, errp); > - if (*errp) { > + if (!smp_parse(ms, config, errp)) { > qapi_free_SMPConfiguration(config); > } > } > This is actually a leak, so I'm replacing this patch with diff --git a/hw/core/machine.c b/hw/core/machine.c index 54f04a5ac6..d49ebc24e2 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -897,7 +897,7 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name, { MachineClass *mc = MACHINE_GET_CLASS(obj); MachineState *ms = MACHINE(obj); - SMPConfiguration *config; + g_autoptr(SMPConfiguration) config = NULL; ERRP_GUARD(); if (!visit_type_SMPConfiguration(v, name, &config, errp)) { @@ -920,7 +920,7 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name, smp_parse(ms, config, errp); if (*errp) { - goto out_free; + return; } /* sanity-check smp_cpus and max_cpus against mc */ @@ -935,9 +935,6 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name, ms->smp.max_cpus, mc->name, mc->max_cpus); } - -out_free: - qapi_free_SMPConfiguration(config); } static void machine_class_init(ObjectClass *oc, void *data) which removes the need. Paolo
On Fri, Oct 01, 2021 at 07:08:51PM +0200, Paolo Bonzini wrote: > On 29/09/21 04:58, Yanan Wang wrote: > > @@ -933,8 +935,7 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name, > > return; > > } > > - smp_parse(ms, config, errp); > > - if (*errp) { > > + if (!smp_parse(ms, config, errp)) { > > qapi_free_SMPConfiguration(config); > > } > > } > > > > This is actually a leak, so I'm replacing this patch with This patch isn't adding a leak, as there's no change in control flow / exit paths. AFAICT, the leak was introduced in patch 15 instead, so the code below shoudl be squashed into that, and this patch left as-is. > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 54f04a5ac6..d49ebc24e2 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -897,7 +897,7 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name, > { > MachineClass *mc = MACHINE_GET_CLASS(obj); > MachineState *ms = MACHINE(obj); > - SMPConfiguration *config; > + g_autoptr(SMPConfiguration) config = NULL; > ERRP_GUARD(); > if (!visit_type_SMPConfiguration(v, name, &config, errp)) { > @@ -920,7 +920,7 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name, > smp_parse(ms, config, errp); > if (*errp) { > - goto out_free; > + return; > } > /* sanity-check smp_cpus and max_cpus against mc */ > @@ -935,9 +935,6 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name, > ms->smp.max_cpus, > mc->name, mc->max_cpus); > } > - > -out_free: > - qapi_free_SMPConfiguration(config); > } > static void machine_class_init(ObjectClass *oc, void *data) > > which removes the need. Regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Fri, Oct 01, 2021 at 07:08:51PM +0200, Paolo Bonzini wrote: >> On 29/09/21 04:58, Yanan Wang wrote: >> > @@ -933,8 +935,7 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name, >> > return; >> > } >> > - smp_parse(ms, config, errp); >> > - if (*errp) { >> > + if (!smp_parse(ms, config, errp)) { >> > qapi_free_SMPConfiguration(config); >> > } >> > } >> > >> >> This is actually a leak, so I'm replacing this patch with > > This patch isn't adding a leak, as there's no change in > control flow / exit paths. AFAICT, the leak was introduced > in patch 15 instead, so the code below shoudl be squashed > into that, and this patch left as-is. Concur.
On 01/10/21 19:15, Daniel P. Berrangé wrote: > On Fri, Oct 01, 2021 at 07:08:51PM +0200, Paolo Bonzini wrote: >> On 29/09/21 04:58, Yanan Wang wrote: >>> @@ -933,8 +935,7 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name, >>> return; >>> } >>> - smp_parse(ms, config, errp); >>> - if (*errp) { >>> + if (!smp_parse(ms, config, errp)) { >>> qapi_free_SMPConfiguration(config); >>> } >>> } >>> >> >> This is actually a leak, so I'm replacing this patch with > > This patch isn't adding a leak, as there's no change in > control flow / exit paths. AFAICT, the leak was introduced > in patch 15 instead, so the code below shoudl be squashed > into that, and this patch left as-is. Yes, even better make it a separate patch and fix the conflict in patch 15. But I'm still not sure of the wisdom of this patch. At this point smp_parse has exactly one caller and it doesn't care about the return value. The "return a boolean" rule adds some complexity (and a possibility for things to be wrong/inconsistent) to the function for the benefit of the callers. If there is only one caller, as is the case here or for virtual functions, the benefit can well be zero (this case) or negative (virtual functions). Paolo ---------------- 8< ---------------- From e7f944bb94a375e8ee7469ffa535ea6e11ce59e1 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini <pbonzini@redhat.com> Date: Fri, 1 Oct 2021 19:04:03 +0200 Subject: [PATCH] machine: Use g_autoptr in machine_set_smp Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/core/machine.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/hw/core/machine.c b/hw/core/machine.c index 54f04a5ac6..d49ebc24e2 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -897,7 +897,7 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name, { MachineClass *mc = MACHINE_GET_CLASS(obj); MachineState *ms = MACHINE(obj); - SMPConfiguration *config; + g_autoptr(SMPConfiguration) config = NULL; ERRP_GUARD(); if (!visit_type_SMPConfiguration(v, name, &config, errp)) { @@ -920,7 +920,7 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name, smp_parse(ms, config, errp); if (*errp) { - goto out_free; + return; } /* sanity-check smp_cpus and max_cpus against mc */ @@ -935,9 +935,6 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name, ms->smp.max_cpus, mc->name, mc->max_cpus); } - -out_free: - qapi_free_SMPConfiguration(config); } static void machine_class_init(ObjectClass *oc, void *data)
Paolo Bonzini <pbonzini@redhat.com> writes: > On 01/10/21 19:15, Daniel P. Berrangé wrote: >> On Fri, Oct 01, 2021 at 07:08:51PM +0200, Paolo Bonzini wrote: >>> On 29/09/21 04:58, Yanan Wang wrote: >>>> @@ -933,8 +935,7 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name, >>>> return; >>>> } >>>> - smp_parse(ms, config, errp); >>>> - if (*errp) { >>>> + if (!smp_parse(ms, config, errp)) { >>>> qapi_free_SMPConfiguration(config); >>>> } >>>> } >>>> >>> >>> This is actually a leak, so I'm replacing this patch with >> >> This patch isn't adding a leak, as there's no change in >> control flow / exit paths. AFAICT, the leak was introduced >> in patch 15 instead, so the code below shoudl be squashed >> into that, and this patch left as-is. > > Yes, even better make it a separate patch and fix the conflict in patch > 15. But I'm still not sure of the wisdom of this patch. > > At this point smp_parse has exactly one caller and it doesn't care about > the return value. The "return a boolean" rule adds some complexity (and > a possibility for things to be wrong/inconsistent) to the function for > the benefit of the callers. Yes, but returning something is only a minor burden. It also makes success vs. failure obvious at a glance. I'm not worrying about inconsistency anymore. In a way, void functions are an exception. Many non-void functions return a distinct error value on failure, like NULL. The only kind of inconsistency I can remember seeing in these functions is forgetting to set an error. Can be screwed up in a void function just as easily. > If there is only one caller, as is the case > here or for virtual functions, the benefit can well be zero (this case) > or negative (virtual functions). Two small benefits here: 1. No need for ERRP_GUARD(). 2. Conforms to the rules. Rules are not laws, but let's stick to them when it's as easy as it is here. For what it's worth, GLib always advised users of GError to return a value. We chose to deviate for our Error, then spent nine years learning how that leads to cumbersome code, leading us to: commit e3fe3988d7851cac30abffae06d2f555ff7bee62 Author: Markus Armbruster <armbru@redhat.com> Date: Tue Jul 7 18:05:31 2020 +0200 error: Document Error API usage rules This merely codifies existing practice, with one exception: the rule advising against returning void, where existing practice is mixed. When the Error API was created, we adopted the (unwritten) rule to return void when the function returns no useful value on success, unlike GError, which recommends to return true on success and false on error then. When a function returns a distinct error value, say false, a checked call that passes the error up looks like if (!frobnicate(..., errp)) { handle the error... } When it returns void, we need Error *err = NULL; frobnicate(..., &err); if (err) { handle the error... error_propagate(errp, err); } Not only is this more verbose, it also creates an Error object even when @errp is null, &error_abort or &error_fatal. People got tired of the additional boilerplate, and started to ignore the unwritten rule. The result is confusion among developers about the preferred usage. Make the rule advising against returning void official by putting it in writing. This will hopefully reduce confusion. Update the examples accordingly. The remainder of this series will update a substantial amount of code to honor the rule. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Greg Kurz <groug@kaod.org> Message-Id: <20200707160613.848843-4-armbru@redhat.com> [Tweak prose as per advice from Eric]
On 2021/10/2 19:27, Markus Armbruster wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> On 01/10/21 19:15, Daniel P. Berrangé wrote: >>> On Fri, Oct 01, 2021 at 07:08:51PM +0200, Paolo Bonzini wrote: >>>> On 29/09/21 04:58, Yanan Wang wrote: >>>>> @@ -933,8 +935,7 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name, >>>>> return; >>>>> } >>>>> - smp_parse(ms, config, errp); >>>>> - if (*errp) { >>>>> + if (!smp_parse(ms, config, errp)) { >>>>> qapi_free_SMPConfiguration(config); >>>>> } >>>>> } >>>>> >>>> This is actually a leak, so I'm replacing this patch with >>> This patch isn't adding a leak, as there's no change in >>> control flow / exit paths. AFAICT, the leak was introduced >>> in patch 15 instead, so the code below shoudl be squashed >>> into that, and this patch left as-is. >> Yes, even better make it a separate patch and fix the conflict in patch >> 15. But I'm still not sure of the wisdom of this patch. >> >> At this point smp_parse has exactly one caller and it doesn't care about >> the return value. The "return a boolean" rule adds some complexity (and >> a possibility for things to be wrong/inconsistent) to the function for >> the benefit of the callers. > Yes, but returning something is only a minor burden. It also makes > success vs. failure obvious at a glance. > > I'm not worrying about inconsistency anymore. In a way, void functions > are an exception. Many non-void functions return a distinct error value > on failure, like NULL. The only kind of inconsistency I can remember > seeing in these functions is forgetting to set an error. Can be screwed > up in a void function just as easily. > >> If there is only one caller, as is the case >> here or for virtual functions, the benefit can well be zero (this case) >> or negative (virtual functions). > Two small benefits here: > > 1. No need for ERRP_GUARD(). > > 2. Conforms to the rules. Rules are not laws, but let's stick to them > when it's as easy as it is here. > > For what it's worth, GLib always advised users of GError to return a > value. We chose to deviate for our Error, then spent nine years > learning how that leads to cumbersome code, leading us to: > > commit e3fe3988d7851cac30abffae06d2f555ff7bee62 > Author: Markus Armbruster <armbru@redhat.com> > Date: Tue Jul 7 18:05:31 2020 +0200 > > error: Document Error API usage rules > > This merely codifies existing practice, with one exception: the rule > advising against returning void, where existing practice is mixed. > > When the Error API was created, we adopted the (unwritten) rule to > return void when the function returns no useful value on success, > unlike GError, which recommends to return true on success and false on > error then. > > When a function returns a distinct error value, say false, a checked > call that passes the error up looks like > > if (!frobnicate(..., errp)) { > handle the error... > } > > When it returns void, we need > > Error *err = NULL; > > frobnicate(..., &err); > if (err) { > handle the error... > error_propagate(errp, err); > } > > Not only is this more verbose, it also creates an Error object even > when @errp is null, &error_abort or &error_fatal. > > People got tired of the additional boilerplate, and started to ignore > the unwritten rule. The result is confusion among developers about > the preferred usage. > > Make the rule advising against returning void official by putting it > in writing. This will hopefully reduce confusion. > > Update the examples accordingly. > > The remainder of this series will update a substantial amount of code > to honor the rule. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Reviewed-by: Greg Kurz <groug@kaod.org> > Message-Id: <20200707160613.848843-4-armbru@redhat.com> > [Tweak prose as per advice from Eric] > Hi, Thanks for the fix, Paolo! I notice that with Paolo's fix applied first and then Patch15 removing the sanity checks out, machine_set_smp() at last simply becomes: static void machine_set_smp(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) { MachineState *ms = MACHINE(obj); g_autoptr(SMPConfiguration) config = NULL; if (!visit_type_SMPConfiguration(v, name, &config, errp)) { return; } smp_parse(ms, config, errp); } It looks good currently, neither the returned boolean nor the errp needs to be checked here now, and smp_parse is only called here. So in this case, we may avoid the boolean until we need to use it and honor the rule. :） Thanks, Yanan
On 02/10/21 13:27, Markus Armbruster wrote: >> The "return a boolean" rule adds some complexity (and >> a possibility for things to be wrong/inconsistent) to the function for >> the benefit of the callers. > Yes, but returning something is only a minor burden. It also makes > success vs. failure obvious at a glance. Fair enough; I'd still prefer to have an exception to the rule for virtual functions. In that case, I really find the benefit to be negative. Paolo
On 07/10/21 05:44, wangyanan (Y) wrote: > > I notice that with Paolo's fix applied first and then Patch15 removing > the sanity checks out, machine_set_smp() at last simply becomes: > > static void machine_set_smp(Object *obj, Visitor *v, const char *name, > void *opaque, Error **errp) > { > MachineState *ms = MACHINE(obj); > g_autoptr(SMPConfiguration) config = NULL; > > if (!visit_type_SMPConfiguration(v, name, &config, errp)) { > return; > } > > smp_parse(ms, config, errp); > } > > It looks good currently, neither the returned boolean nor the errp needs to > be checked here now, and smp_parse is only called here. So in this case, > we may avoid the boolean until we need to use it and honor the rule. :） Even inlining smp_parse is a possibility now. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 02/10/21 13:27, Markus Armbruster wrote: >>> The "return a boolean" rule adds some complexity (and >>> a possibility for things to be wrong/inconsistent) to the function for >>> the benefit of the callers. >> >> Yes, but returning something is only a minor burden. It also makes >> success vs. failure obvious at a glance. > > Fair enough; I'd still prefer to have an exception to the rule for > virtual functions. In that case, I really find the benefit to be > negative. No rule without exceptions :)
diff --git a/hw/core/machine.c b/hw/core/machine.c index 4dc936732e..3e3a2707af 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -788,7 +788,7 @@ static char *cpu_hierarchy_to_string(MachineState *ms) * introduced topology members which are likely to be target specific should * be directly set as 1 if they are omitted (e.g. dies for PC since 4.1). */ -static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) +static bool smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) { MachineClass *mc = MACHINE_GET_CLASS(ms); unsigned cpus = config->has_cpus ? config->cpus : 0; @@ -818,7 +818,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) */ if (!mc->smp_props.dies_supported && dies > 1) { error_setg(errp, "dies not supported by this machine's CPU topology"); - return; + return false; } dies = dies > 0 ? dies : 1; @@ -876,7 +876,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) "product of the hierarchy must match maxcpus: " "%s != maxcpus (%u)", topo_msg, maxcpus); - return; + return false; } if (maxcpus < cpus) { @@ -885,7 +885,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) "maxcpus must be equal to or greater than smp: " "%s == maxcpus (%u) < smp_cpus (%u)", topo_msg, maxcpus, cpus); - return; + return false; } if (ms->smp.cpus < mc->min_cpus) { @@ -893,7 +893,7 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) "supported by machine '%s' is %d", ms->smp.cpus, mc->name, mc->min_cpus); - return; + return false; } if (ms->smp.max_cpus > mc->max_cpus) { @@ -901,8 +901,10 @@ static void smp_parse(MachineState *ms, SMPConfiguration *config, Error **errp) "supported by machine '%s' is %d", ms->smp.max_cpus, mc->name, mc->max_cpus); - return; + return false; } + + return true; } static void machine_get_smp(Object *obj, Visitor *v, const char *name, @@ -933,8 +935,7 @@ static void machine_set_smp(Object *obj, Visitor *v, const char *name, return; } - smp_parse(ms, config, errp); - if (*errp) { + if (!smp_parse(ms, config, errp)) { qapi_free_SMPConfiguration(config); } }
Quoting one of the Rules described in include/qapi/error.h: " Whenever practical, also return a value that indicates success / failure. This can make the error checking more concise, and can avoid useless error object creation and destruction. Note that we still have many functions returning void. We recommend • bool-valued functions return true on success / false on failure, • pointer-valued functions return non-null / null pointer, and • integer-valued functions return non-negative / negative. " So make smp_parse() return true on success and false on failure, so that we can more laconically check whether the parsing has succeeded without touching the errp. Suggested-by: Philippe Mathieu-Daudé <philmd@redhat.com> Signed-off-by: Yanan Wang <wangyanan55@huawei.com> --- hw/core/machine.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)