Re: [patch] xfsprogs: unhandled error check in libxfs_trans_read_buf

From: Alex Elder <aelder@sgi.com>
To: Ajeet Yadav <ajeet.yadav.77@gmail.com>
Cc: Eric Sandeen <sandeen@sandeen.net>, xfs@oss.sgi.com
Subject: Re: [patch] xfsprogs: unhandled error check in libxfs_trans_read_buf
Date: Mon, 07 Feb 2011 19:17:03 -0600	[thread overview]
Message-ID: <1297127823.2078.24.camel@doink> (raw)
In-Reply-To: <AANLkTim2nm9SXPmpRUmK8eWxXYFkPcrzc-gw2ceJ-m2-@mail.gmail.com>

On Thu, 2011-02-03 at 15:17 +0900, Ajeet Yadav wrote:
> xfsprogs: unhandled error check in libxfs_trans_read_buf

Sorry you didn't get a response earlier.  Reporting
problems like this--especially if they come with a
proposed fix--is always appreciated.

> libxfs_trans_read_buf() is used in both mkfs.xfs & xfs_repair.
> During stability testing we found some time occur pagefault in mkfs.xfs,
> code inspection shows that if libxfs_readbuf() fails then occurs a
> page fault in xfs_buf_item_init() called in libxfs_trans_read_buf().
> 
> mkfs.xfs: unhandled page fault (11) at 0x00000070, code 0x017
> 
> Added NULL check and errno handling.

I expect there are other similar problems lurking in
the libxfs code.

I think your change looks good, with one exception,
noted below.  I will gladly adjust your patch for
you if you consent to the change I suggest.

					-Alex

> Signed-off-by: Ajeet Yadav <ajeet.yadav.77@gmail.com>
> 
> diff -Nurp xfsprogs/libxfs/rdwr.c xfsprogs-dirty/libxfs/rdwr.c
> --- xfsprogs/libxfs/rdwr.c      2011-01-28 20:22:11.000000000 +0900
> +++ xfsprogs-dirty/libxfs/rdwr.c        2011-02-02 16:59:16.000000000 +0900
> @@ -207,10 +207,11 @@ libxfs_trace_readbuf(const char *func, c
>  {
>         xfs_buf_t       *bp = libxfs_readbuf(dev, blkno, len, flags);
> 
> -       bp->b_func = func;
> -       bp->b_file = file;
> -       bp->b_line = line;
> -
> +       if (bp){
> +               bp->b_func = func;
> +               bp->b_file = file;
> +               bp->b_line = line;
> +       }
>         return bp;
>  }
> 
> @@ -485,6 +486,7 @@ libxfs_readbuf(dev_t dev, xfs_daddr_t bl
>                 error = libxfs_readbufr(dev, blkno, bp, len, flags);
>                 if (error) {
>                         libxfs_putbuf(bp);
> +                       errno = error;

I think that libxfs_readbuf() simply returns the
value of the special global "errno" if there is
an error.  And I believe that at this point it
will not have been changed, so there's no need
to assign it here.

In other words, I'd like to remove this one piece
of your patch.

>                         return NULL;
>                 }
>         }
> diff -Nurp xfsprogs/libxfs/trans.c xfsprogs-dirty/libxfs/trans.c
> --- xfsprogs/libxfs/trans.c     2011-01-28 20:22:11.000000000 +0900
> +++ xfsprogs-dirty/libxfs/trans.c       2011-02-02 17:00:42.000000000 +0900
> @@ -508,6 +508,10 @@ libxfs_trans_read_buf(
>         }
> 
>         bp = libxfs_readbuf(dev, blkno, len, flags);
> +       if (!bp){
> +               *bpp = NULL;
> +               return errno;
> +       }
>  #ifdef XACT_DEBUG
>         fprintf(stderr, "trans_read_buf buffer %p, transaction %p\n", bp, tp);
>  #endif
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs