Re: [Qemu-devel] Contribution - L2TPv3 transport

[Paolo Bonzini <pbonzini@redhat.com>]

Il 28/02/2014 12:17, Anton Ivanov (antivano) ha scritto:
>> > As mentioned below, I suggest storing the cookies and session ids in
>> > host order in NetL2TPV3State, and doing the conversion in
>> > l2tpv3_form_header and friends.
> I can fix it. I prefer to keep all params in "ready to use" form so that
> no cycles are wasted on conversion in the portions which may affect
> performance.
>

This is just one instruction (bswap) or zero on some hardware (PPC with 
has lwbrz, Haswell which has movbe), no reason to worry about it.  It 
makes the code simpler by making all accesses use stX_be_p.

>> Space before the opening brace, and parentheses around !(a & b) are
>> unnecessary.  More instances in the rest of the file.
>
> Bad habits die hard. After being burned by a couple of buggy borland
> compilers 20 years ago I brace everything to the hilt. You have a point
> though.

We also brace everything, but we do not parenthesize everything. :)

>> Why do you need separate mallocs for these?
>
> You do not really need to use a separate malloc for TX and RX. You can
> reuse the first element of the RX vector for TX.
>
> Fair point.

No, I mean: why not just use arrays in NetL2TPV3State?  All of them are 
sized statically.  Avoiding pointer chasing also improves performance. 
:)  It also avoids memory leaks; I just noticed that you're not freeing 
the memory you allocate in net_l2tpv3_init (I checked s->header-buf).

>> Is the local address mandatory?
>
> In L2TPv3 - yes.

Ok.

> In fact so is the remote - our "listen mode" is a hack.

The listen mode is not implemented in this patch, is it?

Thanks for the prompt reply.

Note that I posted two small fixes to qemu-sockets.c.  You may want to 
include them.

Paolo