xfs
[Top] [All Lists]

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

To: aelder@xxxxxxx
Subject: Re: [patch] xfsprogs: unhandled error check in libxfs_trans_read_buf
From: Ajeet Yadav <ajeet.yadav.77@xxxxxxxxx>
Date: Tue, 8 Feb 2011 11:22:27 +0900
Cc: xfs@xxxxxxxxxxx, Eric Sandeen <sandeen@xxxxxxxxxxx>
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=ZaDDW4DdaZ8r6kxBqw2z+Uw+JcCZqyP3rIdobWCPK4M=; b=CxpqlupOxsmZJcz2ry3io1bD78YsCkztqvFDDcjYtRIVdvwYR3IyM+XPVHYAqVKLlF EUJXpCx5Ip1IWupayV+Jkr4dhmzF8c+7/2SOtUu/4/N65XgBGqOAc2Luc5LYduT8U52+ 7CqfPhQGTx3/Yau8zDCvsqWnvnylad9kV1Rls=
Domainkey-signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=hX8fPFSGvEzETKM4IiKw+4pcaNPVfQXSU3XxyLsq4O/V6qrkhPBRhw23h08QV0W2nr 7UVBjhTE2yKwvLs9uVaGAI+SShLzJql1OuQkL8yQQK0eaQxoysKXfHeX5K2UbXtZlwbD YTmZ4DW7DlEaxjNI8+saPEumn/buvGs/Q5uQs=
In-reply-to: <1297127823.2078.24.camel@doink>
References: <AANLkTim2nm9SXPmpRUmK8eWxXYFkPcrzc-gw2ceJ-m2-@xxxxxxxxxxxxxx> <1297127823.2078.24.camel@doink>
Thank you for providing review comment.

On Tue, Feb 8, 2011 at 10:17 AM, Alex Elder <aelder@xxxxxxx> wrote:
> 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@xxxxxxxxx>
>>
>> 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@xxxxxxxxxxx
>> http://oss.sgi.com/mailman/listinfo/xfs
>
>
>
>

<Prev in Thread] Current Thread [Next in Thread>