From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754670AbbIHNcy (ORCPT ); Tue, 8 Sep 2015 09:32:54 -0400 Received: from mail-pa0-f42.google.com ([209.85.220.42]:35072 "EHLO mail-pa0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754346AbbIHNck (ORCPT ); Tue, 8 Sep 2015 09:32:40 -0400 Date: Tue, 8 Sep 2015 22:33:21 +0900 From: Minchan Kim To: Sergey Senozhatsky Cc: Luis Henriques , linux-kernel@vger.kernel.org, Sergey Senozhatsky Subject: Re: [PATCH] zram: don't copy invalid compression algorithms Message-ID: <20150908133303.GA2174@bbox> References: <1441658910-10226-1-git-send-email-luis.henriques@canonical.com> <20150907235635.GA6896@swordfish> <20150908011443.GB19776@bbox> <20150908013338.GF6896@swordfish> <20150908015831.GG6896@swordfish> <20150908045017.GA30100@bbox> <20150908050442.GA609@swordfish> <20150908081610.GA8633@bbox> <20150908095428.GA14220@swordfish> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150908095428.GA14220@swordfish> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > 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 >