lkml.org 
[lkml]   [2008]   [Apr]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/7] omfs: add inode routines
On Tue, Apr 15, 2008 at 2:03 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> Hmm, looks like error handling needs a makeover if this is really to
> become example code. See comments inline.

Thanks for the review Miklos.

> > + mark_buffer_dirty(bh);
> > + if (wait) {
> > + sync_dirty_buffer(bh);
> > + if (buffer_req(bh) && !buffer_uptodate(bh))
> > + ret = -EIO;
>
> Hmm, here it sets ret, but doesn't exit. Deliberate?

It was - if sync fails, it should still try writing the mirrors. Plus
bh and bh2 get released subsequently.

> > + iget_failed(inode);
> > + return ERR_PTR(-EIO);
>
> Should be:
>
> if (!bh)
> goto iget_failed;

Nod.

> > +out:
> > + ret = -EINVAL;
> > +

> > + if (bh)
> > + brelse(bh);
>
> This is weird. This should be done by jumping to the proper label
> between the brleses.

Hrm, brelse(NULL) is allowed so the check is suspect anyway. I did
this in a couple of other places, so I'll fix those up too.

Thanks!

--
Bob Copeland %% www.bobcopeland.com


\
 
 \ /
  Last update: 2008-04-15 20:37    [W:0.057 / U:0.580 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site