On Fri, 17 Dec 2021 11:48:28 +0800
xkernel.wang@xxxxxxxxxxx wrote:
> From: Xiaoke Wang <xkernel.wang@xxxxxxxxxxx>
>
> kstrdup() is also a memory allocation-related function, it return NULL
> when some memory errors happen. So it is better to check the return
> value of it so to catch the memory error in time. Besides, there should
> have a kfree() to clear up the allocation if we get a failure later in
> this function to prevent memory leak.
>
> Signed-off-by: Xiaoke Wang <xkernel.wang@xxxxxxxxxxx>
Hi a few minor follow on comments. Sorry it took me so long to get back to
you on this. I wasn't rushing as I won't be picking up fixes until the merge
window is done and I can rebase on rc1.
> ---
> Changelog:
> 1. Clean up some error labels(error_ret & error_kzalloc), directly make
> them reflect what they are clearing up and return.
> 2. Clear up indio_dev->name on error path. I put that under label
> `error_free_device`, as kfree(NULL) is safe. Or I may need to add an
> another label as the traget of `goto`.
> Note: Suggestions are from Jonathan Cameron <jic23@xxxxxxxxxxxxxxxxxxxxx>
> ---
> drivers/iio/dummy/iio_simple_dummy.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/dummy/iio_simple_dummy.c b/drivers/iio/dummy/iio_simple_dummy.c
> index c0b7ef9..430c12a 100644
> --- a/drivers/iio/dummy/iio_simple_dummy.c
> +++ b/drivers/iio/dummy/iio_simple_dummy.c
> @@ -577,7 +577,7 @@ static struct iio_sw_device *iio_dummy_probe(const char *name)
> swd = kzalloc(sizeof(*swd), GFP_KERNEL);
> if (!swd) {
> ret = -ENOMEM;
> - goto error_kzalloc;
> + return ERR_PTR(ret);
return ERR_PTR(-ENOMEM);
> }
> /*
> * Allocate an IIO device.
> @@ -589,8 +589,9 @@ static struct iio_sw_device *iio_dummy_probe(const char *name)
> */
> indio_dev = iio_device_alloc(parent, sizeof(*st));
> if (!indio_dev) {
> + kfree(swd);
Ah. Not what I meant unfortunately. This should look something like.
if (!indio_dev) {
ret = -ENOMEM;
goto error_free_swd;
}
With error_ret label renamed to error_free_swd to reflect where it is.
> ret = -ENOMEM;
> - goto error_ret;
> + return ERR_PTR(ret);
> }
>
> st = iio_priv(indio_dev);
> @@ -616,6 +617,10 @@ static struct iio_sw_device *iio_dummy_probe(const char *name)
> * indio_dev->name = spi_get_device_id(spi)->name;
> */
> indio_dev->name = kstrdup(name, GFP_KERNEL);
> + if (!indio_dev->name) {
> + ret = -ENOMEM;
> + goto error_free_device;
> + }
>
> /* Provide description of available channels */
> indio_dev->channels = iio_dummy_channels;
> @@ -650,10 +655,9 @@ static struct iio_sw_device *iio_dummy_probe(const char *name)
> error_unregister_events:
> iio_simple_dummy_events_unregister(indio_dev);
> error_free_device:
> + kfree(indio_dev->name);
> iio_device_free(indio_dev);
> -error_ret:
> kfree(swd);
> -error_kzalloc:
> return ERR_PTR(ret);
> }
>
