Re: [PATCH] zram: check comp algorithm availability earlier

[Minchan Kim <minchan@kernel.org>]

Hello Sergey,

On Tue, May 26, 2015 at 10:13:37PM +0900, Sergey Senozhatsky wrote:
> Improvement idea by Marcin Jabrzyk.
> 
> comp_algorithm_store() silently accepts any supplied algorithm
> name, because zram performs algorithm availability check later,
> during the device configuration phase in disksize_store() and
> prints
>   "zram: Cannot initialise %s compressing backend"
> to syslog. this error line is somewhat generic and, besides,
> can indicate a failed attempt to allocate compression backend's
> working buffers.
> 
> make algorithm availability check earlier, in comp_algorithm_store(),
> and be move verbose:
> 
>   echo lzz > /sys/block/zram0/comp_algorithm
>   -bash: echo: write error: Invalid argument
> 
> dmesg:
>   zram: Error: unavailable compression algorithm: lzz
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Reported-by: Marcin Jabrzyk <m.jabrzyk@samsung.com>
> ---
>  drivers/block/zram/zcomp.c    | 5 +++++
>  drivers/block/zram/zcomp.h    | 1 +
>  drivers/block/zram/zram_drv.c | 6 ++++++
>  3 files changed, 12 insertions(+)
> 
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index a1a8b8e..e10e2b4 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -320,6 +320,11 @@ void zcomp_destroy(struct zcomp *comp)
>  	kfree(comp);
>  }
>  
> +bool zcomp_available_algorithm(const char *comp)
> +{
> +	return find_backend(comp) != NULL;
> +}
> +
>  /*
>   * search available compressors for requested algorithm.
>   * allocate new zcomp and initialize it. return compressing
> diff --git a/drivers/block/zram/zcomp.h b/drivers/block/zram/zcomp.h
> index c59d1fc..46e2b9f 100644
> --- a/drivers/block/zram/zcomp.h
> +++ b/drivers/block/zram/zcomp.h
> @@ -51,6 +51,7 @@ struct zcomp {
>  };
>  
>  ssize_t zcomp_available_show(const char *comp, char *buf);
> +bool zcomp_available_algorithm(const char *comp);
>  
>  struct zcomp *zcomp_create(const char *comp, int max_strm);
>  void zcomp_destroy(struct zcomp *comp);
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 28f6e46..e17b73e 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -378,6 +378,12 @@ static ssize_t comp_algorithm_store(struct device *dev,
>  	if (sz > 0 && zram->compressor[sz - 1] == '\n')
>  		zram->compressor[sz - 1] = 0x00;
>  
> +	if (!zcomp_available_algorithm(zram->compressor)) {
> +		pr_err("Error: unavailable compression algorithm: %s\n",
> +				zram->compressor);
> +		len = -EINVAL;
> +	}
> +

I'm not against this patch because it's better than old.
But let's think more about the pr_err part.

If user try to set wrong algo name, he can see EINVAL.
Isn't it enough?

I think every sane admin can think he passed wrong argument
if he sees -EINVAL.
So, I don't think we need to emit pr_err in here.

The reason I am paranoid about that is that I really don't want
to argue with syslog info which is part of ABI or not in future.
If possible, I don't want to depend on pr_xxx.


>  	up_write(&zram->init_lock);
>  	return len;
>  }
> -- 
> 2.4.1.314.g9532ead
> 

-- 
Kind regards,
Minchan Kim