Re: [PATCH] ceph: match wait_for_completion_timeout return type

[Nicholas Mc Guire]

On Wed, 11 Mar 2015, Yan, Zheng wrote:

> On Tue, Mar 10, 2015 at 11:18 PM, Nicholas Mc Guire <hofrat@xxxxxxxxx> wrote:
> > return type of wait_for_completion_timeout is unsigned long not int. An
> > appropriately named unsigned long is added and the assignment fixed up.
> >
> > Signed-off-by: Nicholas Mc Guire <hofrat@xxxxxxxxx>
> > ---
> >
> > This was only compile tested for x86_64_defconfig + CONFIG_CEPH_FS=m
> >
> > Patch is against 4.0-rc2 linux-next (localversion-next is -next-20150306)
> >
> >  fs/ceph/dir.c |    7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> > index 83e9976..4bee6b7 100644
> > --- a/fs/ceph/dir.c
> > +++ b/fs/ceph/dir.c
> > @@ -1218,6 +1218,7 @@ static int ceph_dir_fsync(struct file *file, loff_t start, loff_t end,
> >         struct ceph_mds_request *req;
> >         u64 last_tid;
> >         int ret = 0;
> > +       unsigned long time_left;
> >
> >         dout("dir_fsync %p\n", inode);
> >         ret = filemap_write_and_wait_range(inode->i_mapping, start, end);
> > @@ -1240,11 +1241,11 @@ static int ceph_dir_fsync(struct file *file, loff_t start, loff_t end,
> >                 dout("dir_fsync %p wait on tid %llu (until %llu)\n",
> >                      inode, req->r_tid, last_tid);
> >                 if (req->r_timeout) {
> > -                       ret = wait_for_completion_timeout(
> > +                       time_left = wait_for_completion_timeout(
> >                                 &req->r_safe_completion, req->r_timeout);
> > -                       if (ret > 0)
> > +                       if (time_left > 0)
> >                                 ret = 0;
> > -                       else if (ret == 0)
> > +                       else if (!time_left)
> >                                 ret = -EIO;  /* timed out */
> >                 } else {
> >                         wait_for_completion(&req->r_safe_completion);
> 
> There are lots of similar codes in kernel. I don't think this code
> causes problem in reality
>
true - there are 38 (of the initial 81 files) left for which no patch has been 
submitted yet - its cleanup in progress.

type correctness I do believe is an issue and code readability as well
so both fixing the type and that name is relevant.

As Wolfram Sang <wsa@xxxxxxxxxxxxx> put it:
<snip>
 'ret' being an int is kind of an idiom, so I'd rather see the variable
 renamed, too, like the other patches do.
<snip>
[http://lkml.iu.edu/hypermail/linux/kernel/1502.1/00084.html]

regarding causing problems - it is hard to say - type missmatch
may go without problems for a long time and then pop up in strange
corner cases. But you are right that it is not fixing any currently
inown incorrect behavior.

The motivation for cleaning this up is also to make static code checkers
happy which eases scanning for incorrect API usage and general bug-hunting,

thx!
hofrat
--
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/