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
