On Tue, Apr 15, 2008 at 2:03 PM, Miklos Szeredi <miklos@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
