Re: [PATCH v2 5/7] zram: support idle/huge page writeback

[Joey Pabalinas]

On Tue, Nov 27, 2018 at 11:13:27AM +0900, Minchan Kim wrote:
> On Sun, Nov 25, 2018 at 11:47:37PM -1000, Joey Pabalinas wrote:
> > On Mon, Nov 26, 2018 at 05:28:11PM +0900, Minchan Kim wrote:
> > > +	strlcpy(mode_buf, buf, sizeof(mode_buf));
> > > +	/* ignore trailing newline */
> > > +	sz = strlen(mode_buf);
> > 
> > One possible idea would be to use strscpy() instead and directly assign
> > the return value to sz, avoiding an extra strlen() call (though you would
> > have to check if `sz == -E2BIG` and do `sz = sizeof(mode_buf) - 1` in that
> > case).
> 
> Thanks for the suggstion.
> If I limit destination buffer smaller, I couldn't meet -E2BIG?

-E2BIG return from strscpy() just means the string was longer than would
fit in the length passed as the third argument. This means only
`sizeof(mode_buf) - 1` bytes were copied into the dest, with the last
byte being the \0 terminator.

You can find the function source in lib/string.c.

> > > +	ret = len;
> > > +	 __free_page(page);
> > > +release_init_lock:
> > > +	up_read(&zram->init_lock);
> > > +	return ret;
> > 
> > Hm, I noticed that this function either returns an error or just the passed
> > in len on success, and I'm left wondering if there might be other useful
> > information which could be passed back to the caller instead. I can't
> > immediately think of any such information, though, so it's possible I'm
> > just daydreaming :)
> 
> It is write syscall semantic of sysfs so not sure it's doable to pass
> other value to user.

Well, with the write system call you can have partial writes, in which
case it would return the (short) number of bytes written. But in this
function, even if there was some sort of partial write possible, this
function still only every returns len or error.

Neither of these are that important though, so Ack from me with or
without these two suggestions.

Reviewed-by: Joey Pabalinas <joeypabalinas@xxxxxxxxx>

-- 
Cheers,
Joey Pabalinas

Attachment: signature.asc
Description: PGP signature