Hi Markus,
(Cc'ing Yi, Clément and Zhenzhong for commit eda4c9b5b3c)
On 12/3/25 10:45, Markus Armbruster wrote:
I stumbled over commits that carry the author's Reviewed-by.
There may be cases where the recorded author isn't the lone author, and
the recorded author did some meaningful review of the patch's parts that
are not theirs. Mind that we do need all authors to provide their
Signed-off-by.
When the only Signed-off-by is from the recorded author, and there's
also their Reviewed-by, the Reviewed-by is almost certainly bogus.
Now, accidents happen, no big deal, etc., etc. I post this to hopefully
help reduce the accident rate :)
Here's my quick & sloppy search for potentially problematic uses of
Reviewed-by:
$ git-log --since 'two years ago' | awk -F: '/^commit / { commit=$0 } /^Author:
/ { guy=$2 } /^ Reviewed-by: / { if ($2 == guy) { print commit; print guy }
}'
Explaining some commits where I'm mentioned:
commit 1e0d4eb4ee7c909323bffc39bc348eb3174b426b
Author: Philippe Mathieu-Daudé <[email protected]>
Date: Fri Apr 12 00:33:30 2024 -0700
backends/tpm: Use qemu_hexdump_line() to avoid sprintf()
sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1.
Using qemu_hexdump_line() both fixes the deprecation warning and
simplifies the code base.
Signed-off-by: Philippe Mathieu-Daudé <[email protected]>
Reviewed-by: Stefan Berger <[email protected]>
[rth: Keep the linebreaks every 16 bytes]
Signed-off-by: Richard Henderson <[email protected]>
Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
Message-ID: <[email protected]>
[PMD: Rebased]
I posted a patch with my S-o-b; Richard took it, improved and reposted
it with his S-o-b; I reviewed Richard's changes (and eventually merged).
commit 0fe4cac5dda1028c22ec3a6997e1b9155a768004
Author: Peter Maydell <[email protected]>
Date: Mon Jul 17 18:29:40 2023 +0200
target/mips: Avoid shift by negative number in page_table_walk_refill()
Coverity points out that in page_table_walk_refill() we can
shift by a negative number, which is undefined behaviour
(CID 1452918, 1452920, 1452922). We already catch the
negative directory_shift and leaf_shift as being a "bail
out early" case, but not until we've already used them to
calculated some offset values.
The shifts can be negative only if ptew > 1, so make the
bail-out-early check look directly at that, and only
calculate the shift amounts and the offsets based on them
after we have done that check. This allows
us to simplify the expressions used to calculate the
shift amounts, use an unsigned type, and avoids the
undefined behaviour.
Signed-off-by: Peter Maydell <[email protected]>
[PMD: Check for ptew > 1, use unsigned type]
Signed-off-by: Philippe Mathieu-Daudé <[email protected]>
Reviewed-by: Peter Maydell <[email protected]>
Message-Id: <[email protected]>
Peter posted the first patch, I reworked it and reposted,
Peter reviewed my changes.
commit c4380f7bcdcb68fdfca876db366782a807fab8f7
Author: Richard Henderson <[email protected]>
Date: Thu Jan 18 21:06:30 2024 +0100
target/arm: Create arm_cpu_mp_affinity
Wrapper to return the mp affinity bits from the cpu.
Signed-off-by: Richard Henderson <[email protected]>
Signed-off-by: Philippe Mathieu-Daudé <[email protected]>
Reviewed-by: Richard Henderson <[email protected]>
Message-id: [email protected]
Signed-off-by: Peter Maydell <[email protected]>
Is this workflow making sense and accepted? Otherwise what should
we change? Maybe clarify along with the tags; or including all
Message-Id could make this easier to track?
Regards,
Phil.
