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);
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))
+ len = -EINVAL;
+
up_write(&zram->init_lock);
return len;
}
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>