Re: [PATCH] dm: Use common error handling code in three functions

On Tue, 9 Jun 2026, Markus Elfring wrote:

> From: Markus Elfring <[email protected]>
> Date: Tue, 9 Jun 2026 21:54:38 +0200
> 
> Use additional labels so that a bit of exception handling can be better
> reused at the end of three if branches.
> 
> This issue was detected by using the Coccinelle software.

Hi

I think that jumping into a nested block isn't good practice and it makes 
the code harder to read and maintain. I would redo the patches so that 
they jump to the end of the topmost function block, for example:

static struct hash_cell *alloc_cell(const char *name, const char *uuid,
				    struct mapped_device *md)
{
	struct hash_cell *hc;

	hc = kmalloc_obj(*hc);
	if (!hc)
		goto ret;
...
	hc->name = kstrdup(name, GFP_KERNEL);
	if (!hc->name)
		goto free_hc_ret;
...
	hc->uuid = kstrdup(uuid, GFP_KERNEL);
	if (!hc->uuid)
		goto free_hc_name_ret;
	
...
	return hc;

free_hc_name_ret:
	kfree(hc->name);
free_hc_ret:
	kfree(hc);
ret:
	return NULL;
}

Mikulas

> Signed-off-by: Markus Elfring <[email protected]>
> ---
>  drivers/md/dm-ioctl.c  | 7 +++----
>  drivers/md/dm-raid1.c  | 4 ++--
>  drivers/md/dm-stripe.c | 7 +++----
>  3 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> index ac77dc0ca225..9537e60f94c3 100644
> --- a/drivers/md/dm-ioctl.c
> +++ b/drivers/md/dm-ioctl.c
> @@ -227,10 +227,8 @@ static struct hash_cell *alloc_cell(const char *name, const char *uuid,
>  		return NULL;
>  
>  	hc->name = kstrdup(name, GFP_KERNEL);
> -	if (!hc->name) {
> -		kfree(hc);
> -		return NULL;
> -	}
> +	if (!hc->name)
> +		goto free_hc;
>  
>  	if (!uuid)
>  		hc->uuid = NULL;
> @@ -239,6 +237,7 @@ static struct hash_cell *alloc_cell(const char *name, const char *uuid,
>  		hc->uuid = kstrdup(uuid, GFP_KERNEL);
>  		if (!hc->uuid) {
>  			kfree(hc->name);
> +free_hc:
>  			kfree(hc);
>  			return NULL;
>  		}
> diff --git a/drivers/md/dm-raid1.c b/drivers/md/dm-raid1.c
> index de5c00704e69..359ca80a67b3 100644
> --- a/drivers/md/dm-raid1.c
> +++ b/drivers/md/dm-raid1.c
> @@ -915,8 +915,7 @@ static struct mirror_set *alloc_context(unsigned int nr_mirrors,
>  	ms->io_client = dm_io_client_create();
>  	if (IS_ERR(ms->io_client)) {
>  		ti->error = "Error creating dm_io client";
> -		kfree(ms);
> -		return NULL;
> +		goto free_ms;
>  	}
>  
>  	ms->rh = dm_region_hash_create(ms, dispatch_bios, wakeup_mirrord,
> @@ -926,6 +925,7 @@ static struct mirror_set *alloc_context(unsigned int nr_mirrors,
>  	if (IS_ERR(ms->rh)) {
>  		ti->error = "Error creating dirty region hash";
>  		dm_io_client_destroy(ms->io_client);
> +free_ms:
>  		kfree(ms);
>  		return NULL;
>  	}
> diff --git a/drivers/md/dm-stripe.c b/drivers/md/dm-stripe.c
> index 750865fd3ae7..58fa0badc90f 100644
> --- a/drivers/md/dm-stripe.c
> +++ b/drivers/md/dm-stripe.c
> @@ -148,10 +148,8 @@ static int stripe_ctr(struct dm_target *ti, unsigned int argc, char **argv)
>  		sc->stripes_shift = __ffs(stripes);
>  
>  	r = dm_set_target_max_io_len(ti, chunk_size);
> -	if (r) {
> -		kfree(sc);
> -		return r;
> -	}
> +	if (r)
> +		goto free_sc;
>  
>  	ti->num_flush_bios = stripes;
>  	ti->num_discard_bios = stripes;
> @@ -176,6 +174,7 @@ static int stripe_ctr(struct dm_target *ti, unsigned int argc, char **argv)
>  			ti->error = "Couldn't parse stripe destination";
>  			while (i--)
>  				dm_put_device(ti, sc->stripe[i].dev);
> +free_sc:
>  			kfree(sc);
>  			return r;
>  		}
> -- 
> 2.54.0
>