Re: [MPTCP] [PATCH RFC 00/10] mptcp: refactor token code

Re: [MPTCP] [PATCH RFC 00/10] mptcp: refactor token code

[Florian Westphal]

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

Peter Krystad <peter.krystad(a)linux.intel.com> wrote:
> I feel somewhat guilty providing a few suggestions regarding how you re-
> factored this code, please see individual PATCH comments.

Why? Your comments are not only helpful, I happen to agree with all
of them - so, thanks a lot, I will incorporate your suggestions asap.

Re: [MPTCP] [PATCH RFC 00/10] mptcp: refactor token code

[Matthieu Baerts]

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

Hi Peter,

On 28/08/2019 02:18, Peter Krystad wrote:
> On Tue, 2019-08-27 at 20:06 +0200, Florian Westphal wrote:
>> Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
>>> For the 'mptcp_' prefix, we made sure to add them to all functions
>>> exposed outside net/mptcp and Peter got the excellent idea to move all
>>> functions only needed in net/mptcp into net/mptcp/protocol.h. We didn't
>>> add 'mptcp_' prefix for all these "internal" functions but it certainly
>>> makes sense to do that. I guess it is useful when debugging if you see
>>> these functions in a call trace or in the list of functions available
>>> for tracing.
>>>
>>> Do you think we should do the same for "crypto" and "subflow" and "pm"?
>>> I guess "pm" and "subflow" are keywords mainly used in "mptcp" but it is
>>> maybe not clear for everybody.
>>
>> Yes.  Subflow is maybe fine though.
>>
>> pm is 'power mangement', so I think it makes sense to prefix mptcp_
>> there.
> 
> Although I argued against this when creating net/mptcp/protocol.h I respect
> the namespace pollution issue more now. Matthieu are you volunteering to do
> this or do you want to have a go? Up to you.

I would be happy to do that but I guess I should focus on helping Mat
with the paper and slides.

If it is not already done by someone else, I can do it later. Maybe
after/at LPC if I really want to force myself to finish the paper/slides
as soon as possible :-)

Cheers,
Matt

> 
> Peter.
> 
>>> The patches look very good to me, just a few questions and one or two
>>> typos, nothing important. I guess other people will also have a look at
>>> them :)
>>
>> Thanks a lot for the review, I will respin a new version later this
>> week.
>> _______________________________________________
>> mptcp mailing list
>> mptcp(a)lists.01.org
>> https://lists.01.org/mailman/listinfo/mptcp
> 

-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

Re: [MPTCP] [PATCH RFC 00/10] mptcp: refactor token code

[Peter Krystad]

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

On Tue, 2019-08-27 at 20:06 +0200, Florian Westphal wrote:
> Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
> > For the 'mptcp_' prefix, we made sure to add them to all functions
> > exposed outside net/mptcp and Peter got the excellent idea to move all
> > functions only needed in net/mptcp into net/mptcp/protocol.h. We didn't
> > add 'mptcp_' prefix for all these "internal" functions but it certainly
> > makes sense to do that. I guess it is useful when debugging if you see
> > these functions in a call trace or in the list of functions available
> > for tracing.
> > 
> > Do you think we should do the same for "crypto" and "subflow" and "pm"?
> > I guess "pm" and "subflow" are keywords mainly used in "mptcp" but it is
> > maybe not clear for everybody.
> 
> Yes.  Subflow is maybe fine though.
> 
> pm is 'power mangement', so I think it makes sense to prefix mptcp_
> there.

Although I argued against this when creating net/mptcp/protocol.h I respect
the namespace pollution issue more now. Matthieu are you volunteering to do
this or do you want to have a go? Up to you.

Peter.

> > The patches look very good to me, just a few questions and one or two
> > typos, nothing important. I guess other people will also have a look at
> > them :)
> 
> Thanks a lot for the review, I will respin a new version later this
> week.
> _______________________________________________
> mptcp mailing list
> mptcp(a)lists.01.org
> https://lists.01.org/mailman/listinfo/mptcp

Re: [MPTCP] [PATCH RFC 00/10] mptcp: refactor token code

[Peter Krystad]

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


Thanks for doing this Florian.

I feel somewhat guilty providing a few suggestions regarding how you re-
factored this code, please see individual PATCH comments.

Peter.

