xfs
[Top] [All Lists]

Re: [PATCH] One more bugfix for xfs_lowbit64

To: Steve Lord <lord@xxxxxxx>
Subject: Re: [PATCH] One more bugfix for xfs_lowbit64
From: Andi Kleen <ak@xxxxxxxxxxxxx>
Date: 22 Aug 2003 23:51:28 +0200
Date: Fri, 22 Aug 2003 23:51:28 +0200
Cc: Andi Kleen <ak@xxxxxx>, linux-xfs@xxxxxxxxxxx
In-reply-to: <1061586593.6476.9.camel@xxxxxxxxxxxxxxxxxxxx>
References: <20030822201012.GA19026@averell> <1061586593.6476.9.camel@xxxxxxxxxxxxxxxxxxxx>
Sender: linux-xfs-bounce@xxxxxxxxxxx
User-agent: Mutt/1.4.1i
On Fri, Aug 22, 2003 at 04:09:53PM -0500, Steve Lord wrote:
> On Fri, 2003-08-22 at 15:10, Andi Kleen wrote:
> > (mea culpa). The error return was broken too, it would return -2,
> > not -1 for errors. Makes no difference in the callers, they never check
> > for -1, but is still better to conform to the spec.
> > 
> > Includes the previous fix for bits > 32.
> > 
> > -Andi
> > 
> > --- linux-2.6.0test3/fs/xfs/xfs_bit.c-o     2003-05-27 03:00:41.000000000 
> > +0200
> > +++ linux-2.6.0test3/fs/xfs/xfs_bit.c       2003-08-22 22:08:14.000000000 
> > +0200
> > @@ -156,12 +156,12 @@
> >  {
> >     int n;
> >     n = ffs((unsigned)v);
> > -   if (n == 0) {
> > +   if (n < 0) {
> >             n = ffs(v >> 32);
> >             if (n >= 0)
> >                     n+=32;
> >     }
> > -   return n-1;
> > +   return (n < 0) ? n : n-1;
> >  }
> >  
> >  /*
> 
> You know, on second thoughts, are you sure about that? generic_ffs
> and man ffs seem to suggest otherwise.

The i386 ffs does not agree with the man page. Manpage says 0 for 
no bit set, but i386 returns -1:

static __inline__ int ffs(int x)
{
        int r;

        __asm__("bsfl %1,%0\n\t"
                "jnz 1f\n\t"                    /* zero flag set when zero */
                "movl $-1,%0\n"                 /* -1 for zero */
                "1:" : "=r" (r) : "rm" (x));
        return r+1;
}

That's probably a bug in itself, but would be a bit risky to change.

If generic_ffs disagrees with that it's very bad.  That would be 
a generic bug.

It would be probably safer to use __ffs which has better defined
semantics.

-Andi


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