Re: [Qemu-devel] [PATCHv7 1/9] slirp: Adding IPv6, ICMPv6 Echo and NDP autoconfiguration

[Samuel Thibault]

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