On Sun, 2019-08-25 at 20:59 +0200, Florian Westphal wrote:
> This series is intentionally marked RFC so noone bothers
> to squash all of this into the mptcp tree.
> 
> The idea is to first do a review-friendly series, with
> each change done in an individual patch.
> 
> Then, once consensus is reached (maybe over several iterations),
> I would send a few larger patches suitable for squashing (rather
> than being review friendly...).
> 
> The patches remove unneeded small helpers, add 'mptcp' prefix
> to non-static functions, add error handling to those that may
> fail and adds a few comments.
> 
> Feedback welcome.
> 
>  pm.c       |    6 
>  protocol.c |    5 
>  protocol.h |   27 +--
>  subflow.c  |   76 ++++++++--
>  token.c    |  447 +++++++++++++++++++++++++++----------------------------------
>  5 files changed, 278 insertions(+), 283 deletions(-)
> 
> _______________________________________________
> mptcp mailing list
> mptcp(a)lists.01.org
> https://lists.01.org/mailman/listinfo/mptcp

Re: [MPTCP] [PATCH RFC 00/10] mptcp: refactor token code

[Florian Westphal]

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

Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
> For the 'mptcp_' prefix, we made sure to add them to all functions
> exposed outside net/mptcp and Peter got the excellent idea to move all
> functions only needed in net/mptcp into net/mptcp/protocol.h. We didn't
> add 'mptcp_' prefix for all these "internal" functions but it certainly
> makes sense to do that. I guess it is useful when debugging if you see
> these functions in a call trace or in the list of functions available
> for tracing.
> 
> Do you think we should do the same for "crypto" and "subflow" and "pm"?
> I guess "pm" and "subflow" are keywords mainly used in "mptcp" but it is
> maybe not clear for everybody.

Yes.  Subflow is maybe fine though.

pm is 'power mangement', so I think it makes sense to prefix mptcp_
there.

> The patches look very good to me, just a few questions and one or two
> typos, nothing important. I guess other people will also have a look at
> them :)

Thanks a lot for the review, I will respin a new version later this
week.

Re: [MPTCP] [PATCH RFC 00/10] mptcp: refactor token code

[Matthieu Baerts]

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

Hi Florian,

On 25/08/2019 20:59, Florian Westphal wrote:
> This series is intentionally marked RFC so noone bothers
> to squash all of this into the mptcp tree.
> 
> The idea is to first do a review-friendly series, with
> each change done in an individual patch.
> 
> Then, once consensus is reached (maybe over several iterations),
> I would send a few larger patches suitable for squashing (rather
> than being review friendly...).
> 
> The patches remove unneeded small helpers, add 'mptcp' prefix
> to non-static functions, add error handling to those that may
> fail and adds a few comments.

That's a very good idea!

For the 'mptcp_' prefix, we made sure to add them to all functions
exposed outside net/mptcp and Peter got the excellent idea to move all
functions only needed in net/mptcp into net/mptcp/protocol.h. We didn't
add 'mptcp_' prefix for all these "internal" functions but it certainly
makes sense to do that. I guess it is useful when debugging if you see
these functions in a call trace or in the list of functions available
for tracing.

Do you think we should do the same for "crypto" and "subflow" and "pm"?
I guess "pm" and "subflow" are keywords mainly used in "mptcp" but it is
maybe not clear for everybody.

The patches look very good to me, just a few questions and one or two
typos, nothing important. I guess other people will also have a look at
them :)

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

[MPTCP] [PATCH RFC 00/10] mptcp: refactor token code

[Florian Westphal]

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

This series is intentionally marked RFC so noone bothers
to squash all of this into the mptcp tree.

The idea is to first do a review-friendly series, with
each change done in an individual patch.

Then, once consensus is reached (maybe over several iterations),
I would send a few larger patches suitable for squashing (rather
than being review friendly...).

The patches remove unneeded small helpers, add 'mptcp' prefix
to non-static functions, add error handling to those that may
fail and adds a few comments.

Feedback welcome.

 pm.c       |    6 
 protocol.c |    5 
 protocol.h |   27 +--
 subflow.c  |   76 ++++++++--
 token.c    |  447 +++++++++++++++++++++++++++----------------------------------
 5 files changed, 278 insertions(+), 283 deletions(-)