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

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

> > @@ -273,10 +276,15 @@ static struct globals *alfred_init(int argc,
> > char *argv[])
> >  			return NULL;
> >  		case 'p':
> >  			sync_period = strtod(optarg, NULL);
> > -			globals->sync_period.tv_sec = (int)
> > sync_period;
> > -			globals->sync_period.tv_nsec = (double)
> > (sync_period - (int) sync_period) * 1e9;
> > +			globals->sync_period.tv_sec =
> > (int)sync_period;
> > +			globals->sync_period.tv_nsec =
> > (double)(sync_period - (int)sync_period) * 1e9;
> >  			printf(" ** Setting sync interval to: %.9f
> > seconds (%ld.%09ld)\n", sync_period, globals->sync_period.tv_sec,
> > globals->sync_period.tv_nsec);
> >  			break;
> This doesn't seem to belong in this patch.

Yes, that is correct.  I must have fixed that formatting when I was
going through everything else.  Would you like me to take it out of
this patch?

> > +static int netsock_open4(struct interface *interface)
> > +{
> > +	int sock;
> > +	int sock_mc;
> > +	struct sockaddr_in sin4, sin_mc;
> > +	struct ip_mreq mreq;
> > +	struct ifreq ifr;
> > +	int ret;
> > +
> > +	interface->netsock = -1;
> > +	interface->netsock_mcast = -1;
> > +
> > +	sock = socket(PF_INET, SOCK_DGRAM, IPPROTO_UDP);
> > +	if (sock  < 0) {
> > +		perror("ipv4: can't open socket");
> > +		return -1;
> > +	}
> > +
> > +	sock_mc = socket(PF_INET, SOCK_DGRAM, IPPROTO_UDP);
> > +	if (sock_mc  < 0) {
> > +		perror("ipv4: can't open mc socket");
> > +		return -1;
> > +	}
> You don't sock in this situation (on errors)?

Huh, I had saw that as well and thought I had it fixed.  I'll make sure
I didn't just miss a commit.

> > 
> > +	memset(&ifr, 0, sizeof(ifr));
> > +	strncpy(ifr.ifr_name, interface->interface, IFNAMSIZ);
> > +	ifr.ifr_name[IFNAMSIZ - 1] = '\0';
> > +	if (ioctl(sock_mc, SIOCGIFHWADDR, &ifr) == -1) {
> > +		perror("ipv4: can't get MAC address");
> > +		goto err4;
> And now (see err4) you only close sock_mc? This applies to basically
> the rest
> of the function. So I will skip it and not mention every place which
> looks
> suspicious.

Same comment as above.

> > 
> > +
> > +	interface->netsock = sock;
> > +	interface->netsock_mcast = sock_mc;
> > +
> > +	return 0;
> > +err4:
> > +	close(sock_mc);
> > +	return -1;
> > +}
> > +
> You only close sock_mc on errors but not sock?
> 
> And please chose something better [1] than err4.

I see why you'd want me to rename the goto - however it was chosen to
differentiate between the err goto in netsock_open.  I called it err4
since it is for the netsock_open4() function dealing with IPv4.  Would
err_ipv4 be better?