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

From: Samuel Thibault <samuel.thibault@gnu.org>
To: Thomas Huth <thuth@redhat.com>
Cc: zhanghailiang <zhang.zhanghailiang@huawei.com>,
	Li Zhijian <lizhijian@cn.fujitsu.com>,
	Stefan Hajnoczi <stefanha@gmail.com>,
	Jason Wang <jasowang@redhat.com>,
	Dave Gilbert <dgilbert@redhat.com>,
	Vasiliy Tolstov <v.tolstov@selfip.ru>,
	qemu-devel@nongnu.org, Gonglei <arei.gonglei@huawei.com>,
	Jan Kiszka <jan.kiszka@siemens.com>,
	Huangpeng <peter.huangpeng@huawei.com>,
	Yang Hongyang <yanghy@cn.fujitsu.com>,
	Guillaume Subiron <maethor@subiron.org>
Subject: Re: [Qemu-devel] [PATCHv7 1/9] slirp: Adding IPv6, ICMPv6 Echo and NDP autoconfiguration
Date: Tue, 9 Feb 2016 17:31:19 +0100	[thread overview]
Message-ID: <20160209163119.GB3102@var.bordeaux.inria.fr> (raw)
In-Reply-To: <56BA1057.6030409@redhat.com>

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