Re: [Qemu-devel] Contribution - L2TPv3 transport

[Anton Ivanov (antivano)]

On 28/02/14 11:36, Paolo Bonzini wrote:
> 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.

OK.

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

Correct, I have not updated the cleanup procedures to deal with freeing
all the vector bits, it will be in the updated version.

>
>>> 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?

Not yet. I had to remove it to add recvmmsg, I was going to add it.
However it is not mandatory by any means. I am happy for this to be
without it.


>
> Thanks for the prompt reply.
>
> Note that I posted two small fixes to qemu-sockets.c.  You may want to
> include them.

OK.

>
> Paolo