Messages in this thread | | | Date | Tue, 15 Apr 2008 14:33:35 -0400 | From | "Bob Copeland" <> | Subject | Re: [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
| |