[lwip-devel] [patch #6537] wnd_scale TCP option addition

[Jonathan Larmour]

Update of patch #6537 (project lwip):

                  Status:                    None => Need Info              
             Assigned to:                    None => jifl                   

    _______________________________________________________

Follow-up Comment #1:

I think this patch looks good, although would suggest a few tweaks, and I
have 2 questions.

First the questions: 
1) why are the various window sizes in tcp.h changed to u32_t - I thought
they were still limited to 65536?

2) why is snd_queuelen increased to u32_t - I can't foresee having more than
65536 segs for a single PCB!

As for some suggested tweaks, rather than trying to bolt-on optdata2, and do
a memcpy, I think we should just rationalise the option addition into a single
new function in tcp.c (which tcp_in.c could call), which allows for some
future additions.

Something like
#if TCP_RCVSCALE
typedef u32_t tcp_option_data[2];
#else
typedef u32_t tcp_option_data;
#endif

void tcp_build_options( struct tcp_pcb *pcb, tcp_option_data *optdata, u8_t
*optlen );

And then the tcp_enqueue stuff becomes something like:
tcp_option_data optdata;
u16_t optlen;
tcp_build_options(npcb, &optdata, &optdatasize)'
tcp_enqueue(npcb, NULL, 0, TCP_SYN | TCP_ACK, 0, (u8_t *)&optdata, optlen);

To make the code more readable, it may be nice to have and use the following
macros in tcp.h:
#if TCP_RCVSCALE
#define RCV_WND_SCALE(pcb, wnd) ((wnd) >> (pcb)->rcv_scale)
#define SND_WND_SCALE(pcb, wnd) ((wnd) << (pcb)->snd_scale)
#else
#define RCV_WND_SCALE(pcb, wnd) (wnd)
#define SND_WND_SCALE(pcb, wnd) (wnd)
#endif

So that elsewhere we could have just something like e.g.
seg->tcphdr->wnd = RCV_WND_SCALE(pcb, htons(pcb->rcv_ann_wnd));

Would you be able to look at doing it that way? I hope it should be an easy
change and I think makes the patch a bit nicer.

Thanks for contributing back!

Jifl



    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?6537>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/