[MPTCP] Re: [MPTCP][PATCH v2 mptcp-next 02/10] mptcp: unify ADD_ADDR and ADD_ADDR6 suboptions writing

* [MPTCP] Re: [MPTCP][PATCH v2 mptcp-next 02/10] mptcp: unify ADD_ADDR and ADD_ADDR6 suboptions writing
@ 2020-11-05 13:55 Matthieu Baerts
  0 siblings, 0 replies; 3+ messages in thread
From: Matthieu Baerts @ 2020-11-05 13:55 UTC (permalink / raw)
  To: mptcp

[-- Attachment #1: Type: text/plain, Size: 4073 bytes --]

Hi Geliang,

On 04/11/2020 08:04, Geliang Tang wrote:
> Hi Matt,
> 
> Matthieu Baerts <matthieu.baerts(a)tessares.net 
> <mailto:matthieu.baerts(a)tessares.net>> 于2020年11月4日周三 上午1:23写道：
> 
>     Hi Geliang,
> 
>     On 31/10/2020 16:24, Geliang Tang wrote:
>      > The length of ADD_ADDR6 is 12 octets longer than ADD_ADDR. That's the
>      > only difference between them.
>      >
>      > This patch dropped the duplicate code between ADD_ADDR and ADD_ADDR6
>      > suboptions writing, and unify them into one.
> 
>     It is nice to avoid similar code but here, I think you are changing the
>     behaviour if CONFIG_MPTCP_IPV6 is disabled. If I am not mistaken, we
>     are
>     going to reserve space but not write anything in it except the hmac.
> 
>     For the moment, it is fine because we will not call mptcp_write_options
>     with OPTION_MPTCP_ADD_ADDR6 but I don't know if we can safely take that
>     into consideration for a "public" function called from TCP.
> 
>     Maybe "safer" by using a helper taking different arguments for v4 and
>     v6? In this case we could have something like:
> 
> 
>              if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
>                      mptcp_write_add_addr_option(ptr, opts,
>                                               
>       TCPOLEN_MPTCP_ADD_ADDR_BASE);
>              }
> 
>     #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>              if (OPTION_MPTCP_ADD_ADDR6 & opts->suboptions) {
>                      mptcp_write_add_addr_option(ptr, opts,
>                                               
>     TCPOLEN_MPTCP_ADD_ADDR6_BASE);
>              }
>     #endif
> 
>     Or we add a comment mentioning we should not have
>     OPTION_MPTCP_ADD_ADDR6
>     if IPV6 is disabled?
> 
> Thanks for your review. Should it be better to put all 
> OPTION_MPTCP_ADD_ADDR6
> into the IS_ENABLED(CONFIG_MPTCP_IPV6) condition? Something like:
> 
>          if ((OPTION_MPTCP_ADD_ADDR
> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>             | OPTION_MPTCP_ADD_ADDR6
> #endif
>              ) & opts->suboptions) {
>                  u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
>                  u8 echo = MPTCP_ADDR_ECHO;
> 
> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>                  if (OPTION_MPTCP_ADD_ADDR6 & opts->suboptions)
>                          len = TCPOLEN_MPTCP_ADD_ADDR6_BASE;
> #endif
> 
>                  if (opts->ahmac) {
>                          len += sizeof(opts->ahmac);
>                          echo = 0;
>                  }
> 
>                  *ptr++ = mptcp_option(MPTCPOPT_ADD_ADDR,
>                                        len, echo, opts->addr_id);
>                  if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
>                          memcpy((u8 *)ptr, (u8 *)&opts->addr.s_addr, 4);
>                          ptr += 1;
>                  }
> #if IS_ENABLED(CONFIG_MPTCP_IPV6)
>                  else if (OPTION_MPTCP_ADD_ADDR6 & opts->suboptions) {
>                          memcpy((u8 *)ptr, opts->addr6.s6_addr, 16);
>                          ptr += 4;
>                  }
> #endif
>                  if (opts->ahmac) {
>                          put_unaligned_be64(opts->ahmac, ptr);
>                          ptr += 2;
>                  }
>          }
> 
> Is this code acceptable?

I don't know if it is clearer but that's typical with v6 :)
(we could even drop the second "#if IS_ENABLED(CONFIG_MPTCP_IPV6)" if we 
want)

But it looks "safer" :)

Cheers,
Matt

PS: the HTML rendering makes the code a bit hard to read. May you turn 
on the option to switch to "plain text mode" only if you don't mind :)
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

^ permalink raw reply	[flat|nested] 3+ messages in thread