Re: [PATCH] zram: check compressor name before setting it

From: Marcin Jabrzyk
Date: Fri May 22 2015 - 09:34:48 EST



Hello Minchan,

On 22/05/15 15:14, Minchan Kim wrote:
Hello Sergey,

On Fri, May 22, 2015 at 09:44:11PM +0900, Sergey Senozhatsky wrote:
On (05/22/15 11:12), Marcin Jabrzyk wrote:

no.

zram already complains about failed comp backend creation.
it's in dmesg (or syslog, etc.):

"zram: Cannot initialise %s compressing backend"

OK, now I see that. Sorry for the noise.

second, there is not much value in exposing zcomp internals,
especially when the result is just another line in dmesg output.

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.

Although Marcin's description is rather misleading, I like the patch.
Every admin doesn't watch dmesg output. Even people could change loglevel
simply so KERN_INFO would be void in that case.
Sorry for being confusing, at the first time I've overlooked that error message in syslog.
I didn't thought about looking for handling exactly this error in completely different place.


Instant error propagation is more strighforward for user point of view
rather than delaying with depending on another event.

Yes this was my exact motivation.
Instant value can be detected in scripts etc. Easier to debug in
automated environment.


Thanks.


-ss

I'm not for exposing more internals, but getting -EINVAL would be nice I


If this would be ok, I can prepare v2 with better description and with
less exposing zcomp internals.

Best regards,
Marcin Jabrzyk

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/