netdev - Re: [PATCH net-next] docs: try to encourage (netdev?) reviewers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 10 Oct 2023 17:52:23 +0200
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Matthieu Baerts <matttbe@...nel.org>
Cc: Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net, netdev@...r.kernel.org, 
	edumazet@...gle.com, pabeni@...hat.com, corbet@....net, 
	workflows@...r.kernel.org, linux-doc@...r.kernel.org, andrew@...n.ch, 
	jesse.brandeburg@...el.com, sd@...asysnail.net, horms@...ge.net.au, 
	przemyslaw.kitszel@...el.com, f.fainelli@...il.com, jiri@...nulli.us, 
	ecree.xilinx@...il.com
Subject: Re: [PATCH net-next] docs: try to encourage (netdev?) reviewers

Hi Matt,

On Tue, Oct 10, 2023 at 5:19 PM Matthieu Baerts <matttbe@...nel.org> wrote:
> On 10/10/2023 00:56, Jakub Kicinski wrote:
> > Add a section to netdev maintainer doc encouraging reviewers
> > to chime in on the mailing list.
> >
> > The questions about "when is it okay to share feedback"
> > keep coming up (most recently at netconf) and the answer
> > is "pretty much always".
> >
> > Extend the section of 7.AdvancedTopics.rst which deals
> > with reviews a little bit to add stuff we had been recommending
> > locally.
>
> Good idea to encourage everybody to review, even the less experimented
> ones. That might push me to send more reviews, even when I don't know
> well the area that is being modified, thanks! :)
>
> (...)
>
> > diff --git a/Documentation/process/7.AdvancedTopics.rst b/Documentation/process/7.AdvancedTopics.rst
> > index bf7cbfb4caa5..415749feed17 100644
> > --- a/Documentation/process/7.AdvancedTopics.rst
> > +++ b/Documentation/process/7.AdvancedTopics.rst
> > @@ -146,6 +146,7 @@ pull.  The git request-pull command can be helpful in this regard; it will
> >  format the request as other developers expect, and will also check to be
> >  sure that you have remembered to push those changes to the public server.
> >
> > +.. _development_advancedtopics_reviews:
> >
> >  Reviewing patches
> >  -----------------
> > @@ -167,6 +168,12 @@ comments as questions rather than criticisms.  Asking "how does the lock
> >  get released in this path?" will always work better than stating "the
> >  locking here is wrong."
>
> The paragraph just above ("it is OK to question the code") is very nice!
> When I'm cced on some patches modifying some code I'm not familiar with
> and there are some parts that look "strange" to me, I sometimes feel
> like I only have two possibilities: either I spend quite some time
> understanding that part or I give up if I don't have such time. I often
> feel like I cannot say "I don't know well this part, but this looks
> strange to me: are you sure it is OK to do that in such conditions?",
> especially when the audience is large and/or the author of the patch is
> an experienced developer.

Yes you can (even experienced developers can make mistakes ;-)!

If it is not obvious that something is safe, it is better to point it
out, so the submitter (or someone else) can give it a (second) thought.
In case it is safe, and you didn't miss the ball completely, it probably
warrants a comment in the code, or an improved patch description.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds