[PATCH] ext4: Variable to signed to check return code

[PATCH] ext4: Variable to signed to check return code

[Philippe Mazenauer]

Variables 'n' and 'err' are both used for less-than-zero error checking,
however both are declared as unsigned. Ensure ext4_map_blocks() and
add_system_zone() are able to have their return values propagated
correctly by redefining them both as signed integers.

../fs/ext4/block_validity.c:158:9: warning: comparison of unsigned
expression < 0 is always false [-Wtype-limits]
    if (n < 0) {
        ^

../fs/ext4/block_validity.c:173:12: warning: comparison of unsigned
expression < 0 is always false [-Wtype-limits]
    if (err < 0)
        ^

Signed-off-by: Philippe Mazenauer <philippe.mazenauer@outlook.de>
---
 fs/ext4/block_validity.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/block_validity.c b/fs/ext4/block_validity.c
index 968f163b5feb..678e99aeef1f 100644
--- a/fs/ext4/block_validity.c
+++ b/fs/ext4/block_validity.c
@@ -142,7 +142,8 @@ static int ext4_protect_reserved_inode(struct super_block *sb, u32 ino)
 	struct inode *inode;
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 	struct ext4_map_blocks map;
-	u32 i = 0, err = 0, num, n;
+	int err = 0, n;
+	u32 i = 0, num;
 
 	if ((ino < EXT4_ROOT_INO) ||
 	    (ino > le32_to_cpu(sbi->s_es->s_inodes_count)))
-- 
2.17.1

Re: [PATCH] ext4: Variable to signed to check return code

[Lee Jones]

On Fri, 17 May 2019, Philippe Mazenauer wrote:

> Variables 'n' and 'err' are both used for less-than-zero error checking,
> however both are declared as unsigned. Ensure ext4_map_blocks() and
> add_system_zone() are able to have their return values propagated
> correctly by redefining them both as signed integers.
> 
> ../fs/ext4/block_validity.c:158:9: warning: comparison of unsigned
> expression < 0 is always false [-Wtype-limits]
>     if (n < 0) {
>         ^
> 
> ../fs/ext4/block_validity.c:173:12: warning: comparison of unsigned
> expression < 0 is always false [-Wtype-limits]
>     if (err < 0)
>         ^
> 
> Signed-off-by: Philippe Mazenauer <philippe.mazenauer@outlook.de>
> ---
>  fs/ext4/block_validity.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] ext4: Variable to signed to check return code

[Theodore Ts'o]

On Fri, May 17, 2019 at 11:25:06AM +0100, Lee Jones wrote:
> On Fri, 17 May 2019, Philippe Mazenauer wrote:
> 
> > Variables 'n' and 'err' are both used for less-than-zero error checking,
> > however both are declared as unsigned. Ensure ext4_map_blocks() and
> > add_system_zone() are able to have their return values propagated
> > correctly by redefining them both as signed integers.

This is already fixed in the ext4.git tree; it will be pushed to Linus
shortly.  (Thanks to Colin Ian King from Canonical for sending the
patch.)

> Acked-by: Lee Jones <lee.jones@linaro.org>

Lee, techncially this should have been Reviewed-by.  Acked-by is used
by the maintainer when a patch is going in via some other tree other
than the Maintainer's (it means the Maintainer has acked the patch).
If you are reviewing a patch, the tag you should be adding is
Reviewed-by.

Cheers,

					- Ted

Re: [PATCH] ext4: Variable to signed to check return code

[Lee Jones]

On Fri, 17 May 2019, Theodore Ts'o wrote:

> On Fri, May 17, 2019 at 11:25:06AM +0100, Lee Jones wrote:
> > On Fri, 17 May 2019, Philippe Mazenauer wrote:
> > 
> > > Variables 'n' and 'err' are both used for less-than-zero error checking,
> > > however both are declared as unsigned. Ensure ext4_map_blocks() and
> > > add_system_zone() are able to have their return values propagated
> > > correctly by redefining them both as signed integers.
> 
> This is already fixed in the ext4.git tree; it will be pushed to Linus
> shortly.  (Thanks to Colin Ian King from Canonical for sending the
> patch.)
> 
> > Acked-by: Lee Jones <lee.jones@linaro.org>
> 
> Lee, techncially this should have been Reviewed-by.  Acked-by is used
> by the maintainer when a patch is going in via some other tree other
> than the Maintainer's (it means the Maintainer has acked the patch).
> If you are reviewing a patch, the tag you should be adding is
> Reviewed-by.

Actually, that's not technically correct.

  "- Acked-by: indicates an agreement by another developer (often a
     maintainer of the relevant code) that the patch is appropriate for
     inclusion into the kernel."

And I, as a developer (and not a Maintainer in this case) do indicate
that this patch is appropriate for inclusion into the kernel.

Reviewed-by has stronger connotations and implies I have in-depth
knowledge of the subsystem/driver AND agree to the Reviewer's
Statement.  I use Acked-by in this case as a weaker agreement after a
shallow review of the patch based on its merits alone.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] ext4: Variable to signed to check return code

