| Message ID | 20171107202430.5892-1-rosenp@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [LEDE-DEV] umdns: Replace unnecessary memset calls with {}. | expand |
On 11/07/2017 09:24 PM, Rosen Penev wrote: > Less verbose > > Signed-off-by: Rosen Penev <rosenp@gmail.com> > --- > interface.c | 22 ++++++++-------------- > 1 file changed, 8 insertions(+), 14 deletions(-) > > diff --git a/interface.c b/interface.c > index 7f814d2..18dee52 100644 > --- a/interface.c > +++ b/interface.c > @@ -44,7 +44,7 @@ > static int > interface_send_packet4(struct interface *iface, struct sockaddr_in *to, struct iovec *iov, int iov_len) > { > - static size_t cmsg_data[( CMSG_SPACE(sizeof(struct in_pktinfo)) / sizeof(size_t)) + 1]; > + static size_t cmsg_data[( CMSG_SPACE(sizeof(struct in_pktinfo)) / sizeof(size_t)) + 1] = {}; > static struct sockaddr_in a; > static struct msghdr m = { > .msg_name = (struct sockaddr *) &a, > @@ -61,7 +61,6 @@ interface_send_packet4(struct interface *iface, struct sockaddr_in *to, struct i > m.msg_iov = iov; > m.msg_iovlen = iov_len; > > - memset(cmsg_data, 0, sizeof(cmsg_data)); > cmsg = CMSG_FIRSTHDR(&m); > cmsg->cmsg_len = m.msg_controllen; > cmsg->cmsg_level = IPPROTO_IP; This one looks like a pitfall to me, since without the memset cmsg_data being static is never zeroed again at runtime. Not sure if all cmsg members are initialized after the removed memset, otherwise the change is wrong. Cheers, Zefir
On 11/08/2017 10:57 AM, Zefir Kurtisi wrote: > On 11/07/2017 09:24 PM, Rosen Penev wrote: >> Less verbose >> >> Signed-off-by: Rosen Penev <rosenp@gmail.com> >> --- >> interface.c | 22 ++++++++-------------- >> 1 file changed, 8 insertions(+), 14 deletions(-) >> >> diff --git a/interface.c b/interface.c >> index 7f814d2..18dee52 100644 >> --- a/interface.c >> +++ b/interface.c >> @@ -44,7 +44,7 @@ >> static int >> interface_send_packet4(struct interface *iface, struct sockaddr_in *to, struct iovec *iov, int iov_len) >> { >> - static size_t cmsg_data[( CMSG_SPACE(sizeof(struct in_pktinfo)) / sizeof(size_t)) + 1]; >> + static size_t cmsg_data[( CMSG_SPACE(sizeof(struct in_pktinfo)) / sizeof(size_t)) + 1] = {}; >> static struct sockaddr_in a; >> static struct msghdr m = { >> .msg_name = (struct sockaddr *) &a, >> @@ -61,7 +61,6 @@ interface_send_packet4(struct interface *iface, struct sockaddr_in *to, struct i >> m.msg_iov = iov; >> m.msg_iovlen = iov_len; >> >> - memset(cmsg_data, 0, sizeof(cmsg_data)); >> cmsg = CMSG_FIRSTHDR(&m); >> cmsg->cmsg_len = m.msg_controllen; >> cmsg->cmsg_level = IPPROTO_IP; > > This one looks like a pitfall to me, since without the memset cmsg_data being > static is never zeroed again at runtime. Not sure if all cmsg members are > initialized after the removed memset, otherwise the change is wrong. > > > Cheers, > Zefir Huh, good catch. But I wonder if there is any reason to make these variables static at all - stack-allocated should be fine? I see that re-initialization of the msghdr is avoided, but I don't consider that a good enough reason, as it makes the code less intuitive. (And if kept static, it should at least get a comment explaining it.) Matthias
Hi, comments inline. > --- > interface.c | 22 ++++++++-------------- > 1 file changed, 8 insertions(+), 14 deletions(-) > > diff --git a/interface.c b/interface.c > index 7f814d2..18dee52 100644 > --- a/interface.c > +++ b/interface.c > @@ -44,7 +44,7 @@ > static int > interface_send_packet4(struct interface *iface, struct sockaddr_in *to, struct iovec *iov, int iov_len) > { > - static size_t cmsg_data[( CMSG_SPACE(sizeof(struct in_pktinfo)) / sizeof(size_t)) + 1]; > + static size_t cmsg_data[( CMSG_SPACE(sizeof(struct in_pktinfo)) / sizeof(size_t)) + 1] = {}; > static struct sockaddr_in a; > static struct msghdr m = { > .msg_name = (struct sockaddr *) &a, > @@ -61,7 +61,6 @@ interface_send_packet4(struct interface *iface, struct sockaddr_in *to, struct i > m.msg_iov = iov; > m.msg_iovlen = iov_len; > > - memset(cmsg_data, 0, sizeof(cmsg_data)); The cmsg_data buffer is static, wouldn't that mean that it is never zeroed again after the first function call? > cmsg = CMSG_FIRSTHDR(&m); > cmsg->cmsg_len = m.msg_controllen; > cmsg->cmsg_level = IPPROTO_IP; > @@ -84,7 +83,7 @@ interface_send_packet4(struct interface *iface, struct sockaddr_in *to, struct i > static int > interface_send_packet6(struct interface *iface, struct sockaddr_in6 *to, struct iovec *iov, int iov_len) > { > - static size_t cmsg_data[( CMSG_SPACE(sizeof(struct in6_pktinfo)) / sizeof(size_t)) + 1]; > + static size_t cmsg_data[( CMSG_SPACE(sizeof(struct in6_pktinfo)) / sizeof(size_t)) + 1] = {}; > static struct sockaddr_in6 a; > static struct msghdr m = { > .msg_name = (struct sockaddr *) &a, > @@ -101,7 +100,6 @@ interface_send_packet6(struct interface *iface, struct sockaddr_in6 *to, struct > m.msg_iov = iov; > m.msg_iovlen = iov_len; > > - memset(cmsg_data, 0, sizeof(cmsg_data)); Same here, I think the buffer needs to be zeroed on each invocation. > cmsg = CMSG_FIRSTHDR(&m); > cmsg->cmsg_len = m.msg_controllen; > cmsg->cmsg_level = IPPROTO_IPV6; > @@ -186,7 +184,7 @@ read_socket4(struct uloop_fd *u, unsigned int events) > struct iovec iov[1]; > char cmsg[CMSG_SPACE(sizeof(struct in_pktinfo)) + CMSG_SPACE(sizeof(int)) + 1]; > struct cmsghdr *cmsgptr; > - struct msghdr msg; > + struct msghdr msg = {}; > socklen_t len; [snip] ~ Jo
On 07/11/17 21:24, Rosen Penev wrote: > Less verbose > > Signed-off-by: Rosen Penev <rosenp@gmail.com> > --- Hi, this does not fix any bugs and is a pure style change. yeah, it might safe a few bytes but i dont see that as a valid trade-off, sorry ... John > interface.c | 22 ++++++++-------------- > 1 file changed, 8 insertions(+), 14 deletions(-) > > diff --git a/interface.c b/interface.c > index 7f814d2..18dee52 100644 > --- a/interface.c > +++ b/interface.c > @@ -44,7 +44,7 @@ > static int > interface_send_packet4(struct interface *iface, struct sockaddr_in *to, struct iovec *iov, int iov_len) > { > - static size_t cmsg_data[( CMSG_SPACE(sizeof(struct in_pktinfo)) / sizeof(size_t)) + 1]; > + static size_t cmsg_data[( CMSG_SPACE(sizeof(struct in_pktinfo)) / sizeof(size_t)) + 1] = {}; > static struct sockaddr_in a; > static struct msghdr m = { > .msg_name = (struct sockaddr *) &a, > @@ -61,7 +61,6 @@ interface_send_packet4(struct interface *iface, struct sockaddr_in *to, struct i > m.msg_iov = iov; > m.msg_iovlen = iov_len; > > - memset(cmsg_data, 0, sizeof(cmsg_data)); > cmsg = CMSG_FIRSTHDR(&m); > cmsg->cmsg_len = m.msg_controllen; > cmsg->cmsg_level = IPPROTO_IP; > @@ -84,7 +83,7 @@ interface_send_packet4(struct interface *iface, struct sockaddr_in *to, struct i > static int > interface_send_packet6(struct interface *iface, struct sockaddr_in6 *to, struct iovec *iov, int iov_len) > { > - static size_t cmsg_data[( CMSG_SPACE(sizeof(struct in6_pktinfo)) / sizeof(size_t)) + 1]; > + static size_t cmsg_data[( CMSG_SPACE(sizeof(struct in6_pktinfo)) / sizeof(size_t)) + 1] = {}; > static struct sockaddr_in6 a; > static struct msghdr m = { > .msg_name = (struct sockaddr *) &a, > @@ -101,7 +100,6 @@ interface_send_packet6(struct interface *iface, struct sockaddr_in6 *to, struct > m.msg_iov = iov; > m.msg_iovlen = iov_len; > > - memset(cmsg_data, 0, sizeof(cmsg_data)); > cmsg = CMSG_FIRSTHDR(&m); > cmsg->cmsg_len = m.msg_controllen; > cmsg->cmsg_level = IPPROTO_IPV6; > @@ -186,7 +184,7 @@ read_socket4(struct uloop_fd *u, unsigned int events) > struct iovec iov[1]; > char cmsg[CMSG_SPACE(sizeof(struct in_pktinfo)) + CMSG_SPACE(sizeof(int)) + 1]; > struct cmsghdr *cmsgptr; > - struct msghdr msg; > + struct msghdr msg = {}; > socklen_t len; > struct sockaddr_in from; > int flags = 0, ifindex = -1; > @@ -202,7 +200,6 @@ read_socket4(struct uloop_fd *u, unsigned int events) > iov[0].iov_base = buffer; > iov[0].iov_len = sizeof(buffer); > > - memset(&msg, 0, sizeof(msg)); > msg.msg_name = (struct sockaddr *) &from; > msg.msg_namelen = sizeof(struct sockaddr_in); > msg.msg_iov = iov; > @@ -260,7 +257,7 @@ read_socket6(struct uloop_fd *u, unsigned int events) > struct iovec iov[1]; > char cmsg6[CMSG_SPACE(sizeof(struct in6_pktinfo)) + CMSG_SPACE(sizeof(int)) + 1]; > struct cmsghdr *cmsgptr; > - struct msghdr msg; > + struct msghdr msg = {}; > socklen_t len; > struct sockaddr_in6 from; > int flags = 0, ifindex = -1; > @@ -276,7 +273,6 @@ read_socket6(struct uloop_fd *u, unsigned int events) > iov[0].iov_base = buffer; > iov[0].iov_len = sizeof(buffer); > > - memset(&msg, 0, sizeof(msg)); > msg.msg_name = (struct sockaddr *) &from; > msg.msg_namelen = sizeof(struct sockaddr_in6); > msg.msg_iov = iov; > @@ -327,17 +323,16 @@ read_socket6(struct uloop_fd *u, unsigned int events) > static int > interface_mcast_setup4(struct interface *iface) > { > - struct ip_mreqn mreq; > + struct ip_mreqn mreq = {}; > uint8_t ttl = 255; > int no = 0; > - struct sockaddr_in sa = { 0 }; > + struct sockaddr_in sa = {}; > int fd = iface->fd.fd; > > sa.sin_family = AF_INET; > sa.sin_port = htons(MCAST_PORT); > inet_pton(AF_INET, MCAST_ADDR, &sa.sin_addr); > > - memset(&mreq, 0, sizeof(mreq)); > mreq.imr_address.s_addr = iface->v4_addr.s_addr; > mreq.imr_multiaddr = sa.sin_addr; > mreq.imr_ifindex = iface->ifindex; > @@ -368,17 +363,16 @@ interface_mcast_setup4(struct interface *iface) > static int > interface_socket_setup6(struct interface *iface) > { > - struct ipv6_mreq mreq; > + struct ipv6_mreq mreq = {}; > int ttl = 255; > int no = 0; > - struct sockaddr_in6 sa = { 0 }; > + struct sockaddr_in6 sa = {}; > int fd = iface->fd.fd; > > sa.sin6_family = AF_INET6; > sa.sin6_port = htons(MCAST_PORT); > inet_pton(AF_INET6, MCAST_ADDR6, &sa.sin6_addr); > > - memset(&mreq, 0, sizeof(mreq)); > mreq.ipv6mr_multiaddr = sa.sin6_addr; > mreq.ipv6mr_interface = iface->ifindex; >
diff --git a/interface.c b/interface.c index 7f814d2..18dee52 100644 --- a/interface.c +++ b/interface.c @@ -44,7 +44,7 @@ static int interface_send_packet4(struct interface *iface, struct sockaddr_in *to, struct iovec *iov, int iov_len) { - static size_t cmsg_data[( CMSG_SPACE(sizeof(struct in_pktinfo)) / sizeof(size_t)) + 1]; + static size_t cmsg_data[( CMSG_SPACE(sizeof(struct in_pktinfo)) / sizeof(size_t)) + 1] = {}; static struct sockaddr_in a; static struct msghdr m = { .msg_name = (struct sockaddr *) &a, @@ -61,7 +61,6 @@ interface_send_packet4(struct interface *iface, struct sockaddr_in *to, struct i m.msg_iov = iov; m.msg_iovlen = iov_len; - memset(cmsg_data, 0, sizeof(cmsg_data)); cmsg = CMSG_FIRSTHDR(&m); cmsg->cmsg_len = m.msg_controllen; cmsg->cmsg_level = IPPROTO_IP; @@ -84,7 +83,7 @@ interface_send_packet4(struct interface *iface, struct sockaddr_in *to, struct i static int interface_send_packet6(struct interface *iface, struct sockaddr_in6 *to, struct iovec *iov, int iov_len) { - static size_t cmsg_data[( CMSG_SPACE(sizeof(struct in6_pktinfo)) / sizeof(size_t)) + 1]; + static size_t cmsg_data[( CMSG_SPACE(sizeof(struct in6_pktinfo)) / sizeof(size_t)) + 1] = {}; static struct sockaddr_in6 a; static struct msghdr m = { .msg_name = (struct sockaddr *) &a, @@ -101,7 +100,6 @@ interface_send_packet6(struct interface *iface, struct sockaddr_in6 *to, struct m.msg_iov = iov; m.msg_iovlen = iov_len; - memset(cmsg_data, 0, sizeof(cmsg_data)); cmsg = CMSG_FIRSTHDR(&m); cmsg->cmsg_len = m.msg_controllen; cmsg->cmsg_level = IPPROTO_IPV6; @@ -186,7 +184,7 @@ read_socket4(struct uloop_fd *u, unsigned int events) struct iovec iov[1]; char cmsg[CMSG_SPACE(sizeof(struct in_pktinfo)) + CMSG_SPACE(sizeof(int)) + 1]; struct cmsghdr *cmsgptr; - struct msghdr msg; + struct msghdr msg = {}; socklen_t len; struct sockaddr_in from; int flags = 0, ifindex = -1; @@ -202,7 +200,6 @@ read_socket4(struct uloop_fd *u, unsigned int events) iov[0].iov_base = buffer; iov[0].iov_len = sizeof(buffer); - memset(&msg, 0, sizeof(msg)); msg.msg_name = (struct sockaddr *) &from; msg.msg_namelen = sizeof(struct sockaddr_in); msg.msg_iov = iov; @@ -260,7 +257,7 @@ read_socket6(struct uloop_fd *u, unsigned int events) struct iovec iov[1]; char cmsg6[CMSG_SPACE(sizeof(struct in6_pktinfo)) + CMSG_SPACE(sizeof(int)) + 1]; struct cmsghdr *cmsgptr; - struct msghdr msg; + struct msghdr msg = {}; socklen_t len; struct sockaddr_in6 from; int flags = 0, ifindex = -1; @@ -276,7 +273,6 @@ read_socket6(struct uloop_fd *u, unsigned int events) iov[0].iov_base = buffer; iov[0].iov_len = sizeof(buffer); - memset(&msg, 0, sizeof(msg)); msg.msg_name = (struct sockaddr *) &from; msg.msg_namelen = sizeof(struct sockaddr_in6); msg.msg_iov = iov; @@ -327,17 +323,16 @@ read_socket6(struct uloop_fd *u, unsigned int events) static int interface_mcast_setup4(struct interface *iface) { - struct ip_mreqn mreq; + struct ip_mreqn mreq = {}; uint8_t ttl = 255; int no = 0; - struct sockaddr_in sa = { 0 }; + struct sockaddr_in sa = {}; int fd = iface->fd.fd; sa.sin_family = AF_INET; sa.sin_port = htons(MCAST_PORT); inet_pton(AF_INET, MCAST_ADDR, &sa.sin_addr); - memset(&mreq, 0, sizeof(mreq)); mreq.imr_address.s_addr = iface->v4_addr.s_addr; mreq.imr_multiaddr = sa.sin_addr; mreq.imr_ifindex = iface->ifindex; @@ -368,17 +363,16 @@ interface_mcast_setup4(struct interface *iface) static int interface_socket_setup6(struct interface *iface) { - struct ipv6_mreq mreq; + struct ipv6_mreq mreq = {}; int ttl = 255; int no = 0; - struct sockaddr_in6 sa = { 0 }; + struct sockaddr_in6 sa = {}; int fd = iface->fd.fd; sa.sin6_family = AF_INET6; sa.sin6_port = htons(MCAST_PORT); inet_pton(AF_INET6, MCAST_ADDR6, &sa.sin6_addr); - memset(&mreq, 0, sizeof(mreq)); mreq.ipv6mr_multiaddr = sa.sin6_addr; mreq.ipv6mr_interface = iface->ifindex;
Less verbose Signed-off-by: Rosen Penev <rosenp@gmail.com> --- interface.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-)