Re: [Qemu-devel] [PATCH 25/41] slirp: Make IP packet ID consistent

[Filip Navara <filip.navara@gmail.com>]

On Wed, Jun 24, 2009 at 4:48 PM, Jan Kiszka<jan.kiszka@siemens.com> wrote:
> Filip Navara wrote:
>> On Wed, Jun 24, 2009 at 2:42 PM, Jan Kiszka<jan.kiszka@siemens.com> wrote:
>>> Currently, ip_id is always initialized to 0 on slirp startup (despite
>>> the broken attempt to derive it from the clock). This is good for
>>> reproducibility. But it is not preserved across save/restore. This patch
>>> therefore drops the dead initialization code from ip_init and introduces
>>> ip_id to the persistent slirp state.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>>
>>>  slirp/ip_input.c |    1 -
>>>  slirp/slirp.c    |    8 +++++++-
>>>  2 files changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/slirp/ip_input.c b/slirp/ip_input.c
>>> index 0356eb5..b07d3d5 100644
>>> --- a/slirp/ip_input.c
>>> +++ b/slirp/ip_input.c
>>> @@ -59,7 +59,6 @@ void
>>>  ip_init(void)
>>>  {
>>>        ipq.ip_link.next = ipq.ip_link.prev = &ipq.ip_link;
>>> -       ip_id = tt.tv_sec & 0xffff;
>>
>> You removed the ip_id initialization and now it's never initialized in
>> the code. That sounds wrong.
>
> Thanks for having a look! All this slirp code is really hairy and no fun
> to dig through.

Totally agreed. At one point I was considering to replace it with
lwip, but I don't have the time necessary for that and it would be
hard to keep all the existing services (TFTP, DHCP, SMB) working.

> But now to your remark: That removal is in fact not changing the
> behavior. ip_id is also initialized to 0 afterwards. To understand this
> one has to track the messy use of 'tt' across slirp. It is in fact
> carrying the time at some point (when select_fill is invoked), but not
> yet on ip_init.

Ah, I didn't see that, makes sense. It actually explains what I saw
last week in the packet dump.

> I was thinking about fixing the above initialization to actually take
> the current time and derive ip_id, but then I thought it might be better
> to keep this for network traffic reproducibility across qemu starts.

Agreed.

Best regards,
Filip Navara