[Theodore Ts'o]

On Sat, May 18, 2019 at 07:38:34AM +0100, Lee Jones wrote:
>   "- Acked-by: indicates an agreement by another developer (often a
>      maintainer of the relevant code) that the patch is appropriate for
>      inclusion into the kernel."
> 
> And I, as a developer (and not a Maintainer in this case) do indicate
> that this patch is appropriate for inclusion into the kernel.
> 
> Reviewed-by has stronger connotations and implies I have in-depth
> knowledge of the subsystem/driver AND agree to the Reviewer's
> Statement.  I use Acked-by in this case as a weaker agreement after a
> shallow review of the patch based on its merits alone.

Note the "often a maintainer of the relevant code" bit.  And
"appropriate for inclusion into the kernel" means to me that you've
done the same level of review as Reviewed-by.  So I have very
different understanding of how Acked-by and Reviewed-by than you do.

That being said, no offence to you, but any kind of Acked-by or
Reviewed-by from you is not going to have as much weight as say, a
Reviewed-by: from someone like Jan Kara.  That's just because I don't
have a good sense to your technical ability, and so I'd be doing a
full review myself and not rely on your review at all....

Cheers,

					- Ted

P.S.  And if I find a problem in the patch, and someone had attached
their Acked-by or Reviewed-by to it, it would have the same negative
hit to their reputation either way.  Not a big deal if it happens only
once, or it's an esepcially tricky issue, but it if happens more than
once or is really blatent, I as the maintainer definitely do remember.

Re: [PATCH] ext4: Variable to signed to check return code

[Lee Jones]

On Sat, 18 May 2019, Theodore Ts'o wrote:

> On Sat, May 18, 2019 at 07:38:34AM +0100, Lee Jones wrote:
> >   "- Acked-by: indicates an agreement by another developer (often a
> >      maintainer of the relevant code) that the patch is appropriate for
> >      inclusion into the kernel."
> > 
> > And I, as a developer (and not a Maintainer in this case) do indicate
> > that this patch is appropriate for inclusion into the kernel.
> > 
> > Reviewed-by has stronger connotations and implies I have in-depth
> > knowledge of the subsystem/driver AND agree to the Reviewer's
> > Statement.  I use Acked-by in this case as a weaker agreement after a
> > shallow review of the patch based on its merits alone.
> 
> Note the "often a maintainer of the relevant code" bit.  And

Exactly, with the *often* (but not always, right!) being the operative
word in that sentence.  As much as I understand the meaning when used
by a Maintainer when commenting on their own subsystem (I use it in
this way a lot too), it doesn't always mean "it's okay for you to take
this patch which usually comes under my jurisdiction".

I think you're missing and if () statement in your understanding:

if (maintainer_of_patches'_subsystem)
   apply_patch_with_supplied_ack();
else
   /* Where 'n' is the regard you hold for the ack supplier. */
   add_n_units_to_patch_credibility(n);

> "appropriate for inclusion into the kernel" means to me that you've
> done the same level of review as Reviewed-by.  So I have very

Actually it doesn't, or else the documentation text for Acked-by would
be just as intense as it is for Reviewed-by.  Reviewed-by IMHO has a
much stronger standing than an Acked-by (caveat: when not provided by
a maintainer of the appropriate subsystem).

> different understanding of how Acked-by and Reviewed-by than you do.

Yes, this is seemingly the case.  It's apparent that the documentation
is not a clear as perhaps it should be, else we wouldn't be having
this conversation.  Maybe this is something which should be discussed
a Kernel Summit.  The result being a patch or two which firms up the
wording/explanation somewhat.

> That being said, no offence to you, but any kind of Acked-by or
> Reviewed-by from you is not going to have as much weight as say, a
> Reviewed-by: from someone like Jan Kara.

Seeing as Jan is a filesystem maintainer, this kind of goes without
saying.  In fact, the only reason a person might have to take the time
to write something like this is to attempt to belittle and cause
offence.  I could be wrong, but probably not. :)

