Re: [PATCH 0/3] zram: Replace pr_* with dev_*

[Sergey Senozhatsky]

On (08/07/15 23:58), Minchan Kim wrote:
[..]
> > note the prefix 'zram:' became 'block zram0:'
> 
> It would be better because it's more clear if we can make several
> blocks.

in that case I'd rather prefer to add zram%d to some of the existing
messages, than mix pr_* with dev_*. besides, 'block zram0:' is a bit
too long, a short 'zram0:' looks better.

well... we already return -errno from every path that does interact
with user space, which is (I think) good enough; adding device_ids
to those 'error return paths' may be OK.

for example. suppose zram is a swap device and we have !handle case

        handle = zs_malloc(meta->mem_pool, clen);
        if (!handle) {
                pr_info("Error allocating memory for compressed page: %u, size=%zu\n",
                        index, clen);
                ret = -ENOMEM;
                goto out;
        }

if zs_malloc() failed, then the system is in such big troubles, that
device_id in the message is the last thing anyone will look at.


so I'm skeptical. the benefit is sort of minimal.


[..]
> > 
> > where %d is a supplied max_comp_stream value
> > 
> > From: "Can't change algorithm for initialized device\n"
> > To: "Can't change algorithm to %s for initialized device\n"
> 
> It's okay but hope to have prefix "block zram0"

it's not ok. I haven't tested it, but I think that the error message
is broken. the supplied string most likely will contain trailing
\n (I think people usually use echo, not echo -n), so it will be:
	Can't change algorithm to XXX
	 for initialized device

... haven't tested.


BUT why would compression algorithm name even matter here?? the error
is 'you attempted to configure _an already configured device_', not
'you attempted to configure an already configured device _with XXX_'.
that XXX part is not relevant here, 'already configured device' is.

	-ss
--
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/