On Mon, Aug 10, 2015 at 10:43:46PM +0200, Martin Pieuchot wrote: > In general these messages do not help. Here we have two cases of > messages, either logged at LOG_INFO or LOG_ERR.
Yes, multiple logs for the same error are bad. But there is a path where we ignore the return code of in6_update_ifa(). in6_update_ifa() in6_ifattach_loopback() in6_ifattach() The latter is called without error check from if_up() ifioctl() ifnewlladdr() in6_control() So we should either propagate the error in these functions or add a log somewhere in the call stack. bluhm > > In the first case a proper error code is returned via ioctl(2), so > the userland application knows it has done something incorrect. > > The second case correspond to a ENOMEM error, which is something that > can happen and is properly handled (goto cleanup). > > Ok? > > Index: netinet6/in6.c > =================================================================== > RCS file: /cvs/src/sys/netinet6/in6.c,v > retrieving revision 1.161 > diff -u -p -r1.161 in6.c > --- netinet6/in6.c 18 Jul 2015 15:05:32 -0000 1.161 > +++ netinet6/in6.c 10 Aug 2015 20:38:04 -0000 > @@ -633,17 +633,10 @@ in6_update_ifa(struct ifnet *ifp, struct > * must be 128. > */ > if (ifra->ifra_dstaddr.sin6_family == AF_INET6) { > - if ((ifp->if_flags & (IFF_POINTOPOINT|IFF_LOOPBACK)) == 0) { > - /* XXX: noisy message */ > - nd6log((LOG_INFO, "in6_update_ifa: a destination can " > - "be specified for a p2p or a loopback IF only\n")); > + if ((ifp->if_flags & (IFF_POINTOPOINT|IFF_LOOPBACK)) == 0) > return (EINVAL); > - } > - if (plen != 128) { > - nd6log((LOG_INFO, "in6_update_ifa: prefixlen should " > - "be 128 when dstaddr is specified\n")); > + if (plen != 128) > return (EINVAL); > - } > } > /* lifetime consistency check */ > lt = &ifra->ifra_lifetime; > @@ -703,10 +696,6 @@ in6_update_ifa(struct ifnet *ifp, struct > */ > if (ia6->ia_prefixmask.sin6_len && > in6_mask2len(&ia6->ia_prefixmask.sin6_addr, NULL) != plen) { > - nd6log((LOG_INFO, "in6_update_ifa: the prefix length of > an" > - " existing (%s) address should not be changed\n", > - inet_ntop(AF_INET6, &ia6->ia_addr.sin6_addr, > - addr, sizeof(addr)))); > error = EINVAL; > goto unlink; > } > @@ -806,14 +795,8 @@ in6_update_ifa(struct ifnet *ifp, struct > ifra->ifra_addr.sin6_addr.s6_addr32[3]; > llsol.sin6_addr.s6_addr8[12] = 0xff; > imm = in6_joingroup(ifp, &llsol.sin6_addr, &error); > - if (!imm) { > - nd6log((LOG_ERR, "in6_update_ifa: " > - "addmulti failed for %s on %s (errno=%d)\n", > - inet_ntop(AF_INET6, &llsol.sin6_addr, > - addr, sizeof(addr)), > - ifp->if_xname, error)); > + if (!imm) > goto cleanup; > - } > LIST_INSERT_HEAD(&ia6->ia6_memberships, imm, i6mm_chain); > > bzero(&mltmask, sizeof(mltmask)); > @@ -867,15 +850,8 @@ in6_update_ifa(struct ifnet *ifp, struct > rtfree(rt); > } > imm = in6_joingroup(ifp, &mltaddr.sin6_addr, &error); > - if (!imm) { > - nd6log((LOG_WARNING, > - "in6_update_ifa: addmulti failed for " > - "%s on %s (errno=%d)\n", > - inet_ntop(AF_INET6, &mltaddr.sin6_addr, > - addr, sizeof(addr)), > - ifp->if_xname, error)); > + if (!imm) > goto cleanup; > - } > LIST_INSERT_HEAD(&ia6->ia6_memberships, imm, i6mm_chain); > > /* > @@ -884,11 +860,6 @@ in6_update_ifa(struct ifnet *ifp, struct > if (in6_nigroup(ifp, hostname, hostnamelen, &mltaddr) == 0) { > imm = in6_joingroup(ifp, &mltaddr.sin6_addr, &error); > if (!imm) { > - nd6log((LOG_WARNING, "in6_update_ifa: " > - "addmulti failed for %s on %s (errno=%d)\n", > - inet_ntop(AF_INET6, &mltaddr.sin6_addr, > - addr, sizeof(addr)), > - ifp->if_xname, error)); > /* XXX not very fatal, go on... */ > } else { > LIST_INSERT_HEAD(&ia6->ia6_memberships, > @@ -935,14 +906,8 @@ in6_update_ifa(struct ifnet *ifp, struct > rtfree(rt); > } > imm = in6_joingroup(ifp, &mltaddr.sin6_addr, &error); > - if (!imm) { > - nd6log((LOG_WARNING, "in6_update_ifa: " > - "addmulti failed for %s on %s (errno=%d)\n", > - inet_ntop(AF_INET6, &mltaddr.sin6_addr, > - addr, sizeof(addr)), > - ifp->if_xname, error)); > + if (!imm) > goto cleanup; > - } > LIST_INSERT_HEAD(&ia6->ia6_memberships, imm, i6mm_chain); > } >