diff mbox series

[17/17] target/s390x/cpu_models: Fix missing ERRP_GUARD() for error_prepend()

Message ID 20240229143914.1977550-18-zhao1.liu@linux.intel.com
State New
Headers show
Series Cleanup up to fix missing ERRP_GUARD() for error_prepend() | expand

Commit Message

Zhao Liu Feb. 29, 2024, 2:39 p.m. UTC
From: Zhao Liu <zhao1.liu@intel.com>

As the comment in qapi/error, passing @errp to error_prepend() requires
ERRP_GUARD():

* = Why, when and how to use ERRP_GUARD() =
*
* Without ERRP_GUARD(), use of the @errp parameter is restricted:
...
* - It should not be passed to error_prepend(), error_vprepend() 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.

ERRP_GUARD() could avoid the case when @errp is the pointer of
error_fatal, the user can't see this additional information, because
exit() happens in error_setg earlier than information is added [1].

In target/s390x/cpu_models.c, there're 2 functions passing @errp to
error_prepend() without ERRP_GUARD():
- check_compatibility()
- s390_realize_cpu_model()

Though both their @errp parameters point to their callers' local @err
virables and don't cause the issue as [1] said, to follow the
requirement of @errp, also add missing ERRP_GUARD() at their beginning.

[1]: Issue description in the commit message of commit ae7c80a7bd73
     ("error: New macro ERRP_GUARD()").

Cc: David Hildenbrand <david@redhat.com>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Ilya Leoshkevich <iii@linux.ibm.com>
Cc: Thomas Huth <thuth@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 target/s390x/cpu_models.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Thomas Huth Feb. 29, 2024, 5:17 p.m. UTC | #1
On 29/02/2024 15.39, Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> As the comment in qapi/error, passing @errp to error_prepend() requires
> ERRP_GUARD():
> 
> * = Why, when and how to use ERRP_GUARD() =
> *
> * Without ERRP_GUARD(), use of the @errp parameter is restricted:
> ...
> * - It should not be passed to error_prepend(), error_vprepend() 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.
> 
> ERRP_GUARD() could avoid the case when @errp is the pointer of
> error_fatal, the user can't see this additional information, because
> exit() happens in error_setg earlier than information is added [1].
> 
> In target/s390x/cpu_models.c, there're 2 functions passing @errp to
> error_prepend() without ERRP_GUARD():
> - check_compatibility()
> - s390_realize_cpu_model()
> 
> Though both their @errp parameters point to their callers' local @err
> virables and don't cause the issue as [1] said, to follow the
> requirement of @errp, also add missing ERRP_GUARD() at their beginning.
> 
> [1]: Issue description in the commit message of commit ae7c80a7bd73
>       ("error: New macro ERRP_GUARD()").
> 
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Ilya Leoshkevich <iii@linux.ibm.com>
> Cc: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>   target/s390x/cpu_models.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index a63d990e4e8e..1a1c09612271 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -503,6 +503,7 @@ static void error_prepend_missing_feat(const char *name, void *opaque)
>   static void check_compatibility(const S390CPUModel *max_model,
>                                   const S390CPUModel *model, Error **errp)
>   {
> +    ERRP_GUARD();
>       S390FeatBitmap missing;
>   
>       if (model->def->gen > max_model->def->gen) {
> @@ -566,6 +567,7 @@ S390CPUModel *get_max_cpu_model(Error **errp)
>   
>   void s390_realize_cpu_model(CPUState *cs, Error **errp)
>   {
> +    ERRP_GUARD();
>       Error *err = NULL;

I think that function could use an additional clean-up now: Remove the local 
"err" variable and use "errp" only instead.

  Thomas


>       S390CPUClass *xcc = S390_CPU_GET_CLASS(cs);
>       S390CPU *cpu = S390_CPU(cs);
Zhao Liu March 1, 2024, 6:46 a.m. UTC | #2
> > diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> > index a63d990e4e8e..1a1c09612271 100644
> > --- a/target/s390x/cpu_models.c
> > +++ b/target/s390x/cpu_models.c
> > @@ -503,6 +503,7 @@ static void error_prepend_missing_feat(const char *name, void *opaque)
> >   static void check_compatibility(const S390CPUModel *max_model,
> >                                   const S390CPUModel *model, Error **errp)
> >   {
> > +    ERRP_GUARD();
> >       S390FeatBitmap missing;
> >       if (model->def->gen > max_model->def->gen) {
> > @@ -566,6 +567,7 @@ S390CPUModel *get_max_cpu_model(Error **errp)
> >   void s390_realize_cpu_model(CPUState *cs, Error **errp)
> >   {
> > +    ERRP_GUARD();
> >       Error *err = NULL;
> 
> I think that function could use an additional clean-up now: Remove the local
> "err" variable and use "errp" only instead.
>

Thanks! It's cleaner. Will do this.

Regards,
Zhao
Thomas Huth March 8, 2024, 10:08 a.m. UTC | #3
On 08/03/2024 11.12, Zhao Liu wrote:
> Hi Thomas,
> 
>>>    void s390_realize_cpu_model(CPUState *cs, Error **errp)
>>>    {
>>> +    ERRP_GUARD();
>>>        Error *err = NULL;
>>
>> I think that function could use an additional clean-up now: Remove the local
>> "err" variable and use "errp" only instead.
>>
> 
> Since many cases check @err to determine if the callee function fails,
> it's better also make those functions reture something like boolean
> instead of void. Then we could avoid checking @err/@errp.
> 
> Thus, the remaining cleanups include:
> * related conversion of local @err to @errp and
> * make the callee function return someting,
> 
> I can do these centrally in another series, and this series (part 1 +
> part 2, 33 patches in total) will only be used for adding ERRP_GUARD(),
> which makes it easier to review as well as easier to merge.
> 
> Do you agree? ;-)

Fine for me.

  Thomas
Zhao Liu March 8, 2024, 10:12 a.m. UTC | #4
Hi Thomas,

> >   void s390_realize_cpu_model(CPUState *cs, Error **errp)
> >   {
> > +    ERRP_GUARD();
> >       Error *err = NULL;
> 
> I think that function could use an additional clean-up now: Remove the local
> "err" variable and use "errp" only instead.
>

Since many cases check @err to determine if the callee function fails,
it's better also make those functions reture something like boolean
instead of void. Then we could avoid checking @err/@errp.

Thus, the remaining cleanups include:
* related conversion of local @err to @errp and
* make the callee function return someting,

I can do these centrally in another series, and this series (part 1 +
part 2, 33 patches in total) will only be used for adding ERRP_GUARD(),
which makes it easier to review as well as easier to merge.

Do you agree? ;-)

Thanks,
Zhao
diff mbox series

Patch

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index a63d990e4e8e..1a1c09612271 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -503,6 +503,7 @@  static void error_prepend_missing_feat(const char *name, void *opaque)
 static void check_compatibility(const S390CPUModel *max_model,
                                 const S390CPUModel *model, Error **errp)
 {
+    ERRP_GUARD();
     S390FeatBitmap missing;
 
     if (model->def->gen > max_model->def->gen) {
@@ -566,6 +567,7 @@  S390CPUModel *get_max_cpu_model(Error **errp)
 
 void s390_realize_cpu_model(CPUState *cs, Error **errp)
 {
+    ERRP_GUARD();
     Error *err = NULL;
     S390CPUClass *xcc = S390_CPU_GET_CLASS(cs);
     S390CPU *cpu = S390_CPU(cs);