> That's just because I don't have a good sense to your technical
> ability

I guess you could always use Git to gain a reasonable sense of my
technical ability.  The 4,000 committed contributions and many more
thousands of reviews on the mailing list(s), should give you a brief
glimpse.

> and so I'd be doing a full review myself

I'd think a great deal less of you if you didn't.

> and not rely on your review at all....

"at all" - wow!  What kind of message do you think this gives to first
time contributors (like Philippe here), or would-be reviewers?  That
there isn't any point in attempting to review patches, since
Maintainers are unlikely to take it into consideration "at all"?  I
know that when I come to review a patch, if *any* contributor has
taken the time to review a patch, it always plays an important role.

> P.S.  And if I find a problem in the patch, and someone had attached
> their Acked-by or Reviewed-by to it, it would have the same negative
> hit to their reputation either way.  Not a big deal if it happens only
> once, or it's an esepcially tricky issue, but it if happens more than
> once or is really blatent, I as the maintainer definitely do remember.

Again, not really sure of your intentions when you write this out, or
what it has to do with this patch submission or the review there
after, but IMHO this is sending the wrong message to new and would-be
contributors.  As a community we're supposed to be providing a
supportive, encouraging atmosphere.  This paragraph is likely to do
nothing more than scare off people who would otherwise be willing
to have a go [at submitting or reviewing a patch].

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] ext4: Variable to signed to check return code

[Theodore Ts'o]

On Mon, May 20, 2019 at 09:24:02AM +0100, Lee Jones wrote:
> > "appropriate for inclusion into the kernel" means to me that you've
> > done the same level of review as Reviewed-by.  So I have very
> 
> Actually it doesn't, or else the documentation text for Acked-by would
> be just as intense as it is for Reviewed-by.  Reviewed-by IMHO has a
> much stronger standing than an Acked-by (caveat: when not provided by
> a maintainer of the appropriate subsystem).

Well quoting from submitting-patches: "It [Acked-by: ] is a record
that the acker has AT LEAST REVIEWED THE PATCH" (emphasis mine).

The primary use of it is to "signify and record their approval of it".
And...

    Acked-by: does not necessarily indicate acknowledgement of the
    entire patch.  For example, if a patch affects multiple subsystems
    and has an Acked-by: from one subsystem maintainer then this
    usually indicates acknowledgement of just the part which affects
    that maintainer's code.  Judgement should be used here.  When in
    doubt people should refer to the original discussion in the
    mailing list archives.

My question is what is the *point* of including a non-maintainer's
Acked-by: to the git record?  And if it's not the maintainer (or a
core developer for a subsystem), then it becomes unclear what portion
of the patch the non-Maintainer has reviewed.  So at that point, how
can a Maintainer rely on a non-Maintainer Acked-by at all in the first
place?

> > and so I'd be doing a full review myself
> 
> I'd think a great deal less of you if you didn't.
> 
> > and not rely on your review at all....
> 
> "at all" - wow!  What kind of message do you think this gives to first
> time contributors (like Philippe here), or would-be reviewers?  That
> there isn't any point in attempting to review patches, since
> Maintainers are unlikely to take it into consideration "at all"?  I
> know that when I come to review a patch, if *any* contributor has
> taken the time to review a patch, it always plays an important role.

So if I'm going to have to do a full review (which you approve), that
by definition means I'm not relying on the review at all, right?

The way I handle things is that while I'm not going to rely on a
Reviewed-by from an unknown reviewer, I do remember who provides
reviews, and this will bump up their reputation so that perhaps in the
future, I will rely on their reviews.  Reviews that including some
kind of substantive comments (if correct) will enhance the reviewer's
reputation much more than a blind "Reviewd-by: ".

BTW, empty reviews for a patch authored by alice@company-foo.com,
coming from bob@company-foo.com (e.g., where the only content of the
review is Reviewed-by: bob@company-foo.com) are things that I give
much less weight, especially when bob@company-foo.com is not a known
developer to me.

(Yes, I know that sometimes patches get developed behind closed doors,
and then squirt out fully formed with a four or five Reviewed-by:
lines.  But if I haven't seen the process, it doesn't give much value
to the maintainer trying to judge the suitability of the patch.  Red
Hat's approach to do all patch discussions in the open is highly to be
commended here.)

