From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754647AbbIHJxt (ORCPT ); Tue, 8 Sep 2015 05:53:49 -0400 Received: from mail-pa0-f52.google.com ([209.85.220.52]:34762 "EHLO mail-pa0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752692AbbIHJxq (ORCPT ); Tue, 8 Sep 2015 05:53:46 -0400 Date: Tue, 8 Sep 2015 18:54:28 +0900 From: Sergey Senozhatsky To: Minchan Kim Cc: Sergey Senozhatsky , Luis Henriques , linux-kernel@vger.kernel.org, Sergey Senozhatsky Subject: Re: [PATCH] zram: don't copy invalid compression algorithms Message-ID: <20150908095428.GA14220@swordfish> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150908081610.GA8633@bbox> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. 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. 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; + 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