Re: [PATCH ethtool 2/3] Add IPv6 support to NFC

[Ben Hutchings <ben@decadent.org.uk>]

[-- Attachment #1: Type: text/plain, Size: 2571 bytes --]

On Wed, 2016-03-16 at 14:53 +0000, Edward Cree wrote:
> On 13/03/16 16:43, Ben Hutchings wrote:
> > 
> > On Mon, 2016-02-15 at 14:59 +0000, Edward Cree wrote:
> > > 
> > > Signed-off-by: Edward Cree <ecree@solarflare.com>
> > [...]
> > > 
> > > @@ -950,6 +1154,19 @@ static int rxclass_get_mask(char *str, unsigned char *p,
> > >                 *(__be32 *)&p[opt->moffset] = ~val;
> > >                 break;
> > >         }
> > > +       case OPT_IP6: {
> > > +               __be32 val[4];
> > > +               int i;
> > > +               err = rxclass_get_ipv6(str, val);
> > > +               if (err)
> > > +                       return -1;
> > > +               for (i = 0; i < 4; i++) {
> > > +                       ((__be32 *)&p[opt->offset])[i] = val[i];
> > > +                       if (opt->moffset >= 0)
> > > +                               ((__be32 *)&p[opt->moffset])[i] = ~val[i];
> > This pointer arithmetic looks terrible.  I think memcpy() would be much
> > clearer here.
> I've changed the version in rxclass_get_val to use memcpy() (and memset() the
> mask).  Unfortunately, we can't do that here, because we need to complement
> the mask valueas we go, and afaik there's no library function to copy-and-
> complement a byte array.

Sorry, I missed the inversion.  But it would still be cleaner to use a
local variable:

			__be32 *field = (__be32 *)&p[opt->offset];

> Glibc does, however, have a function memfrob(), which XORs every byte of an
> arraywiththe constant 42.  Useful feature, that.

It is!

#include <string.h>

int main(void)
{
    char answer;
    memset(&answer, 0, 1);
    memfrob(&answer, 1);
    return answer;
}

> On the other hand, the quoted code is still wrong because it's also writing
> throughopt->offset and checking for opt->moffset>= 0, both daft copy-and-
> paste errors onmypart.  Will fix in next version.
> > 
> > I won't apply patches labelled as "confidential".  You need to stop
> > including this nonsense in your public messages (I thought you fixed
> > this once before).
> In theory it's been fixed harder now - please let me know if not.

Looks like you're sending two copies, one with and one without.  Which
works for me, though it might annoy some recipients...

Ben.

-- 
Ben Hutchings
If you seem to know what you are doing, you'll be given more to do.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]