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? > > > + if (!strcmp(mode_buf, "idle")) > > + mode = IDLE_WRITEBACK; > > + if (!strcmp(mode_buf, "huge")) > > + mode = HUGE_WRITEBACK; > > Maybe using `else if (!strcmp(mode_buf, "huge"))` would be slightly > better here, avoiding a second strcmp() if mode_buf has already > matched "idle". I considered "huge|idle" as an option. Anyway, in that case, mode should "mode |= ". At this moment, yes, lets use "else if" since I don't have strong opinion to support "idle|huge". > > > + if ((mode & IDLE_WRITEBACK && > > + !zram_test_flag(zram, index, ZRAM_IDLE)) && > > + (mode & HUGE_WRITEBACK && > > + !zram_test_flag(zram, index, ZRAM_HUGE))) > > + goto next; > > Wouldn't writing this as `mode & (IDLE_WRITEBACK | HUGE_WRITEBACK)` > be a bit easier to read as well as slightly more compact? > > > + 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. > > -- > Cheers, > Joey Pabalinas