lkml.org 
[lkml]   [2008]   [Dec]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC v11][PATCH 03/13] General infrastructure for checkpoint restart


On Tue, 16 Dec 2008, Mike Waychison wrote:
>
> set_fs(fs) here

Btw, this all is an excellent example of why people should try to aim for
small functions and use lots of them.

It's often _way_ more readable to do

static inline int __some_fn(...)
{
.. do the real work here ..
}

int some_fn(...)
{
int retval;

prepare();
retval = __some_fn(..)
finish();

return retval;
}

where "prepare/finish" can be about locking, or set_fs(), or allocation
and de-allocation of temporary buffers, or any number of things like that.

With set_fs() in particular, the wrapper function also tends to be the
perfect place to change a regular (kernel) pointer into a user pointer.
IOW, it's the place to make sparse happy, where you can do things like

uptr = (__force void __user *)ptr;

and comment on the fact that the forced user pointer cast is valid only
because of the set_fs().

Because it looks like the code isn't sparse-clean.

Btw, I also think that code like this is bogus:

nwrite = file->f_op->write(file, addr, nleft, &file->f_pos);

because you're not supposed to pass in the raw file->f_pos to that
function. It's fundamentally thread-unsafe. I realize that maybe you don't
care, but the thing is, you're supposed to do

loff_t pos = file->pos;
..
nwrite = file->f_op->write(file, addr, nleft, &pos);
..
file->f_pos = pos;

and in fact preferably use "file_pos_read()" and "file_pos_write()" (but
we've never exposed them outside of fs/read_write.c, so I guess we should
do that).

And yes, I realize that some code does take the address of f_pos directly
(splice, nfsctl, others), and I realize that it works, but it's still bad
form. Please don't add more of them.

Linus


\
 
 \ /
  Last update: 2008-12-16 20:31    [W:1.037 / U:0.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site