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

On Donnerstag, 19. Januar 2017 21:01:04 CET Jonathan Haws wrote:

@@ -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.

+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)?

• 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.

[....]

• 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.

Kind regards, Sven

[1] https://static.lwn.net/kerneldoc/process/coding-style.html#centralized-exiti...