Re: [Qemu-devel] [PATCH 1/5] eth: Extend vlan stripping functions

[Dmitry Fleytman]

> On 3 Mar 2017, at 18:52 PM, Philippe Mathieu-Daudé <address@hidden> wrote:
> 
> On 02/16/2017 09:29 AM, Dmitry Fleytman wrote:
>> Make VLAN stripping functions return number of bytes
>> copied to given Ethernet header buffer.
>> 
>> This information should be used to re-compose
>> packet IOV after VLAN stripping.
>> 
>> Signed-off-by: Dmitry Fleytman <address@hidden>
>> ---
>> include/net/eth.h |  4 ++--
>> net/eth.c         | 25 ++++++++++++++-----------
>> 2 files changed, 16 insertions(+), 13 deletions(-)
>> 
>> diff --git a/include/net/eth.h b/include/net/eth.h
>> index 2013175..afeb45b 100644
>> --- a/include/net/eth.h
>> +++ b/include/net/eth.h
>> @@ -331,12 +331,12 @@ eth_get_pkt_tci(const void *p)
>>     }
>> }
>> 
>> -bool
>> +size_t
>> eth_strip_vlan(const struct iovec *iov, int iovcnt, size_t iovoff,
>>                uint8_t *new_ehdr_buf,
>>                uint16_t *payload_offset, uint16_t *tci);
>> 
>> -bool
>> +size_t
>> eth_strip_vlan_ex(const struct iovec *iov, int iovcnt, size_t iovoff,
>>                   uint16_t vet, uint8_t *new_ehdr_buf,
>>                   uint16_t *payload_offset, uint16_t *tci);
>> diff --git a/net/eth.c b/net/eth.c
>> index df81efb..5b9ba26 100644
>> --- a/net/eth.c
>> +++ b/net/eth.c
>> @@ -232,7 +232,7 @@ void eth_get_protocols(const struct iovec *iov, int 
>> iovcnt,
>>     }
>> }
>> 
>> -bool
>> +size_t
>> eth_strip_vlan(const struct iovec *iov, int iovcnt, size_t iovoff,
>>                uint8_t *new_ehdr_buf,
>>                uint16_t *payload_offset, uint16_t *tci)
>> @@ -244,7 +244,7 @@ eth_strip_vlan(const struct iovec *iov, int iovcnt, 
>> size_t iovoff,
>>                                new_ehdr, sizeof(*new_ehdr));
>> 
>>     if (copied < sizeof(*new_ehdr)) {
>> -        return false;
>> +        return 0;
>>     }
>> 
>>     switch (be16_to_cpu(new_ehdr->h_proto)) {
>> @@ -254,7 +254,7 @@ eth_strip_vlan(const struct iovec *iov, int iovcnt, 
>> size_t iovoff,
>>                             &vlan_hdr, sizeof(vlan_hdr));
>> 
>>         if (copied < sizeof(vlan_hdr)) {
>> -            return false;
>> +            return 0;
>>         }
>> 
>>         new_ehdr->h_proto = vlan_hdr.h_proto;
>> @@ -268,18 +268,21 @@ eth_strip_vlan(const struct iovec *iov, int iovcnt, 
>> size_t iovoff,
>>                                 PKT_GET_VLAN_HDR(new_ehdr), 
>> sizeof(vlan_hdr));
>> 
>>             if (copied < sizeof(vlan_hdr)) {
>> -                return false;
>> +                return 0;
>>             }
>> 
>>             *payload_offset += sizeof(vlan_hdr);
>> +
>> +            return sizeof(struct eth_header) + sizeof(struct vlan_header);
>> +        } else {
>> +            return sizeof(struct eth_header);
> 
> remove this "else return ..."
> 
>>         }
>> -        return true;
> 
> here avoid fall-through with 'return sizeof(struct eth_header);’


Hi Philippe,

IIUYC there is no functional difference, just a matter of style. Am I right?

As for me current code is more explicit so I would leave it as is unless
there is a specific project coding style requirement.

~Dmitry

> 
>>     default:
>> -        return false;
>> +        return 0;
>>     }
>> }
>> 
>> -bool
>> +size_t
>> eth_strip_vlan_ex(const struct iovec *iov, int iovcnt, size_t iovoff,
>>                   uint16_t vet, uint8_t *new_ehdr_buf,
>>                   uint16_t *payload_offset, uint16_t *tci)
>> @@ -291,7 +294,7 @@ eth_strip_vlan_ex(const struct iovec *iov, int iovcnt, 
>> size_t iovoff,
>>                                new_ehdr, sizeof(*new_ehdr));
>> 
>>     if (copied < sizeof(*new_ehdr)) {
>> -        return false;
>> +        return 0;
>>     }
>> 
>>     if (be16_to_cpu(new_ehdr->h_proto) == vet) {
>> @@ -299,17 +302,17 @@ eth_strip_vlan_ex(const struct iovec *iov, int iovcnt, 
>> size_t iovoff,
>>                             &vlan_hdr, sizeof(vlan_hdr));
>> 
>>         if (copied < sizeof(vlan_hdr)) {
>> -            return false;
>> +            return 0;
>>         }
>> 
>>         new_ehdr->h_proto = vlan_hdr.h_proto;
>> 
>>         *tci = be16_to_cpu(vlan_hdr.h_tci);
>>         *payload_offset = iovoff + sizeof(*new_ehdr) + sizeof(vlan_hdr);
>> -        return true;
>> +        return sizeof(struct eth_header);
>>     }
>> 
>> -    return false;
>> +    return 0;
>> }
>> 
>> void
>> 
> 
> with changes:
> Reviewed-by: Philippe Mathieu-Daudé <address@hidden <mailto:address@hidden>>
> 
> Phil.