Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751040AbbEYGS3 (ORCPT ); Mon, 25 May 2015 02:18:29 -0400 Received: from mail-pd0-f182.google.com ([209.85.192.182]:35871 "EHLO mail-pd0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750783AbbEYGSZ (ORCPT ); Mon, 25 May 2015 02:18:25 -0400 Date: Mon, 25 May 2015 15:18:38 +0900 From: Sergey Senozhatsky To: Marcin Jabrzyk Cc: Sergey Senozhatsky , minchan@kernel.org, ngupta@vflare.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, kyungmin.park@samsung.com Subject: Re: [PATCH] zram: check compressor name before setting it Message-ID: <20150525061838.GB555@swordfish> References: <1432283515-2005-1-git-send-email-m.jabrzyk@samsung.com> <20150522085523.GA709@swordfish> <555EF30C.60108@samsung.com> <20150522124411.GA3793@swordfish> <555F2E7C.4090707@samsung.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <555F2E7C.4090707@samsung.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4688 Lines: 139 On (05/22/15 15:26), Marcin Jabrzyk wrote: > >> From the other hand, the only valid values that can be written are > >>in 'comp_algorithm'. > >>So when writing other one, returning -EINVAL seems to be reasonable. > >>The user would get immediately information that he can't do that, > >>now the information can be very deferred in time. > > > >it's not. > >the error message appears in syslog right before we return -EINVAL > >back to user. > > Yes I've read up the code more detailed and I saw that error message > just before returning to user with error value. > > But this happens when 'disksize' is wirtten, not when 'comp_algorithm'. > I understood, the error message in dmesg is clear there is no such > algorithm. > > But this is not an immediate error, when setting the 'comp_algorithm', > where we already know that it's wrong, not existing etc. > Anything after that moment would be wrong and would not work at all. > > From what I saw 'comp_algorithm_store' is the only *_store in zram that > believes user that he writes proper value and just makes strlcpy. > > So what I've ing mind is to provide direct feedback, you have > written wrong name of compressor, you got -EINVAL, please write > correct value. This would be very useful when scripting. > I can't see how printing error 0.0012 seconds earlier helps. really. if one sets a compression algorithm the very next thing to do is to set device's disksize. even if he/she usually watch a baseball game in between, then the error message appears right when it's needed anyway: during `setup my device and make it usable' stage. >I'm not for exposing more internals, but getting -EINVAL would be nice I you are. find_backend() returns back to its caller a raw and completely initialized zcomp_backend pointer. this is very dangerous zcomp internals, which should never be exposed. from zcomp layer we return either ERR_PTR() or a usable zcomp_backend pointer. that's the rule. if you guys still insist that this is critical and very important change, then there should be a small helper function instead with a clear name (starting with zcomp_ to indicate its place) which will simply return bool TRUE for `algorithm is known' case and FALSE otherwise (aka `algorithm is unknown'). something like below: # echo LZ5 > /sys/block/zram0/comp_algorithm -bash: echo: write error: Invalid argument dmesg [ 7440.544852] Error: unknown compression algorithm: LZ5 p.s. but, I still see a very little value. p.p.s notice a necessary and inevitable `dmesg'. (back to 'no one reads syslog' argument). --- drivers/block/zram/zcomp.c | 10 ++++++++++ drivers/block/zram/zcomp.h | 1 + drivers/block/zram/zram_drv.c | 3 +++ 3 files changed, 14 insertions(+) diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c index a1a8b8e..b68b16f 100644 --- a/drivers/block/zram/zcomp.c +++ b/drivers/block/zram/zcomp.c @@ -54,11 +54,16 @@ static struct zcomp_backend *backends[] = { static struct zcomp_backend *find_backend(const char *compress) { int i = 0; + while (backends[i]) { if (sysfs_streq(compress, backends[i]->name)) break; i++; } + + if (!backends[i]) + pr_err("Error: unknown compression algorithm: %s\n", + compress); return backends[i]; } @@ -320,6 +325,11 @@ void zcomp_destroy(struct zcomp *comp) kfree(comp); } +bool zcomp_known_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..773bdf1 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_known_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 f750e34..2197a81 100644 --- a/drivers/block/zram/zram_drv.c +++ b/drivers/block/zram/zram_drv.c @@ -378,6 +378,9 @@ static ssize_t comp_algorithm_store(struct device *dev, if (sz > 0 && zram->compressor[sz - 1] == '\n') zram->compressor[sz - 1] = 0x00; + if (!zcomp_known_algorithm(zram->compressor)) + len = -EINVAL; + up_write(&zram->init_lock); return len; } -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/