Re: [B.A.T.M.A.N.] [PATCH] IPv4 multicast distribution support.

[Jonathan Haws <Jonathan.Haws@sdl.usu.edu>]

> Patch subject prefix is wrong (actually, it is missing).

Good catch.  Just missed it.  I'll modify the patch with that included.

> > 
> > +int ipv4_to_mac(const alfred_addr *addr, struct ether_addr *mac)
> > +{
> > +  mac->ether_addr_octet[0] = 0;
> > +  mac->ether_addr_octet[1] = 0;
> > +  mac->ether_addr_octet[2] = (addr->ipv4.s_addr >> 24) & 0xFF;
> > +  mac->ether_addr_octet[3] = (addr->ipv4.s_addr >> 16) & 0xFF;
> > +  mac->ether_addr_octet[4] = (addr->ipv4.s_addr >>  8) & 0xFF;
> > +  mac->ether_addr_octet[5] = (addr->ipv4.s_addr >>  0) & 0xFF;
> > +
> > +  if (!is_valid_ether_addr(mac->ether_addr_octet))
> > +    return -EINVAL;
> > +
> > +  return 0;
> > +}
> 
> This will not return the mac address of the device. It will therefore
> break
> the synchronization code. see SOURCE_FIRST_HAND in sync_data and the
> code
> which sets data_source in finish_alfred_push_data.

You are correct - this does not return the MAC address of the device.
 Rather it uses the source IP address.  Since we're dealing with IPv4
in this case I believe it is safe to assume that the network has been
properly configured and the remote node will have a valid IPv4 address,
which is used here.  In my testing the synchronization and data sharing
worked properly and all data was shared as expected.

What would you like to see here?  Would you prefer an ARP request to go
and get the MAC address of the remote node?

> > 
> > @@ -61,6 +62,7 @@ static void alfred_usage(void)
> >         printf("                                      other
> > masters\n");
> >         printf("  -p, --sync-period [period]          set
> > synchronization period, in seconds\n");
> >         printf("                                      fractional
> > seconds are supported (i.e. 0.2 = 5 Hz)\n");
> > +       printf("  -4                                  specify IPv4
> > multicast address and operate in IPv4 mode");
> >         printf("\n");
> >         printf("  -u, --unix-path [path]              path to unix
> > socket used for client-server\n");
> >         printf("                                      communication
> > (default: \""ALFRED_SOCK_PATH_DEFAULT"\")\n");
> 
> The documentation is wrong. It requires an argument but the argument
> is not
> shown.
> 
> Same problem in the manpage.

Correct - again, my mistake.  I'll fix it in the updated patch.

> > I realize that the code in this patch is not formatted properly,
> > but I
> > was unable to get checkpatch.pl to scan this right - it needs a
> > full
> > kernel tree.  Is there another formatting script I can run?
> What is the problem with downloading the kernel sources? And auto-
> formatting
> scripts tend to not get everything right and make things worse in
> some cases.
> checkpatch.pl is therefore only to point out some obvious problems.

I did download the kernel source, but when I try to run the script it
either tells me that I don't have a valid kernel tree (when using the
sources for my Xubuntu kernel) or that the file I'm trying to scan
isn't part of the tree (when using the latest from kernel.org).  I've
never used that tool in the past - can you send me a usage example for
scanning a file in the alfred tree?

> For example that you are replace tabs (8 spaces wide in alfred code)
> sometimes
> with 2 spaces.

Yes - I'd love for checkpatch.pl to scan it and give me a list of what
to fix to make sure I don't miss anything.


Thanks!