[PATCH 0/2] kunit: fix minor error path mistakes

[PATCH 0/2] kunit: fix minor error path mistakes

[Wander Lairson Costa]

Hi,

These two patches fix some minor error path mistakes in the device
module.

Wander Lairson Costa (2):
  kunit: unregister the device on error
  kunit: avoid memory leak on device register error

 lib/kunit/device.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
2.44.0

[PATCH 1/2] kunit: unregister the device on error

[Wander Lairson Costa]

kunit_init_device() should unregister the device on bus register error.

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 lib/kunit/device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/kunit/device.c b/lib/kunit/device.c
index abc603730b8e..25c81ed465fb 100644
--- a/lib/kunit/device.c
+++ b/lib/kunit/device.c
@@ -51,7 +51,7 @@ int kunit_bus_init(void)
 
 	error = bus_register(&kunit_bus_type);
 	if (error)
-		bus_unregister(&kunit_bus_type);
+		root_device_unregister(kunit_bus_device);
 	return error;
 }
 
-- 
2.44.0

[PATCH 2/2] kunit: avoid memory leak on device register error

[Wander Lairson Costa]

If the device register fails, free the allocated memory before
returning.

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 lib/kunit/device.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/kunit/device.c b/lib/kunit/device.c
index 25c81ed465fb..d8c09dcb3e79 100644
--- a/lib/kunit/device.c
+++ b/lib/kunit/device.c
@@ -131,6 +131,7 @@ static struct kunit_device *kunit_device_register_internal(struct kunit *test,
 	err = device_register(&kunit_dev->dev);
 	if (err) {
 		put_device(&kunit_dev->dev);
+		kfree(kunit_dev);
 		return ERR_PTR(err);
 	}
 
-- 
2.44.0

Re: [PATCH 1/2] kunit: unregister the device on error

[Markus Elfring]

> kunit_init_device() should unregister the device on bus register error.

* Would another imperative wording be desirable also for this change description?

* Will the tag “Fixes” become relevant here?

Regards,
Markus

Re: [PATCH 1/2] kunit: unregister the device on error

[Wander Lairson Costa]

On Thu, Apr 18, 2024 at 12:06 PM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> > kunit_init_device() should unregister the device on bus register error.
>


> * Would another imperative wording be desirable also for this change description?

It makes sense, I will change the comment description.

>
> * Will the tag “Fixes” become relevant here?

I often forget this tag. I will add it.

>
> Regards,
> Markus
>

Re: [PATCH 2/2] kunit: avoid memory leak on device register error

[Markus Elfring]

> If the device register fails, free the allocated memory before
> returning.

* I suggest to use the word “registration” (instead of “register”)
  in the commit message.

* Would you like to add the tag “Fixes” accordingly?


> +++ b/lib/kunit/device.c
> @@ -131,6 +131,7 @@ static struct kunit_device *kunit_device_register_internal(struct kunit *test,
>  	err = device_register(&kunit_dev->dev);
>  	if (err) {
>  		put_device(&kunit_dev->dev);
> +		kfree(kunit_dev);
>  		return ERR_PTR(err);
>  	}

Common error handling code can be used instead
if an additional label would be applied for a corresponding jump target.

How do you think about to increase the application of scope-based resource management here?

Regards,
Markus

Re: [PATCH 2/2] kunit: avoid memory leak on device register error

[Wander Lairson Costa]

On Thu, Apr 18, 2024 at 12:24 PM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> > If the device register fails, free the allocated memory before
> > returning.
>
> * I suggest to use the word “registration” (instead of “register”)
>   in the commit message.
>
> * Would you like to add the tag “Fixes” accordingly?
>
>
> > +++ b/lib/kunit/device.c
> > @@ -131,6 +131,7 @@ static struct kunit_device *kunit_device_register_internal(struct kunit *test,
> >       err = device_register(&kunit_dev->dev);
> >       if (err) {
> >               put_device(&kunit_dev->dev);
> > +             kfree(kunit_dev);
> >               return ERR_PTR(err);
> >       }
>
> Common error handling code can be used instead
> if an additional label would be applied for a corresponding jump target.
>
> How do you think about to increase the application of scope-based resource management here?
>

I thought about that. But I think the code is simple enough (for now)
to not require an exit label.

> Regards,
> Markus
>

Re: [ 2/2] kunit: avoid memory leak on device register error

[Markus Elfring]

>> Common error handling code can be used instead
>> if an additional label would be applied for a corresponding jump target.
>>
>> How do you think about to increase the application of scope-based resource management here?
>
> I thought about that. But I think the code is simple enough (for now)
> to not require an exit label.

Please follow a known advice (besides other recommended improvements).
https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources

Regards,
Markus