On Tue, Sep 08, 2015 at 06:54:28PM +0900, Sergey Senozhatsky wrote: > On (09/08/15 17:16), Minchan Kim wrote: > > we should help them to *correct* it rather than keeping such weired > > thing. > > A simple quiz > > A) > echo zzz > /sys/block/zram0/comp_algorithm > /dev/null > echo 1G > /sys/block/zram0/disksize > -bash: echo: write error: Invalid argument > echo $? > 1 > > > B) > echo zzz > /sys/block/zram0/comp_algorithm > /dev/null > echo 1G > /sys/block/zram0/disksize > echo $? > 0 > > > which one *DOES* help finding and correcting an error and which one *DOES > NOT*? > a million dolla question.
Wrong quiz. For the helping finding, user should do this. echo zzz > /sys/block/zram0/comp_algorithm echo $? If he want to not show error message, he should do. echo zzz > /sys/block/zram/comp_algorithm 2> /dev/null echo $? > > the difference between comp_algorithm store and any other store function - is > that comp_algorithm_store DOES ABSOLUTELY NOTHING. it does not allocate > memory, > free memory, register or unregister anything, change backends, etc., etc., > etc. > it does none of those. its only purpose is to strcpy() given data. this data > will be used later by a completely different function as a result of > additional > actions taken by user space. You are saying implementation of kernel, not interface itself. Nothing different. It's a interface to say something from the user to the kenrel. If user passes wrong input, normally, kernel should return a error and user should check it. It's a general rule for the programming for several decades. > > Returning back to our quiz. I do suspect that the answer is... "B"! > > > So, I still NACK the patch. It introduces a goto label, etc. In fact all > we need to do is to move zcomp_available_algorithm() up, before we grab > the ->init_lock. zcomp_available_algorithm() does not depend on anything > that requires a ->init_lock protection. > > Next, the patch lacks a reasoning/motivation in its commit message. What > we do in fact here is we introduce compression algorithm fallback to a > previously selected (knowingly supported, which has already passed > zcomp_available_algorithm()) or the `default_compressor'. > > Summarizing, it's something like this: > > --- > > From: Sergey SENOZHATSKY <sergey.senozhat...@gmail.com> > Subject: [PATCH] zram: introduce comp algorithm fallback functionality > > When user supplies an unsupported compression algorithm, keep > a previously selected one (knowingly supported) or the default > one (in case if compression algorithm hasn't been changed before). > --- > drivers/block/zram/zram_drv.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c > index 55e09db..255d68b 100644 > --- a/drivers/block/zram/zram_drv.c > +++ b/drivers/block/zram/zram_drv.c > @@ -365,6 +365,9 @@ static ssize_t comp_algorithm_store(struct device *dev, > struct zram *zram = dev_to_zram(dev); > size_t sz; > > + if (!zcomp_available_algorithm(buf)) > + return -EINVAL; If you ignore tailling space, your change would make a bug. If you fix it, it looks good to me. I hope Luis can send it with his SOB and indication of your credit about checking with avoiding unnecessary locking if you don't mind. Thanks, Guys! > + > down_write(&zram->init_lock); > if (init_done(zram)) { > up_write(&zram->init_lock); > @@ -378,9 +381,6 @@ 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)) > - len = -EINVAL; > - > up_write(&zram->init_lock); > return len; > } > -- > 2.5.1.454.g1616360 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/