> Again, not really sure of your intentions when you write this out, or
> what it has to do with this patch submission or the review there
> after, but IMHO this is sending the wrong message to new and would-be
> contributors.  As a community we're supposed to be providing a
> supportive, encouraging atmosphere.  This paragraph is likely to do
> nothing more than scare off people who would otherwise be willing
> to have a go [at submitting or reviewing a patch].

One of the things that I worry about are people who game git
statistics by submitting a lot of empty Reviewed-by or Acked-by lines.
It's right up there with people who send huge numbers of whitespace
fixes.  So my personal approach is to not include Reviewed-by or
Acked-by if it didn't add any value to the project.  It may indeed
still value to the reviewer's reputation, and if the reviewer has
helped to improve the patch, I'll make sure they get credit.

I do agree that we should provide a supportive atmosphere.  But we
also need provide encouragement that contributors provide more
substantive patches and more substantive reviewes.

Cheers,

					- Ted

Re: [PATCH] ext4: Variable to signed to check return code

[Lee Jones]

On Mon, 20 May 2019, Theodore Ts'o wrote:

> On Mon, May 20, 2019 at 09:24:02AM +0100, Lee Jones wrote:
> > > "appropriate for inclusion into the kernel" means to me that you've
> > > done the same level of review as Reviewed-by.  So I have very
> > 
> > Actually it doesn't, or else the documentation text for Acked-by would
> > be just as intense as it is for Reviewed-by.  Reviewed-by IMHO has a
> > much stronger standing than an Acked-by (caveat: when not provided by
> > a maintainer of the appropriate subsystem).
> 
> Well quoting from submitting-patches: "It [Acked-by: ] is a record
> that the acker has AT LEAST REVIEWED THE PATCH" (emphasis mine).

Absolutely.  Anyone who hasn't reviewed a patch to the best of their
ability before providing their {Acked,Reviewed}-by would be a very
silly person indeed.

But that's not what you said and it not what I'm disagreeing with, is
it?  See above, you said "[an Acked-by] means to me that you've done
the same level of review as Reviewed-by", which I do not agree with.

A Reviewed-by to me means that a person knows the area, including
possible side-effects a patch may cause.  In this EXTx case, I do not
consider myself a domain expert and thus am not in a position to
provide that level of review.  Instead, the patch was reviewed on its
own merits, and since it looked good (which it still does), an
Acked-by was provided.

> The primary use of it is to "signify and record their approval of it".
> And...
> 
>     Acked-by: does not necessarily indicate acknowledgement of the
>     entire patch.  For example, if a patch affects multiple subsystems
>     and has an Acked-by: from one subsystem maintainer then this
>     usually indicates acknowledgement of just the part which affects
>     that maintainer's code.  Judgement should be used here.  When in
>     doubt people should refer to the original discussion in the
>     mailing list archives.

Exactly, there's that *if* I was talking about.  "IF, a patch affects
multiple subsystems".  This patch does not.  The Acked by is also not
coming from a maintainer from this subsystem, thus this statement does
not come into play.

> My question is what is the *point* of including a non-maintainer's
> Acked-by: to the git record?

You'll have to take that up with the documentation, which clearly
states:

 "Acked-by: indicates an agreement by another developer (often a
  maintainer of the relevant code) that the patch is appropriate for
  inclusion into the kernel.

... which is *exactly* as I meant it.  I, as 'another developer', do
consider this patch appropriate for the inclusion into the kernel.

How is that less than totally clear?  If the documentation is wrong,
something needs to be done about that.  Until then, I shall continue
using Acked-bys to mean "I'm not an expert in this area, but based on
the patch alone, the change looks good to me".

> And if it's not the maintainer (or a
> core developer for a subsystem), then it becomes unclear what portion
> of the patch the non-Maintainer has reviewed.

A reviewer will normally indicate which portion of the code has been
reviewed, which can then be reflected in the change-log.  If a
maintainer Ack is applied, it can be assumed that only the part under
their jurisdiction is relevant.  If not a maintainer or core-dev, and
no extra indication is provided, then I think we can assume they
reviewed the whole patch.  Extra indication can look like this in the
*rare cases* a non-maintainer/core-dev provides a part-review:

  Acked-by: Joe Bloggs <j.b@someemail.com> [ext2]

