Re: [PATCH] ALSA: emu10k1: Refactor copy_to_user() usage in snd_emu10k1_fx8010_ioctl()

[Takashi Iwai <tiwai@xxxxxxx>]

On Sun, 15 Sep 2024 16:24:25 +0200,
Markus Elfring wrote:
> 
> From: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
> Date: Sun, 15 Sep 2024 16:16:45 +0200
> 
> Assign values from selected expressions where copy_to_user() calls
> are involved to additional local variables so that the number of
> kfree() calls can be reduced accordingly.
> 
> This issue was transformed by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>

This doesn't look like an improvement.  With the refactoring, it's
rather harder to read.


thanks,

Takashi

> ---
>  sound/pci/emu10k1/emufx.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/sound/pci/emu10k1/emufx.c b/sound/pci/emu10k1/emufx.c
> index 03efc317e05f..2f20878a78c9 100644
> --- a/sound/pci/emu10k1/emufx.c
> +++ b/sound/pci/emu10k1/emufx.c
> @@ -2496,12 +2496,12 @@ static int snd_emu10k1_fx8010_ioctl(struct snd_hwdep * hw, struct file *file, un
>  		if (!info)
>  			return -ENOMEM;
>  		snd_emu10k1_fx8010_info(emu, info);
> -		if (copy_to_user(argp, info, sizeof(*info))) {
> +
> +		{
> +			unsigned long ctu = copy_to_user(argp, info, sizeof(*info));
>  			kfree(info);
> -			return -EFAULT;
> +			return ctu ? -EFAULT : 0;
>  		}
> -		kfree(info);
> -		return 0;
>  	case SNDRV_EMU10K1_IOCTL_CODE_POKE:
>  		if (!capable(CAP_SYS_ADMIN))
>  			return -EPERM;
> @@ -2517,12 +2517,12 @@ static int snd_emu10k1_fx8010_ioctl(struct snd_hwdep * hw, struct file *file, un
>  		if (IS_ERR(icode))
>  			return PTR_ERR(icode);
>  		res = snd_emu10k1_icode_peek(emu, icode);
> -		if (res == 0 && copy_to_user(argp, icode, sizeof(*icode))) {
> +
> +		{
> +			bool ctu = res == 0 && copy_to_user(argp, icode, sizeof(*icode));
>  			kfree(icode);
> -			return -EFAULT;
> +			return ctu ? -EFAULT : res;
>  		}
> -		kfree(icode);
> -		return res;
>  	case SNDRV_EMU10K1_IOCTL_PCM_POKE:
>  		ipcm = memdup_user(argp, sizeof(*ipcm));
>  		if (IS_ERR(ipcm))
> @@ -2535,12 +2535,12 @@ static int snd_emu10k1_fx8010_ioctl(struct snd_hwdep * hw, struct file *file, un
>  		if (IS_ERR(ipcm))
>  			return PTR_ERR(ipcm);
>  		res = snd_emu10k1_ipcm_peek(emu, ipcm);
> -		if (res == 0 && copy_to_user(argp, ipcm, sizeof(*ipcm))) {
> +
> +		{
> +			bool ctu = res == 0 && copy_to_user(argp, ipcm, sizeof(*ipcm));
>  			kfree(ipcm);
> -			return -EFAULT;
> +			return ctu ? -EFAULT : res;
>  		}
> -		kfree(ipcm);
> -		return res;
>  	case SNDRV_EMU10K1_IOCTL_TRAM_SETUP:
>  		if (!capable(CAP_SYS_ADMIN))
>  			return -EPERM;
> --
> 2.46.0
>