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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3361 bytes --]

On 02/09/2016 09:14 AM, Thomas Huth wrote:
> On 08.02.2016 11:28, Samuel Thibault wrote:
>> From: Guillaume Subiron <maethor@subiron.org>
>>
>> This patch adds the functions needed to handle IPv6 packets. ICMPv6 and
>> NDP headers are implemented.
>>

>> +static inline int in6_equal_net(struct in6_addr a, struct in6_addr b,
>> +        int prefix_len)
>> +{
>> +    if (memcmp(&a, &b, prefix_len / 8) != 0) {
>> +        return 0;
>> +    }
>> +
>> +    if (prefix_len % 8 == 0) {
>> +        return 1;
>> +    }
>> +
>> +    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

'>>' binds higher than '==', so you could write:

return a.s6... % 8))
    == b.s6... % 8));

Make this function return bool, while you are at it.

> 
> There are also some other stylistic problems that checkpatch.pl reports
> in this file ... would be nice to fix them.
> 
>> +}
>> +
>> +static inline int in6_equal_mach(struct in6_addr a, struct in6_addr b,
>> +        int prefix_len)

Another candidate for returning bool.


>> +static inline void in6_compute_ethaddr(struct in6_addr ip,
>> +                                       uint8_t eth[ETH_ALEN])
>> +{
>> +    eth[0] = 0x52;
>> +    eth[1] = 0x56;
>> +    memcpy(&eth[2], &(ip.s6_addr[16-(ETH_ALEN-2)]), ETH_ALEN-2);
> 
> checkpatch.pl complains about the style here ... and I think you could
> also drop the parentheses around "ip.s6_addr[16-(ETH_ALEN-2)]".

And remember spaces around both binary '-'

>> +++ b/slirp/ip6_icmp.c
>> @@ -0,0 +1,350 @@
>> +/*
>> + * Copyright (c) 2013

Want to add 2016?

>> + * Guillaume Subiron, Yann Bordenave, Serigne Modou Wagne.
>> + *
>> + * Please read the file COPYRIGHT for the
>> + * terms and conditions of the copyright.

We don't have a file named 'COPYRIGHT' in the tree.  By default you are
getting GPLv2+; be explicit if you meant something else.

>> + */
>> +
>> +#include "slirp.h"
>> +#include "ip6_icmp.h"
>> +#include "qemu/timer.h"
>> +#include "qemu/error-report.h"
>> +#include <stdlib.h>
>> +#include <time.h>

New .c files need to include "qemu/osdep.h" first; at which point
<stdlib.h> is pre-included.

>> +
>> +#define rand_a_b(a, b)\
>> +    (rand()%(int)(b-a)+a)

Spacing around binary operators.  Should we rely on glib for nicer
random interval functions?


>> +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.

It's not portable to C89, but QEMU requires C99 where it is completely
portable.  However, being portable and being commonly used in the rest
of the source tree are two different things.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]