Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751382AbbEYOV6 (ORCPT ); Mon, 25 May 2015 10:21:58 -0400 Received: from mail-pd0-f178.google.com ([209.85.192.178]:36318 "EHLO mail-pd0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751045AbbEYOV4 (ORCPT ); Mon, 25 May 2015 10:21:56 -0400 Date: Mon, 25 May 2015 23:21:49 +0900 From: Minchan Kim To: Sergey Senozhatsky Cc: Marcin Jabrzyk , 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: <20150525142149.GD14922@blaptop> 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> <20150525061838.GB555@swordfish> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150525061838.GB555@swordfish> 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: 5496 Lines: 158 On Mon, May 25, 2015 at 03:18:38PM +0900, Sergey Senozhatsky wrote: > 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); find_backend is just utility function to get zcomp_backend. IOW, it might be used for several cases in future so I want make error report as caller's work. > 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)) In here, we could report back to the user. > + len = -EINVAL; > + > up_write(&zram->init_lock); > return len; > } > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majordomo@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: email@kvack.org -- Kind regards, Minchan Kim -- 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/