linux-kernel - Re: [PATCH] Documentation/patch-tags v3

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]

Message-Id: <1192139467.7899.55.camel@heimdal.trondhjem.org>
Date:	Thu, 11 Oct 2007 17:51:07 -0400
From:	Trond Myklebust <trond.myklebust@....uio.no>
To:	Stefan Richter <stefanr@...6.in-berlin.de>
Cc:	Jonathan Corbet <corbet@....net>, linux-kernel@...r.kernel.org,
	akpm@...ux-foundation.org
Subject: Re: [PATCH] Documentation/patch-tags v3


On Thu, 2007-10-11 at 23:21 +0200, Stefan Richter wrote:
> Trond Myklebust wrote:
> > Does 'Reviewed-by' also imply 'Signed-off-by'?
> 
> Does a technical review include a review of licensing and copyright
> issues?  (It doesn't seem to be a big issue though if the submitter
> signed off on it, like he should.)
> 
> > In other words, who is actually supposed to add this tag?
> > 
> > Is it the reviewer who passes on an officially 'reviewed' patch to the
> > maintainer, or is it the patch author him/herself who is responsible for
> > soliciting reviews and adding the tag?
> 
> Anybody in the patch forwarding chain (author, maintainers... usually
> the latter) can add Acked-by and Tested-by, based on incoming feedback.
> The feedback may have explicitly stated an Acked-by or Tested-by or may
> have said something equivalent.  (In case of Tested-by, an appropriate
> description of how was tested should have been sent.  An explicit
> Tested-by from the tester himself is moot then.)
> 
> Reviewed-by is a different beast.  If Jon's definition of Reviewed-by
> (or another definition) is "officially" adopted, people in the patch
> forwarding chain should only add this tag if the reviewer sent it
> explicitly in his response.  Unlike with Acked-by and Tested-by, we must
> not guess whether a reviewer wants to have his Reviewed-by added.

In that case the reviewer should be made part of the forwarding chain,
and it should be made clear to whoever is upstream that this is a patch
that has not been modified since it was reviewed.

> [...]
> >> + (c) While there may be things that could be improved with this submission,
> >> +     I believe that it is, at this time, (1) a worthwhile modification to
> >> +     the kernel, and (2) free of known issues which would argue against its
> >> +     inclusion.
> >> +
> >> + (d) While I have reviewed the patch and believe it to be sound, I do not
> >> +     (unless explicitly stated elsewhere) make any warranties or guarantees
> >> +     that it will achieve its stated purpose or function properly in any
> >> +     given situation.
> > 
> > I'm confused about how to reconcile (c) and (d) here. If you are not
> > sure about whether or not the patch will achieve its stated purpose, why
> > would you be arguing that it is a worthwhile modification?
> 
> Being sure of something and making guarantees are different things.

To a lawyer, yes. To everyone else, no, and the GPL already tells you
that you are given no warranties.

The reviewed-by tag has, as far as I understand, no legal standing:
unlike the DCE, we're not expecting anyone to be able to sue over this.
So we should at least be trying to ensure that reviewers are 'reasonably
sure' that the patch works. Otherwise, reviews will turn into yet
another coding standards witch hunt of zero value.

Trond

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/