Thomas Huth, on Tue 09 Feb 2016 17:14:15 +0100, wrote:
> > + return (a.s6_addr[prefix_len / 8] >> (8 - (prefix_len % 8)))
> > + == (b.s6_addr[prefix_len / 8] >> (8 - (prefix_len % 8)));
>
> checkpatch.pl complains here:
>
> ERROR: return is not a function, parentheses are not required
This is a false positive, there are no outer parentheses here.
> There are also some other stylistic problems that checkpatch.pl reports
> in this file ... would be nice to fix them.
Uh, it seems they are new, I had made a checkpatch run at the time (but
that was like several months ago now...). So I'll have to go through it
again...
> > +static void icmp6_send_echoreply(struct mbuf *m, Slirp *slirp, struct ip6
> > *ip,
> > + struct icmp6 *icmp)
> > +{
> > + struct mbuf *t = m_get(slirp);
> > + t->m_len = sizeof(struct ip6) + ntohs(ip->ip_pl);
> > + memcpy(t->m_data, m->m_data, t->m_len);
> > +
> > + /* IPv6 Packet */
> > + struct ip6 *rip = mtod(t, struct ip6 *);
>
> Not sure how strictly this is handled in QEMU, but for proper portable
> C, variables should be declared at the beginning of a scope, as far as I
> know.
AIUI, qemu requires C99, doesn't it?
Personnally I find it more readable to declare variables where they
start mattering instead of far away.
> > + m->m_len += ETH_HLEN;
> > + m->m_data -= ETH_HLEN;
> > + struct ethhdr *eth = mtod(m, struct ethhdr *);
> > + m->m_len -= ETH_HLEN;
> > + m->m_data += ETH_HLEN;
>
> Manipulating m_len is not really required here, is it?
Indeed, I however preferred to keep it for always keeping consistency,
in case mtod starts doing other stuff.
> > + /* ICMPv6 Checksum */
> > + t->m_data -= NDPOPT_LINKLAYER_LEN;
> > + t->m_data -= ICMP6_NDP_RA_MINLEN;
> > + t->m_data -= sizeof(struct ip6);
>
> I think you could shorten this to:
>
> t_m_data = rip;
That's equivalent, yes. We are here however following the current slirp
practice, which explicits what is happening here.
I'll also have a closer look at the rest, thanks,
Samuel