See 8a7f97b902f4 ("treewide: add checks for the return value of
memblock_alloc*()") as the first example I came across.

> So at that point, how
> can a Maintainer rely on a non-Maintainer Acked-by at all in the first
> place?

At what point?

> > > and so I'd be doing a full review myself
> > 
> > I'd think a great deal less of you if you didn't.
> > 
> > > and not rely on your review at all....
> > 
> > "at all" - wow!  What kind of message do you think this gives to first
> > time contributors (like Philippe here), or would-be reviewers?  That
> > there isn't any point in attempting to review patches, since
> > Maintainers are unlikely to take it into consideration "at all"?  I
> > know that when I come to review a patch, if *any* contributor has
> > taken the time to review a patch, it always plays an important role.
> 
> So if I'm going to have to do a full review (which you approve), that
> by definition means I'm not relying on the review at all, right?

Your review should not replace and over-ride another review (unless
you disagree with it, obviously), it should compliment it.  Both *-bys
should be added to the patch when/if it is applied.

> The way I handle things is that while I'm not going to rely on a
> Reviewed-by from an unknown reviewer, I do remember who provides
> reviews, and this will bump up their reputation so that perhaps in the
> future, I will rely on their reviews.  Reviews that including some
> kind of substantive comments (if correct) will enhance the reviewer's
> reputation much more than a blind "Reviewd-by: ".

Perfect.  This is a positive statement. The way you worded this before
was very negative and almost sounded like a threat.

> BTW, empty reviews for a patch authored by alice@company-foo.com,
> coming from bob@company-foo.com (e.g., where the only content of the
> review is Reviewed-by: bob@company-foo.com) are things that I give
> much less weight, especially when bob@company-foo.com is not a known
> developer to me.

Absolutely.  I even alluded to this in my previous email.  Ah bugger!
After trying and find that part in order to quote it, it became
apparent that I took that sentence out.  But yes, I totally agree with
you, reviews mean more if they are from an independent source.

> (Yes, I know that sometimes patches get developed behind closed doors,
> and then squirt out fully formed with a four or five Reviewed-by:
> lines.  But if I haven't seen the process, it doesn't give much value
> to the maintainer trying to judge the suitability of the patch.  Red
> Hat's approach to do all patch discussions in the open is highly to be
> commended here.)

+1

NB: I still keep the *-bys applied when merging a patch, even if they
do not hold much gravitas during my review process.

> > Again, not really sure of your intentions when you write this out, or
> > what it has to do with this patch submission or the review there
> > after, but IMHO this is sending the wrong message to new and would-be
> > contributors.  As a community we're supposed to be providing a
> > supportive, encouraging atmosphere.  This paragraph is likely to do
> > nothing more than scare off people who would otherwise be willing
> > to have a go [at submitting or reviewing a patch].
> 
> One of the things that I worry about are people who game git
> statistics by submitting a lot of empty Reviewed-by or Acked-by lines.

I can't think of a time I have been on either end of this situation.
If I saw anyone doing this I would call them out on it.

Just to clarify, in case you think that's what is happening here (I'm
sure you've looked though the Git log and seen for yourself, but
still); I reviewed this patch solely because I am helping Philippe
through the process of becoming a Kernel contributor, since he showed
a great deal of interest and enthusiasm for it when I met him a few
weeks ago whilst rock climbing.

> It's right up there with people who send huge numbers of whitespace
> fixes.

Right, this drives me mad also.

> So my personal approach is to not include Reviewed-by or
> Acked-by if it didn't add any value to the project.

That's wrong of you.  I do not support this behaviour at all.  If
someone has gone to the trouble to review a patch and provide a
suitable *-by, you, as the maintainer have a duty to credit this work
by applying it to the patch (if accepted).

Of course the statement above comes with some caveats.  For example if
you think someone is gaming the Git statistics, then you have a right
to call them out and not apply the tag.  However, you really should be
giving people (especially people you don't know) the benefit of the
doubt.

> It may indeed
> still value to the reviewer's reputation, and if the reviewer has
> helped to improve the patch, I'll make sure they get credit.
> 
> I do agree that we should provide a supportive atmosphere.  But we
> also need provide encouragement that contributors provide more
> substantive patches and more substantive reviewes.

That policy is crazy.  You are saying that people should only be
providing negative reviews?  So what happens if someone conducts a
review and they cannot find anything wrong with the submission?  You
are suggesting that you are not going to apply their tag anyway, so
what would be the point in them providing one?  You are essentially
saying that unless they have previously given a patch a NACK, then
don't bother to provide an ACK.  Bonkers!

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] ext4: Variable to signed to check return code

