Re: [MPTCP] patch set: checkpatch.pl warnings/errors

Re: [MPTCP] patch set: checkpatch.pl warnings/errors

[Florian Westphal]

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

Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
> - line over 80 characters: do we have to be strict? Some lines are easy
> to cut, I can fix that. But what about some lines like:
> 
>     if (!((opt_rx->mptcp.flags & MPTCP_CAP_FLAG_MASK) ==
> MPTCP_CAP_HMAC_SHA1) ||

I would keep this as-is.

>     static inline struct socket *mptcp_subflow_tcp_socket(const struct
> subflow_context *subflow)

Can be done like this:

static inline struct socket *
mptcp_subflow_tcp_socket(const struct subflow_context *subflow)

> - Unnecessary parentheses around 'X': can we ignore them if the author
> kept them to improve readability / reduce ambiguity?
>
> - Comparison to NULL could be written "X": same here: ignore or fix?
> Maybe these ones make more sense for the maintainers to fix?

Both are in the eye of the beholder.
I tend to "fix" both, just to avoid the followup patches that will
be coming otherwise (just look for "checkpatch fixes" commits
in the kernel ...)

> - kselftests: do we have to keep the same style as the rest of the code?
> There are quite a few warnings there. Do we have to be strict? Maybe
> better to fix the most obvious ones not to distract reviewers with these
> details? I also guess we can accept longer lines in the bash scripts.

I've never bothered with checkpatch errors in the scripts.

> - what about the MAINTAINERS file? Can we delay this modification for later?

Yes, but I think it would make sense to add an MPTCP entry anyway, at
least once v1 for merging is sent.

There is a "get_maintainer" script that uses git history + the
MAINTAINERS file to suggest a list of persons and/or mailing lists
to CC to a patch.

> - header of the new files: what do we want to do with them? Do we need
> to update the info (description, copyright, etc.)? checkpatch complains
> about: "networking block comments don't use an empty /* line, use /*
> Comment...". Because we have:
> 
>     +/*
>     + * Multipath TCP

Hrhrhrhr...

I would change it to /* Multipath ...

I prefer the current style (which is what almost the entire kernel is
using...), but same as the 'Unnecessary parentheses' and 'Comparison to
NULL', better change it in the initial merge submission so we avoid the
unneeded 3rd party 'fix patches'.

> - not related to checkpatch but could we squash "tcp: Check for filled
> TCP option space before SACK" in "mptcp: Handle MP_CAPABLE options for
> outgoing" or move it just after the patch adding this MP_CAPABLE support?

I would keep it separate to ease review, and i think its in the right
place too.

Re: [MPTCP] patch set: checkpatch.pl warnings/errors

[Florian Westphal]

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

Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
> I will then try to do the appropriated changes next week by following
> your recommendations!
> I am sure Eric would not mind reading the current patches, there are not
>
> so many warnings/errors but certainly better to show him that we do care
> about checkpatch and maybe better if he is not distracted by these
> warnings/errors.

I'm sure he doesn't mind, but it still might be worth do "fix" it up
anyway.

So, thanks for doing this!

Re: [MPTCP] patch set: checkpatch.pl warnings/errors

[Matthieu Baerts]

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

Hi Florian,

Thank you for your useful feedback!

I will then try to do the appropriated changes next week by following
your recommendations!
I am sure Eric would not mind reading the current patches, there are not
so many warnings/errors but certainly better to show him that we do care
about checkpatch and maybe better if he is not distracted by these
warnings/errors.

On 08/06/2019 11:01, Florian Westphal wrote:
> Matthieu Baerts <matthieu.baerts(a)tessares.net> wrote:
>> - line over 80 characters: do we have to be strict? Some lines are easy
>> to cut, I can fix that. But what about some lines like:
>>
>>     if (!((opt_rx->mptcp.flags & MPTCP_CAP_FLAG_MASK) ==
>> MPTCP_CAP_HMAC_SHA1) ||
> 
> I would keep this as-is.
> 
>>     static inline struct socket *mptcp_subflow_tcp_socket(const struct
>> subflow_context *subflow)
> 
> Can be done like this:
> 
> static inline struct socket *
> mptcp_subflow_tcp_socket(const struct subflow_context *subflow)

I will do that, thank you!

>> - Unnecessary parentheses around 'X': can we ignore them if the author
>> kept them to improve readability / reduce ambiguity?
>>
>> - Comparison to NULL could be written "X": same here: ignore or fix?
>> Maybe these ones make more sense for the maintainers to fix?
> 
> Both are in the eye of the beholder.
> I tend to "fix" both, just to avoid the followup patches that will
> be coming otherwise (just look for "checkpatch fixes" commits
> in the kernel ...)

Oh OK, I didn't know that. It makes sense to follow checkpatch then!

>> - kselftests: do we have to keep the same style as the rest of the code?
>> There are quite a few warnings there. Do we have to be strict? Maybe
>> better to fix the most obvious ones not to distract reviewers with these
>> details? I also guess we can accept longer lines in the bash scripts.
> 
> I've never bothered with checkpatch errors in the scripts.

OK!

>> - what about the MAINTAINERS file? Can we delay this modification for later?
> 
> Yes, but I think it would make sense to add an MPTCP entry anyway, at
> least once v1 for merging is sent.
> 
> There is a "get_maintainer" script that uses git history + the
> MAINTAINERS file to suggest a list of persons and/or mailing lists
> to CC to a patch.

OK I see, thank you! So we can do that later and take the decision
together about who to put in this list!

>> - header of the new files: what do we want to do with them? Do we need
>> to update the info (description, copyright, etc.)? checkpatch complains
>> about: "networking block comments don't use an empty /* line, use /*
>> Comment...". Because we have:
>>
>>     +/*
>>     + * Multipath TCP
> 
> Hrhrhrhr...
> 
> I would change it to /* Multipath ...
> 
> I prefer the current style (which is what almost the entire kernel is
> using...), but same as the 'Unnecessary parentheses' and 'Comparison to
> NULL', better change it in the initial merge submission so we avoid the
> unneeded 3rd party 'fix patches'.

Make sense! Thank you!

About the info to put in it (description, copyright, etc.), do we have
to do something?

We can do that later if needed of course!

>> - not related to checkpatch but could we squash "tcp: Check for filled
>> TCP option space before SACK" in "mptcp: Handle MP_CAPABLE options for
>> outgoing" or move it just after the patch adding this MP_CAPABLE support?
> 
> I would keep it separate to ease review, and i think its in the right
> place too.

Fine for me!

Again thank you for your feedback!

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 set: checkpatch.pl warnings/errors

[Matthieu Baerts]

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

Hello,

Yesterday I was quickly looking at the checkpatch warnings/errors as we
said that we should look at them before any submission.

Some are easy to fix and have to be fixed at some points, e.g. mix of
tabs/spaces, where the '*' is set, 'const' missing, etc. I already
started to fix some of them.

Some warnings can be ignored, e.g. we add an entry in an "old" file like
in.h and we keep the same style as the previous lines.

But for the rest, I have a few questions:

- line over 80 characters: do we have to be strict? Some lines are easy
to cut, I can fix that. But what about some lines like:

    if (!((opt_rx->mptcp.flags & MPTCP_CAP_FLAG_MASK) ==
MPTCP_CAP_HMAC_SHA1) ||

    static inline struct socket *mptcp_subflow_tcp_socket(const struct
subflow_context *subflow)

- Unnecessary parentheses around 'X': can we ignore them if the author
kept them to improve readability / reduce ambiguity?

- Comparison to NULL could be written "X": same here: ignore or fix?
Maybe these ones make more sense for the maintainers to fix?

- kselftests: do we have to keep the same style as the rest of the code?
There are quite a few warnings there. Do we have to be strict? Maybe
better to fix the most obvious ones not to distract reviewers with these
details? I also guess we can accept longer lines in the bash scripts.

- what about the MAINTAINERS file? Can we delay this modification for later?

- header of the new files: what do we want to do with them? Do we need
to update the info (description, copyright, etc.)? checkpatch complains
about: "networking block comments don't use an empty /* line, use /*
Comment...". Because we have:

    +/*
    + * Multipath TCP

- not related to checkpatch but could we squash "tcp: Check for filled
TCP option space before SACK" in "mptcp: Handle MP_CAPABLE options for
outgoing" or move it just after the patch adding this MP_CAPABLE support?


I don't know what the net-next maintainers/devs find important? I know
that the "reversed Xmas tree variable declaration" is important but not
sure about the others. Any feedback welcome :-)

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