[Theodore Ts'o]

On Tue, May 21, 2019 at 08:25:53AM +0100, Lee Jones wrote:
> A Reviewed-by to me means that a person knows the area, including
> possible side-effects a patch may cause.  In this EXTx case, I do not
> consider myself a domain expert and thus am not in a position to
> provide that level of review.  Instead, the patch was reviewed on its
> own merits, and since it looked good (which it still does), an
> Acked-by was provided.

So that kind of "review", where "I'm not an expert, but it looks good
to me", has very, very little value as far as I'm concerned.  A
computer program can do a "it builds, ship it" test, and checkpatch
can find the whitespace nits.  So what value does a "I can't consider
side-effects" review have?  Again, as a maintainer, I can put very
little (read: zero) reliance on it.

> Exactly, there's that *if* I was talking about.  "IF, a patch affects
> multiple subsystems".  This patch does not.  The Acked by is also not
> coming from a maintainer from this subsystem, thus this statement does
> not come into play.

And my assertion is that if a patch does not affect multiple
subsystems, an Acked-by has close to zero value.  So why do it?  In my
opinion, we should tighten up the documentation to only use it for the
Maintainer reviewing a patch that's not flowing through the
Maintainer's tree.

> > > "at all" - wow!  What kind of message do you think this gives to first
> > > time contributors (like Philippe here), or would-be reviewers?  That
> > > there isn't any point in attempting to review patches, since
> > > Maintainers are unlikely to take it into consideration "at all"?  I
> > > know that when I come to review a patch, if *any* contributor has
> > > taken the time to review a patch, it always plays an important role.
> > 
> > So if I'm going to have to do a full review (which you approve), that
> > by definition means I'm not relying on the review at all, right?
> 
> Your review should not replace and over-ride another review (unless
> you disagree with it, obviously), it should compliment it.  Both *-bys
> should be added to the patch when/if it is applied.

We're using "reviewed" in two different ways.  I'm talking about an
empty "reviewed-by" or "acked-by", for which if it's an developer
unknown to me, I have no way of telling how much time they spent on
the review.  Was it 5 seconds?  Or 5 minutes?  Or something in
between?

You're saying that if someone sends a "Reviewed-by" or "Acked-by", I
*must* never drop it, even if I have no idea how much value it has
(and therefore, as far as I'm concerned, it has no value).  Sorry, I'm
not going to play things that way.  Feel free to call me bad if you
want, it's not going to change how I do things.

Worse, what value does it add if we record it for all posterity?  What
should someone who is reviewing the git log after the fact, perhaps a
year later, take from the fact that there is an unknowned Reviewed-By
or Acked-By attached to the commit?  Do you think someone reviewing a
commit year later will spend time crawling over the git history to
determine how many reviews someone has done?

(And if all maintainer's add empty Reviewed-by to their commit,
looking at Git histories might not tell you much anyway.  It cheapens
the Reviewed-by and Acked-by headers.  I consider part of the
maintianer's job to curate not just patches, but Reviewed-By and
Acked-by headers.)

And BTW, if the maintainer is using a non-rewinding git tree, and the
git has already been published on git.kernel.org, it's physically not
*possible* for them to add a late Reviewed-by, or Acked-by.  And I
think that's perfectly justifiable, since if the decision has already
been made to accept the commit, the Reviewed-By or Acked-By has no
value to the project.

> > So my personal approach is to not include Reviewed-by or
> > Acked-by if it didn't add any value to the project.
> 
> That's wrong of you.  I do not support this behaviour at all.  If
> someone has gone to the trouble to review a patch and provide a
> suitable *-by, you, as the maintainer have a duty to credit this work
> by applying it to the patch (if accepted).

But I don't *know* that someone has gone to the trouble to review a
patch.  If they made any kind of comment (positive or negative, or
evaluating tradeoffs), then I have some kind of signal about how much
time they spent reviewing the patch, and how much comprehension they
have about the patch.  This is why my metric is "value to the
subsystem / value to the project as a whole".

You yourself have asked me to count the number of Reviewed-by you have
as a sign of your technical ability.  Would you then want to make sure
that that signal isn't cheapened?

> That policy is crazy.  You are saying that people should only be
> providing negative reviews?  So what happens if someone conducts a
> review and they cannot find anything wrong with the submission?  You
> are suggesting that you are not going to apply their tag anyway, so
> what would be the point in them providing one?  You are essentially
> saying that unless they have previously given a patch a NACK, then
> don't bother to provide an ACK.  Bonkers!

It's a question of developer history.  Not just of a particular patch,
but their reviewer history as a whole.  If Jan Kara or Andreas Dilger
or Lukas Czerner sends me a empty Reviewed-By, it has great weight,
because they've found issues with other patches in the past, and we've
had design discussions about what is the right way to fix a particular
issue.  But for a someone with whom I've never interacted before, and
all I get is a drive-by, empty Acked-by?  No, it's not going to get
included by me.  Sorry.

					- Ted

Re: [PATCH] ext4: Variable to signed to check return code

[Lee Jones]

On Tue, 21 May 2019, Theodore Ts'o wrote:

> On Tue, May 21, 2019 at 08:25:53AM +0100, Lee Jones wrote:
> > A Reviewed-by to me means that a person knows the area, including
> > possible side-effects a patch may cause.  In this EXTx case, I do not
> > consider myself a domain expert and thus am not in a position to
> > provide that level of review.  Instead, the patch was reviewed on its
> > own merits, and since it looked good (which it still does), an
> > Acked-by was provided.
> 
> So that kind of "review", where "I'm not an expert, but it looks good
> to me", has very, very little value as far as I'm concerned.  A
> computer program can do a "it builds, ship it" test, and checkpatch
> can find the whitespace nits.  So what value does a "I can't consider
> side-effects" review have?  Again, as a maintainer, I can put very
> little (read: zero) reliance on it.

So what your asking is that only domain experts are to review
patches in the subsystems you maintain.  That's absurd, and among the
least inclusive statements I've heard in a while.  No wonder we have
an issue attracting new reviewers/maintainers to the fray.

> > Exactly, there's that *if* I was talking about.  "IF, a patch affects
> > multiple subsystems".  This patch does not.  The Acked by is also not
> > coming from a maintainer from this subsystem, thus this statement does
> > not come into play.
> 
> And my assertion is that if a patch does not affect multiple
> subsystems, an Acked-by has close to zero value.  So why do it?  In my

At the moment, your assertion isn't based on the documented meaning.
So you're enforcing rules based on what you think it should mean,
rather than what it actually means (according to the current
and up-to-date documentation).

> opinion, we should tighten up the documentation to only use it for the
> Maintainer reviewing a patch that's not flowing through the
> Maintainer's tree.

That's your opinion, and you're entitled to it.  But frankly that's
all it is at the moment.  If you wish to push this meaning onto
others, you need to start a conversation and/or submit patches for
review.

> > > > "at all" - wow!  What kind of message do you think this gives to first
> > > > time contributors (like Philippe here), or would-be reviewers?  That
> > > > there isn't any point in attempting to review patches, since
> > > > Maintainers are unlikely to take it into consideration "at all"?  I
> > > > know that when I come to review a patch, if *any* contributor has
> > > > taken the time to review a patch, it always plays an important role.
> > > 
> > > So if I'm going to have to do a full review (which you approve), that
> > > by definition means I'm not relying on the review at all, right?
> > 
> > Your review should not replace and over-ride another review (unless
> > you disagree with it, obviously), it should compliment it.  Both *-bys
> > should be added to the patch when/if it is applied.
> 
> We're using "reviewed" in two different ways.  I'm talking about an
> empty "reviewed-by" or "acked-by", for which if it's an developer
> unknown to me, I have no way of telling how much time they spent on
> the review.  Was it 5 seconds?  Or 5 minutes?  Or something in
> between?

What is an empty {Acked,Reviewed}-by?  We have no provision for
indicating time taken to review.  As maintainers we have to make the
call on how many 'credits' to award the {Acked,Reviewed}-by based on
our experiences of that person.  I have reviewers I trust and
reviewers who are new to me.  My trust levels of a patch increase a
given amount depending on who they are and how many I receive.  That
level is seldom zero though!

> You're saying that if someone sends a "Reviewed-by" or "Acked-by", I
> *must* never drop it, even if I have no idea how much value it has
> (and therefore, as far as I'm concerned, it has no value).  Sorry, I'm
> not going to play things that way.  Feel free to call me bad if you
> want, it's not going to change how I do things.

Yes, that's bad.

> Worse, what value does it add if we record it for all posterity?  What
> should someone who is reviewing the git log after the fact, perhaps a
> year later, take from the fact that there is an unknowned Reviewed-By
> or Acked-By attached to the commit?  Do you think someone reviewing a
> commit year later will spend time crawling over the git history to
> determine how many reviews someone has done?

Don't forget these reviewers you speak of, they are only "unknown" to
you.  They will have; friends, co-workers, employers, potential
employers, community colleagues, etc.  Their interest in a patch,
driver or sub-system may not be known to you, but may be intrinsic to
their role else where.  Who are you (we) to decide who is unimportant
when it comes to our areas of responsibility?  Or who should and
should not gain credit for their efforts?

It's about given credit where credit is due.

> (And if all maintainer's add empty Reviewed-by to their commit,
> looking at Git histories might not tell you much anyway.  It cheapens
> the Reviewed-by and Acked-by headers.  I consider part of the
> maintianer's job to curate not just patches, but Reviewed-By and
> Acked-by headers.)

There is no such thing as an empty {Acked,Reviewed}-by - you made that
up.  Unless someone is gaming the Git statistics and giving out *-bys
willy-nilly, it means they have put some effort into reviewing a
patch.

Cheapens the headers?  Total nonsense!  Any review, even by non-domain
experts only serves to strengthen the likelihood that the patch is of
the required quality.

I'm sorry to say, but I find your views dated, elitist and arrogant.

> And BTW, if the maintainer is using a non-rewinding git tree, and the
> git has already been published on git.kernel.org, it's physically not
> *possible* for them to add a late Reviewed-by, or Acked-by.  And I
> think that's perfectly justifiable, since if the decision has already
> been made to accept the commit, the Reviewed-By or Acked-By has no
> value to the project.

Agreed.  Late reviews are generally not credited.

> > > So my personal approach is to not include Reviewed-by or
> > > Acked-by if it didn't add any value to the project.
> > 
> > That's wrong of you.  I do not support this behaviour at all.  If
> > someone has gone to the trouble to review a patch and provide a
> > suitable *-by, you, as the maintainer have a duty to credit this work
> > by applying it to the patch (if accepted).
> 
> But I don't *know* that someone has gone to the trouble to review a
> patch.  If they made any kind of comment (positive or negative, or
> evaluating tradeoffs), then I have some kind of signal about how much
> time they spent reviewing the patch, and how much comprehension they
> have about the patch.  This is why my metric is "value to the
> subsystem / value to the project as a whole".

No, you don't know.  You can never know.  That's why you have to give
the benefit of the doubt in most cases.  How much credit you actually
give to the review is your decision as the maintainer - it should not
be your decision to dismiss it completely based on your (our)
ignorance of who this person might be.

> You yourself have asked me to count the number of Reviewed-by you have
> as a sign of your technical ability.  Would you then want to make sure
> that that signal isn't cheapened?

Actually my SoBs would have been more important, but that's by the
by.

Granted, some (not all, not most, not even many) people write lots of
trivial patches, submit reviews which have not been thought out
properly, game Git statistics, etc.  They should be dealt with in the
appropriate manor.  However until proven guilty, reviewers should be
guided and encouraged to do continue doing so.  These are our future
maintainers and should be nurtured.  You absolutely should not ignore
their efforts.  Doing so will only make them feel undervalued and will
likely discourage them from further contributions.

> > That policy is crazy.  You are saying that people should only be
> > providing negative reviews?  So what happens if someone conducts a
> > review and they cannot find anything wrong with the submission?  You
> > are suggesting that you are not going to apply their tag anyway, so
> > what would be the point in them providing one?  You are essentially
> > saying that unless they have previously given a patch a NACK, then
> > don't bother to provide an ACK.  Bonkers!
> 
> It's a question of developer history.  Not just of a particular patch,
> but their reviewer history as a whole.  If Jan Kara or Andreas Dilger
> or Lukas Czerner sends me a empty Reviewed-By, it has great weight,
> because they've found issues with other patches in the past, and we've
> had design discussions about what is the right way to fix a particular
> issue.  But for a someone with whom I've never interacted before, and
> all I get is a drive-by, empty Acked-by?  No, it's not going to get
> included by me.  Sorry.

That's not a question of developer history.  If you'd conducted your
due diligence on me (just using this patch as an example), you would
have seen that I have a solid 'developer history'.  What you're
judging on is your own opinion based on your experiences alone.  As I
said before, people shouldn't be ignored/discouraged based on our
ignorance of their existence.

Deciding not to apply a *-by because you have no way to gauge the
level quality of the review, due solely to a gap in your own knowledge
is an awful policy.

This discussion is starting to take up a lot of my time now, so I'd
like it to come to a close.  I guess the summary is; if you wish to
continue being; discouraging, exclusive and elitist, you continue
doing things your way.  If you wish other contributors to adhere to
your meanings of the various *-by tags, you'll have to conduct a
discussion and update the documentation accordingly.  I'd certainly be
happy to change the way I use them, but only if reflected clearly in
the documentation.

Have a lovely day Ted.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog