fsdev/file-op-9p.h | 5 + fsdev/qemu-fsdev-opts.c | 7 +- fsdev/qemu-fsdev.c | 11 ++ hw/9pfs/9p.c | 488 +++++++++++++++++++++++++++++++++++++++++++++--- hw/9pfs/9p.h | 51 +++++ qemu-options.hx | 33 +++- vl.c | 6 +- 7 files changed, 565 insertions(+), 36 deletions(-)
This is v6 of a proposed patch set for fixing file ID collisions with 9pfs.
v5->v6:
* Rebased to https://github.com/gkurz/qemu/commits/9p-next
(SHA1 177fd3b6a8).
* Replaced previous boolean option 'remap_inodes' by tertiary option
'multidevs=remap|forbid|warn', where 'warn' is the new/old default
behaviour for not breaking existing installations:
https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg07098.html
* Dropped incomplete fix in v9fs_do_readdir() which aimed to prevent
exposing info outside export root with '..' entry. Postponed this
fix for now for the reasons described:
https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg01862.html
Christian Schoenebeck (4):
9p: Treat multiple devices on one export as an error
9p: Added virtfs option 'multidevs=remap|forbid|warn'
9p: stat_to_qid: implement slow path
9p: Use variable length suffixes for inode remapping
fsdev/file-op-9p.h | 5 +
fsdev/qemu-fsdev-opts.c | 7 +-
fsdev/qemu-fsdev.c | 11 ++
hw/9pfs/9p.c | 488 +++++++++++++++++++++++++++++++++++++++++++++---
hw/9pfs/9p.h | 51 +++++
qemu-options.hx | 33 +++-
vl.c | 6 +-
7 files changed, 565 insertions(+), 36 deletions(-)
--
2.11.0
Patchew URL: https://patchew.org/QEMU/cover.1566503584.git.qemu_oss@crudebyte.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Subject: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions
Message-id: cover.1566503584.git.qemu_oss@crudebyte.com
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
* [new tag] patchew/cover.1566503584.git.qemu_oss@crudebyte.com -> patchew/cover.1566503584.git.qemu_oss@crudebyte.com
Submodule 'capstone' (https://git.qemu.org/git/capstone.git) registered for path 'capstone'
Submodule 'dtc' (https://git.qemu.org/git/dtc.git) registered for path 'dtc'
Submodule 'roms/QemuMacDrivers' (https://git.qemu.org/git/QemuMacDrivers.git) registered for path 'roms/QemuMacDrivers'
Submodule 'roms/SLOF' (https://git.qemu.org/git/SLOF.git) registered for path 'roms/SLOF'
Submodule 'roms/edk2' (https://git.qemu.org/git/edk2.git) registered for path 'roms/edk2'
Submodule 'roms/ipxe' (https://git.qemu.org/git/ipxe.git) registered for path 'roms/ipxe'
Submodule 'roms/openbios' (https://git.qemu.org/git/openbios.git) registered for path 'roms/openbios'
Submodule 'roms/openhackware' (https://git.qemu.org/git/openhackware.git) registered for path 'roms/openhackware'
Submodule 'roms/opensbi' (https://git.qemu.org/git/opensbi.git) registered for path 'roms/opensbi'
Submodule 'roms/qemu-palcode' (https://git.qemu.org/git/qemu-palcode.git) registered for path 'roms/qemu-palcode'
Submodule 'roms/seabios' (https://git.qemu.org/git/seabios.git/) registered for path 'roms/seabios'
Submodule 'roms/seabios-hppa' (https://git.qemu.org/git/seabios-hppa.git) registered for path 'roms/seabios-hppa'
Submodule 'roms/sgabios' (https://git.qemu.org/git/sgabios.git) registered for path 'roms/sgabios'
Submodule 'roms/skiboot' (https://git.qemu.org/git/skiboot.git) registered for path 'roms/skiboot'
Submodule 'roms/u-boot' (https://git.qemu.org/git/u-boot.git) registered for path 'roms/u-boot'
Submodule 'roms/u-boot-sam460ex' (https://git.qemu.org/git/u-boot-sam460ex.git) registered for path 'roms/u-boot-sam460ex'
Submodule 'slirp' (https://git.qemu.org/git/libslirp.git) registered for path 'slirp'
Submodule 'tests/fp/berkeley-softfloat-3' (https://git.qemu.org/git/berkeley-softfloat-3.git) registered for path 'tests/fp/berkeley-softfloat-3'
Submodule 'tests/fp/berkeley-testfloat-3' (https://git.qemu.org/git/berkeley-testfloat-3.git) registered for path 'tests/fp/berkeley-testfloat-3'
Submodule 'ui/keycodemapdb' (https://git.qemu.org/git/keycodemapdb.git) registered for path 'ui/keycodemapdb'
Cloning into 'capstone'...
Submodule path 'capstone': checked out '22ead3e0bfdb87516656453336160e0a37b066bf'
Cloning into 'dtc'...
Submodule path 'dtc': checked out '88f18909db731a627456f26d779445f84e449536'
Cloning into 'roms/QemuMacDrivers'...
Submodule path 'roms/QemuMacDrivers': checked out '90c488d5f4a407342247b9ea869df1c2d9c8e266'
Cloning into 'roms/SLOF'...
Submodule path 'roms/SLOF': checked out '7bfe584e321946771692711ff83ad2b5850daca7'
Cloning into 'roms/edk2'...
Submodule path 'roms/edk2': checked out '20d2e5a125e34fc8501026613a71549b2a1a3e54'
Submodule 'SoftFloat' (https://github.com/ucb-bar/berkeley-softfloat-3.git) registered for path 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3'
Submodule 'CryptoPkg/Library/OpensslLib/openssl' (https://github.com/openssl/openssl) registered for path 'CryptoPkg/Library/OpensslLib/openssl'
Cloning into 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3'...
Submodule path 'roms/edk2/ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3': checked out 'b64af41c3276f97f0e181920400ee056b9c88037'
Cloning into 'CryptoPkg/Library/OpensslLib/openssl'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl': checked out '50eaac9f3337667259de725451f201e784599687'
Submodule 'boringssl' (https://boringssl.googlesource.com/boringssl) registered for path 'boringssl'
Submodule 'krb5' (https://github.com/krb5/krb5) registered for path 'krb5'
Submodule 'pyca.cryptography' (https://github.com/pyca/cryptography.git) registered for path 'pyca-cryptography'
Cloning into 'boringssl'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/boringssl': checked out '2070f8ad9151dc8f3a73bffaa146b5e6937a583f'
Cloning into 'krb5'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/krb5': checked out 'b9ad6c49505c96a088326b62a52568e3484f2168'
Cloning into 'pyca-cryptography'...
Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/pyca-cryptography': checked out '09403100de2f6f1cdd0d484dcb8e620f1c335c8f'
Cloning into 'roms/ipxe'...
Submodule path 'roms/ipxe': checked out 'de4565cbe76ea9f7913a01f331be3ee901bb6e17'
Cloning into 'roms/openbios'...
Submodule path 'roms/openbios': checked out 'c79e0ecb84f4f1ee3f73f521622e264edd1bf174'
Cloning into 'roms/openhackware'...
Submodule path 'roms/openhackware': checked out 'c559da7c8eec5e45ef1f67978827af6f0b9546f5'
Cloning into 'roms/opensbi'...
Submodule path 'roms/opensbi': checked out 'ce228ee0919deb9957192d723eecc8aaae2697c6'
Cloning into 'roms/qemu-palcode'...
Submodule path 'roms/qemu-palcode': checked out 'bf0e13698872450164fa7040da36a95d2d4b326f'
Cloning into 'roms/seabios'...
Submodule path 'roms/seabios': checked out 'a5cab58e9a3fb6e168aba919c5669bea406573b4'
Cloning into 'roms/seabios-hppa'...
Submodule path 'roms/seabios-hppa': checked out '0f4fe84658165e96ce35870fd19fc634e182e77b'
Cloning into 'roms/sgabios'...
Submodule path 'roms/sgabios': checked out 'cbaee52287e5f32373181cff50a00b6c4ac9015a'
Cloning into 'roms/skiboot'...
Submodule path 'roms/skiboot': checked out '261ca8e779e5138869a45f174caa49be6a274501'
Cloning into 'roms/u-boot'...
Submodule path 'roms/u-boot': checked out 'd3689267f92c5956e09cc7d1baa4700141662bff'
Cloning into 'roms/u-boot-sam460ex'...
Submodule path 'roms/u-boot-sam460ex': checked out '60b3916f33e617a815973c5a6df77055b2e3a588'
Cloning into 'slirp'...
Submodule path 'slirp': checked out '126c04acbabd7ad32c2b018fe10dfac2a3bc1210'
Cloning into 'tests/fp/berkeley-softfloat-3'...
Submodule path 'tests/fp/berkeley-softfloat-3': checked out 'b64af41c3276f97f0e181920400ee056b9c88037'
Cloning into 'tests/fp/berkeley-testfloat-3'...
Submodule path 'tests/fp/berkeley-testfloat-3': checked out '5a59dcec19327396a011a17fd924aed4fec416b3'
Cloning into 'ui/keycodemapdb'...
Submodule path 'ui/keycodemapdb': checked out '6b3d716e2b6472eb7189d3220552280ef3d832ce'
Switched to a new branch 'test'
28d6fb8 9p: Use variable length suffixes for inode remapping
23fc4f6 9p: stat_to_qid: implement slow path
f227708 9p: Added virtfs option 'multidevs=remap|forbid|warn'
bb69de6 9p: Treat multiple devices on one export as an error
=== OUTPUT BEGIN ===
1/4 Checking commit bb69de63f788 (9p: Treat multiple devices on one export as an error)
ERROR: Author email address is mangled by the mailing list
#2:
Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>
total: 1 errors, 0 warnings, 175 lines checked
Patch 1/4 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/4 Checking commit f227708a87da (9p: Added virtfs option 'multidevs=remap|forbid|warn')
ERROR: Author email address is mangled by the mailing list
#2:
Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>
WARNING: Block comments use a leading /* on a separate line
#167: FILE: hw/9pfs/9p.c:600:
+/* stat_to_qid needs to map inode number (64 bits) and device id (32 bits)
ERROR: line over 90 characters
#467: FILE: vl.c:3338:
+ const char *writeout, *sock_fd, *socket, *path, *security_model, *multidevs;
total: 2 errors, 1 warnings, 394 lines checked
Patch 2/4 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/4 Checking commit 23fc4f6d5258 (9p: stat_to_qid: implement slow path)
ERROR: Author email address is mangled by the mailing list
#2:
Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>
WARNING: line over 80 characters
#79: FILE: hw/9pfs/9p.c:622:
+ qht_init(&pdu->s->qpf_table, qpf_lookup_func, 1 << 16, QHT_MODE_AUTO_RESIZE);
total: 1 errors, 1 warnings, 142 lines checked
Patch 3/4 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
4/4 Checking commit 28d6fb899725 (9p: Use variable length suffixes for inode remapping)
ERROR: Author email address is mangled by the mailing list
#2:
Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>
ERROR: space prohibited after that open parenthesis '('
#92: FILE: hw/9pfs/9p.c:586:
+ return ((uint64_t)mirror8bit( value & 0xff) << 56) |
ERROR: space prohibited before that close parenthesis ')'
#98: FILE: hw/9pfs/9p.c:592:
+ ((uint64_t)mirror8bit((value >> 48) & 0xff) << 8 ) |
ERROR: space prohibited before that close parenthesis ')'
#99: FILE: hw/9pfs/9p.c:593:
+ ((uint64_t)mirror8bit((value >> 56) & 0xff) ) ;
WARNING: Block comments use a leading /* on a separate line
#102: FILE: hw/9pfs/9p.c:596:
+/** @brief Parameter k for the Exponential Golomb algorihm to be used.
WARNING: Block comments use a leading /* on a separate line
#121: FILE: hw/9pfs/9p.c:615:
+/** @brief Exponential Golomb algorithm for arbitrary k (including k=0).
WARNING: Block comments use a leading /* on a separate line
#148: FILE: hw/9pfs/9p.c:642:
+/** @brief Converts a suffix into a prefix, or a prefix into a suffix.
ERROR: "foo* bar" should be "foo *bar"
#158: FILE: hw/9pfs/9p.c:652:
+static VariLenAffix invertAffix(const VariLenAffix* affix)
WARNING: line over 80 characters
#161: FILE: hw/9pfs/9p.c:655:
+ .type = (affix->type == AffixType_Suffix) ? AffixType_Prefix : AffixType_Suffix,
WARNING: line over 80 characters
#162: FILE: hw/9pfs/9p.c:656:
+ .value = mirror64bit(affix->value) >> ((sizeof(affix->value) * 8) - affix->bits),
WARNING: Block comments use a leading /* on a separate line
#167: FILE: hw/9pfs/9p.c:661:
+/** @brief Generates suffix numbers with "suffix-free" property.
WARNING: Block comments use a leading /* on a separate line
#246: FILE: hw/9pfs/9p.c:752:
+/** @brief Slow / full mapping host inode nr -> guest inode nr.
WARNING: Block comments use a leading /* on a separate line
#300: FILE: hw/9pfs/9p.c:805:
+/** @brief Quick mapping host inode nr -> guest inode nr.
ERROR: spaces required around that '-' (ctx:VxV)
#348: FILE: hw/9pfs/9p.c:849:
+ .ino_prefix = (uint16_t) (stbuf->st_ino >> (64-ino_hash_bits))
^
WARNING: Block comments use a leading /* on a separate line
#429: FILE: hw/9pfs/9p.h:244:
+/** @brief Unique affix of variable length.
ERROR: line over 90 characters
#442: FILE: hw/9pfs/9p.h:257:
+ int bits; /* Lenght of the affix, that is how many (of the lowest) bits of @c value must be used for appending/prepending this affix to its final resulting, unique number. */
ERROR: line over 90 characters
#448: FILE: hw/9pfs/9p.h:263:
+ int prefix_bits; /* How many (high) bits of the original inode number shall be used for hashing. */
total: 8 errors, 9 warnings, 386 lines checked
Patch 4/4 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/cover.1566503584.git.qemu_oss@crudebyte.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
On Thu, 22 Aug 2019 15:18:54 -0700 (PDT)
no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/cover.1566503584.git.qemu_oss@crudebyte.com/
>
>
>
> Hi,
>
> This series seems to have some coding style problems. See output below for
> more information:
>
> Type: series
> Subject: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions
> Message-id: cover.1566503584.git.qemu_oss@crudebyte.com
>
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> git rev-parse base > /dev/null || exit 0
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> git config --local diff.algorithm histogram
> ./scripts/checkpatch.pl --mailback base..
> === TEST SCRIPT END ===
>
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> From https://github.com/patchew-project/qemu
> * [new tag] patchew/cover.1566503584.git.qemu_oss@crudebyte.com -> patchew/cover.1566503584.git.qemu_oss@crudebyte.com
> Submodule 'capstone' (https://git.qemu.org/git/capstone.git) registered for path 'capstone'
> Submodule 'dtc' (https://git.qemu.org/git/dtc.git) registered for path 'dtc'
> Submodule 'roms/QemuMacDrivers' (https://git.qemu.org/git/QemuMacDrivers.git) registered for path 'roms/QemuMacDrivers'
> Submodule 'roms/SLOF' (https://git.qemu.org/git/SLOF.git) registered for path 'roms/SLOF'
> Submodule 'roms/edk2' (https://git.qemu.org/git/edk2.git) registered for path 'roms/edk2'
> Submodule 'roms/ipxe' (https://git.qemu.org/git/ipxe.git) registered for path 'roms/ipxe'
> Submodule 'roms/openbios' (https://git.qemu.org/git/openbios.git) registered for path 'roms/openbios'
> Submodule 'roms/openhackware' (https://git.qemu.org/git/openhackware.git) registered for path 'roms/openhackware'
> Submodule 'roms/opensbi' (https://git.qemu.org/git/opensbi.git) registered for path 'roms/opensbi'
> Submodule 'roms/qemu-palcode' (https://git.qemu.org/git/qemu-palcode.git) registered for path 'roms/qemu-palcode'
> Submodule 'roms/seabios' (https://git.qemu.org/git/seabios.git/) registered for path 'roms/seabios'
> Submodule 'roms/seabios-hppa' (https://git.qemu.org/git/seabios-hppa.git) registered for path 'roms/seabios-hppa'
> Submodule 'roms/sgabios' (https://git.qemu.org/git/sgabios.git) registered for path 'roms/sgabios'
> Submodule 'roms/skiboot' (https://git.qemu.org/git/skiboot.git) registered for path 'roms/skiboot'
> Submodule 'roms/u-boot' (https://git.qemu.org/git/u-boot.git) registered for path 'roms/u-boot'
> Submodule 'roms/u-boot-sam460ex' (https://git.qemu.org/git/u-boot-sam460ex.git) registered for path 'roms/u-boot-sam460ex'
> Submodule 'slirp' (https://git.qemu.org/git/libslirp.git) registered for path 'slirp'
> Submodule 'tests/fp/berkeley-softfloat-3' (https://git.qemu.org/git/berkeley-softfloat-3.git) registered for path 'tests/fp/berkeley-softfloat-3'
> Submodule 'tests/fp/berkeley-testfloat-3' (https://git.qemu.org/git/berkeley-testfloat-3.git) registered for path 'tests/fp/berkeley-testfloat-3'
> Submodule 'ui/keycodemapdb' (https://git.qemu.org/git/keycodemapdb.git) registered for path 'ui/keycodemapdb'
> Cloning into 'capstone'...
> Submodule path 'capstone': checked out '22ead3e0bfdb87516656453336160e0a37b066bf'
> Cloning into 'dtc'...
> Submodule path 'dtc': checked out '88f18909db731a627456f26d779445f84e449536'
> Cloning into 'roms/QemuMacDrivers'...
> Submodule path 'roms/QemuMacDrivers': checked out '90c488d5f4a407342247b9ea869df1c2d9c8e266'
> Cloning into 'roms/SLOF'...
> Submodule path 'roms/SLOF': checked out '7bfe584e321946771692711ff83ad2b5850daca7'
> Cloning into 'roms/edk2'...
> Submodule path 'roms/edk2': checked out '20d2e5a125e34fc8501026613a71549b2a1a3e54'
> Submodule 'SoftFloat' (https://github.com/ucb-bar/berkeley-softfloat-3.git) registered for path 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3'
> Submodule 'CryptoPkg/Library/OpensslLib/openssl' (https://github.com/openssl/openssl) registered for path 'CryptoPkg/Library/OpensslLib/openssl'
> Cloning into 'ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3'...
> Submodule path 'roms/edk2/ArmPkg/Library/ArmSoftFloatLib/berkeley-softfloat-3': checked out 'b64af41c3276f97f0e181920400ee056b9c88037'
> Cloning into 'CryptoPkg/Library/OpensslLib/openssl'...
> Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl': checked out '50eaac9f3337667259de725451f201e784599687'
> Submodule 'boringssl' (https://boringssl.googlesource.com/boringssl) registered for path 'boringssl'
> Submodule 'krb5' (https://github.com/krb5/krb5) registered for path 'krb5'
> Submodule 'pyca.cryptography' (https://github.com/pyca/cryptography.git) registered for path 'pyca-cryptography'
> Cloning into 'boringssl'...
> Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/boringssl': checked out '2070f8ad9151dc8f3a73bffaa146b5e6937a583f'
> Cloning into 'krb5'...
> Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/krb5': checked out 'b9ad6c49505c96a088326b62a52568e3484f2168'
> Cloning into 'pyca-cryptography'...
> Submodule path 'roms/edk2/CryptoPkg/Library/OpensslLib/openssl/pyca-cryptography': checked out '09403100de2f6f1cdd0d484dcb8e620f1c335c8f'
> Cloning into 'roms/ipxe'...
> Submodule path 'roms/ipxe': checked out 'de4565cbe76ea9f7913a01f331be3ee901bb6e17'
> Cloning into 'roms/openbios'...
> Submodule path 'roms/openbios': checked out 'c79e0ecb84f4f1ee3f73f521622e264edd1bf174'
> Cloning into 'roms/openhackware'...
> Submodule path 'roms/openhackware': checked out 'c559da7c8eec5e45ef1f67978827af6f0b9546f5'
> Cloning into 'roms/opensbi'...
> Submodule path 'roms/opensbi': checked out 'ce228ee0919deb9957192d723eecc8aaae2697c6'
> Cloning into 'roms/qemu-palcode'...
> Submodule path 'roms/qemu-palcode': checked out 'bf0e13698872450164fa7040da36a95d2d4b326f'
> Cloning into 'roms/seabios'...
> Submodule path 'roms/seabios': checked out 'a5cab58e9a3fb6e168aba919c5669bea406573b4'
> Cloning into 'roms/seabios-hppa'...
> Submodule path 'roms/seabios-hppa': checked out '0f4fe84658165e96ce35870fd19fc634e182e77b'
> Cloning into 'roms/sgabios'...
> Submodule path 'roms/sgabios': checked out 'cbaee52287e5f32373181cff50a00b6c4ac9015a'
> Cloning into 'roms/skiboot'...
> Submodule path 'roms/skiboot': checked out '261ca8e779e5138869a45f174caa49be6a274501'
> Cloning into 'roms/u-boot'...
> Submodule path 'roms/u-boot': checked out 'd3689267f92c5956e09cc7d1baa4700141662bff'
> Cloning into 'roms/u-boot-sam460ex'...
> Submodule path 'roms/u-boot-sam460ex': checked out '60b3916f33e617a815973c5a6df77055b2e3a588'
> Cloning into 'slirp'...
> Submodule path 'slirp': checked out '126c04acbabd7ad32c2b018fe10dfac2a3bc1210'
> Cloning into 'tests/fp/berkeley-softfloat-3'...
> Submodule path 'tests/fp/berkeley-softfloat-3': checked out 'b64af41c3276f97f0e181920400ee056b9c88037'
> Cloning into 'tests/fp/berkeley-testfloat-3'...
> Submodule path 'tests/fp/berkeley-testfloat-3': checked out '5a59dcec19327396a011a17fd924aed4fec416b3'
> Cloning into 'ui/keycodemapdb'...
> Submodule path 'ui/keycodemapdb': checked out '6b3d716e2b6472eb7189d3220552280ef3d832ce'
> Switched to a new branch 'test'
> 28d6fb8 9p: Use variable length suffixes for inode remapping
> 23fc4f6 9p: stat_to_qid: implement slow path
> f227708 9p: Added virtfs option 'multidevs=remap|forbid|warn'
> bb69de6 9p: Treat multiple devices on one export as an error
>
> === OUTPUT BEGIN ===
> 1/4 Checking commit bb69de63f788 (9p: Treat multiple devices on one export as an error)
> ERROR: Author email address is mangled by the mailing list
> #2:
> Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>
>
This is problematic since it ends up in the Author: field in git. Please find
a way to fix that.
Other warnings/errors should also be fixed but they look trivial.
> total: 1 errors, 0 warnings, 175 lines checked
>
> Patch 1/4 has style problems, please review. If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
> 2/4 Checking commit f227708a87da (9p: Added virtfs option 'multidevs=remap|forbid|warn')
> ERROR: Author email address is mangled by the mailing list
> #2:
> Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>
>
> WARNING: Block comments use a leading /* on a separate line
> #167: FILE: hw/9pfs/9p.c:600:
> +/* stat_to_qid needs to map inode number (64 bits) and device id (32 bits)
>
> ERROR: line over 90 characters
> #467: FILE: vl.c:3338:
> + const char *writeout, *sock_fd, *socket, *path, *security_model, *multidevs;
>
> total: 2 errors, 1 warnings, 394 lines checked
>
> Patch 2/4 has style problems, please review. If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
> 3/4 Checking commit 23fc4f6d5258 (9p: stat_to_qid: implement slow path)
> ERROR: Author email address is mangled by the mailing list
> #2:
> Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>
>
> WARNING: line over 80 characters
> #79: FILE: hw/9pfs/9p.c:622:
> + qht_init(&pdu->s->qpf_table, qpf_lookup_func, 1 << 16, QHT_MODE_AUTO_RESIZE);
>
> total: 1 errors, 1 warnings, 142 lines checked
>
> Patch 3/4 has style problems, please review. If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
> 4/4 Checking commit 28d6fb899725 (9p: Use variable length suffixes for inode remapping)
> ERROR: Author email address is mangled by the mailing list
> #2:
> Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>
>
> ERROR: space prohibited after that open parenthesis '('
> #92: FILE: hw/9pfs/9p.c:586:
> + return ((uint64_t)mirror8bit( value & 0xff) << 56) |
>
> ERROR: space prohibited before that close parenthesis ')'
> #98: FILE: hw/9pfs/9p.c:592:
> + ((uint64_t)mirror8bit((value >> 48) & 0xff) << 8 ) |
>
> ERROR: space prohibited before that close parenthesis ')'
> #99: FILE: hw/9pfs/9p.c:593:
> + ((uint64_t)mirror8bit((value >> 56) & 0xff) ) ;
>
> WARNING: Block comments use a leading /* on a separate line
> #102: FILE: hw/9pfs/9p.c:596:
> +/** @brief Parameter k for the Exponential Golomb algorihm to be used.
>
> WARNING: Block comments use a leading /* on a separate line
> #121: FILE: hw/9pfs/9p.c:615:
> +/** @brief Exponential Golomb algorithm for arbitrary k (including k=0).
>
> WARNING: Block comments use a leading /* on a separate line
> #148: FILE: hw/9pfs/9p.c:642:
> +/** @brief Converts a suffix into a prefix, or a prefix into a suffix.
>
> ERROR: "foo* bar" should be "foo *bar"
> #158: FILE: hw/9pfs/9p.c:652:
> +static VariLenAffix invertAffix(const VariLenAffix* affix)
>
> WARNING: line over 80 characters
> #161: FILE: hw/9pfs/9p.c:655:
> + .type = (affix->type == AffixType_Suffix) ? AffixType_Prefix : AffixType_Suffix,
>
> WARNING: line over 80 characters
> #162: FILE: hw/9pfs/9p.c:656:
> + .value = mirror64bit(affix->value) >> ((sizeof(affix->value) * 8) - affix->bits),
>
> WARNING: Block comments use a leading /* on a separate line
> #167: FILE: hw/9pfs/9p.c:661:
> +/** @brief Generates suffix numbers with "suffix-free" property.
>
> WARNING: Block comments use a leading /* on a separate line
> #246: FILE: hw/9pfs/9p.c:752:
> +/** @brief Slow / full mapping host inode nr -> guest inode nr.
>
> WARNING: Block comments use a leading /* on a separate line
> #300: FILE: hw/9pfs/9p.c:805:
> +/** @brief Quick mapping host inode nr -> guest inode nr.
>
> ERROR: spaces required around that '-' (ctx:VxV)
> #348: FILE: hw/9pfs/9p.c:849:
> + .ino_prefix = (uint16_t) (stbuf->st_ino >> (64-ino_hash_bits))
> ^
>
> WARNING: Block comments use a leading /* on a separate line
> #429: FILE: hw/9pfs/9p.h:244:
> +/** @brief Unique affix of variable length.
>
> ERROR: line over 90 characters
> #442: FILE: hw/9pfs/9p.h:257:
> + int bits; /* Lenght of the affix, that is how many (of the lowest) bits of @c value must be used for appending/prepending this affix to its final resulting, unique number. */
>
> ERROR: line over 90 characters
> #448: FILE: hw/9pfs/9p.h:263:
> + int prefix_bits; /* How many (high) bits of the original inode number shall be used for hashing. */
>
> total: 8 errors, 9 warnings, 386 lines checked
>
> Patch 4/4 has style problems, please review. If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
> === OUTPUT END ===
>
> Test command exited with code: 1
>
>
> The full log is available at
> http://patchew.org/logs/cover.1566503584.git.qemu_oss@crudebyte.com/testing.checkpatch/?type=message.
> ---
> Email generated automatically by Patchew [https://patchew.org/].
> Please send your feedback to patchew-devel@redhat.com
On Donnerstag, 29. August 2019 19:02:34 CEST Greg Kurz wrote:
> On Thu, 22 Aug 2019 15:18:54 -0700 (PDT)
>
> no-reply@patchew.org wrote:
> > Patchew URL:
> > https://patchew.org/QEMU/cover.1566503584.git.qemu_oss@crudebyte.com/
> >
> >
> >
> > Hi,
> >
> > This series seems to have some coding style problems. See output below for
> > more information:
[snip]
> >
> > === OUTPUT BEGIN ===
> > 1/4 Checking commit bb69de63f788 (9p: Treat multiple devices on one export
> > as an error) ERROR: Author email address is mangled by the mailing list
> > #2:
> > Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>
>
> This is problematic since it ends up in the Author: field in git. Please
> find a way to fix that.
Like in which way do you imagine that? And where is the actual practical
problem? I mean every patch still has my signed-off-by tag with the correct
email address ending up in git history.
The cause for this issue is that the domain is configured to require DKIM
signatures for all outgoing emails. That's why mailman replaces my address by
"Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>" placeholder
since it could not provide a valid signature.
There were good reasons for enabling DKIM and it is a good thing for all
domains in general, since that ensures that (i.e. foreign) email addresses
cannot be used as sender address if the actual sender is not authorized for
sending emails with that address.
What I changed in the meantime though is that you should get all my patches
directly to your personal address, not only from the list. Or did you receive
v6 again just from the list?
> Other warnings/errors should also be fixed but they look trivial.
Yeah, they are trivial. *But* there is one thing ...
> > Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>
> >
> > ERROR: space prohibited after that open parenthesis '('
> > #92: FILE: hw/9pfs/9p.c:586:
> > + return ((uint64_t)mirror8bit( value & 0xff) << 56) |
> >
> > ERROR: space prohibited before that close parenthesis ')'
> > #98: FILE: hw/9pfs/9p.c:592:
> > + ((uint64_t)mirror8bit((value >> 48) & 0xff) << 8 ) |
> >
> > ERROR: space prohibited before that close parenthesis ')'
> > #99: FILE: hw/9pfs/9p.c:593:
> > + ((uint64_t)mirror8bit((value >> 56) & 0xff) ) ;
... I would like to ignore this specific bot whining, because that particular
function looks much more readable the way it is (in that patch) right now.
> > WARNING: Block comments use a leading /* on a separate line
> > #102: FILE: hw/9pfs/9p.c:596:
> > +/** @brief Parameter k for the Exponential Golomb algorihm to be used.
> >
> > WARNING: Block comments use a leading /* on a separate line
> > #121: FILE: hw/9pfs/9p.c:615:
> > +/** @brief Exponential Golomb algorithm for arbitrary k (including k=0).
> >
> > WARNING: Block comments use a leading /* on a separate line
> > #148: FILE: hw/9pfs/9p.c:642:
> > +/** @brief Converts a suffix into a prefix, or a prefix into a suffix.
On Sun, 01 Sep 2019 21:28:45 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> On Donnerstag, 29. August 2019 19:02:34 CEST Greg Kurz wrote:
> > On Thu, 22 Aug 2019 15:18:54 -0700 (PDT)
> >
> > no-reply@patchew.org wrote:
> > > Patchew URL:
> > > https://patchew.org/QEMU/cover.1566503584.git.qemu_oss@crudebyte.com/
> > >
> > >
> > >
> > > Hi,
> > >
> > > This series seems to have some coding style problems. See output below for
> > > more information:
> [snip]
> > >
> > > === OUTPUT BEGIN ===
> > > 1/4 Checking commit bb69de63f788 (9p: Treat multiple devices on one export
> > > as an error) ERROR: Author email address is mangled by the mailing list
> > > #2:
> > > Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>
> >
> > This is problematic since it ends up in the Author: field in git. Please
> > find a way to fix that.
>
> Like in which way do you imagine that? And where is the actual practical
> problem? I mean every patch still has my signed-off-by tag with the correct
> email address ending up in git history.
>
Yes, this only breaks Author: if the patch is applied from the list.
> The cause for this issue is that the domain is configured to require DKIM
> signatures for all outgoing emails. That's why mailman replaces my address by
> "Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>" placeholder
> since it could not provide a valid signature.
>
> There were good reasons for enabling DKIM and it is a good thing for all
> domains in general, since that ensures that (i.e. foreign) email addresses
> cannot be used as sender address if the actual sender is not authorized for
> sending emails with that address.
>
Don't know much about DKIM but google seems to confirm what you say. So,
this means that patchew will complain each time you post if we can't find
a proper way to address that... :-\
> What I changed in the meantime though is that you should get all my patches
> directly to your personal address, not only from the list. Or did you receive
> v6 again just from the list?
>
I've received the patches in my mailbox but I prefer to use the patchwork's
pwclient CLI to apply patches... and patchwork captures the patches from
the list, so I end up having to patch the authorship manually anyway.
How are you sending patches ? With git send-email ? If so, maybe you can pass
something like --from='"Christian Schoenebeck" <qemu_oss@crudebyte.com>'.
Since this is a different string, git will assume you're sending someone else's
patch : it will automatically add an extra From: made out of the commit Author
as recorded in the git tree.
> > Other warnings/errors should also be fixed but they look trivial.
>
> Yeah, they are trivial. *But* there is one thing ...
>
> > > Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>
> > >
> > > ERROR: space prohibited after that open parenthesis '('
> > > #92: FILE: hw/9pfs/9p.c:586:
> > > + return ((uint64_t)mirror8bit( value & 0xff) << 56) |
> > >
> > > ERROR: space prohibited before that close parenthesis ')'
> > > #98: FILE: hw/9pfs/9p.c:592:
> > > + ((uint64_t)mirror8bit((value >> 48) & 0xff) << 8 ) |
> > >
> > > ERROR: space prohibited before that close parenthesis ')'
> > > #99: FILE: hw/9pfs/9p.c:593:
> > > + ((uint64_t)mirror8bit((value >> 56) & 0xff) ) ;
>
> ... I would like to ignore this specific bot whining, because that particular
> function looks much more readable the way it is (in that patch) right now.
Prettier certainly but...
/* Same as mirror8bit() just for a 64 bit data type instead for a byte. */
static inline uint64_t mirror64bit(uint64_t value)
{
return ((uint64_t)mirror8bit(value & 0xff) << 56) |
((uint64_t)mirror8bit((value >> 8) & 0xff) << 48) |
((uint64_t)mirror8bit((value >> 16) & 0xff) << 40) |
((uint64_t)mirror8bit((value >> 24) & 0xff) << 32) |
((uint64_t)mirror8bit((value >> 32) & 0xff) << 24) |
((uint64_t)mirror8bit((value >> 40) & 0xff) << 16) |
((uint64_t)mirror8bit((value >> 48) & 0xff) << 8) |
((uint64_t)mirror8bit((value >> 56) & 0xff));
}
... is readable enough IMHO and makes checkpatch happy :)
>
> > > WARNING: Block comments use a leading /* on a separate line
> > > #102: FILE: hw/9pfs/9p.c:596:
> > > +/** @brief Parameter k for the Exponential Golomb algorihm to be used.
> > >
> > > WARNING: Block comments use a leading /* on a separate line
> > > #121: FILE: hw/9pfs/9p.c:615:
> > > +/** @brief Exponential Golomb algorithm for arbitrary k (including k=0).
> > >
> > > WARNING: Block comments use a leading /* on a separate line
> > > #148: FILE: hw/9pfs/9p.c:642:
> > > +/** @brief Converts a suffix into a prefix, or a prefix into a suffix.
On Montag, 2. September 2019 17:34:32 CEST Greg Kurz wrote:
> On Sun, 01 Sep 2019 21:28:45 +0200
>
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > On Donnerstag, 29. August 2019 19:02:34 CEST Greg Kurz wrote:
> > > On Thu, 22 Aug 2019 15:18:54 -0700 (PDT)
> > >
> > > no-reply@patchew.org wrote:
> > > > Patchew URL:
> > > > https://patchew.org/QEMU/cover.1566503584.git.qemu_oss@crudebyte.com/
> > > >
> > > >
> > > >
> > > > Hi,
> > > >
> > > > This series seems to have some coding style problems. See output below
> > > > for
> >
> > > > more information:
> > [snip]
> >
> > > > === OUTPUT BEGIN ===
> > > > 1/4 Checking commit bb69de63f788 (9p: Treat multiple devices on one
> > > > export
> > > > as an error) ERROR: Author email address is mangled by the mailing
> > > > list
> > > > #2:
> > > > Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>
> > >
> > > This is problematic since it ends up in the Author: field in git. Please
> > > find a way to fix that.
> >
> > Like in which way do you imagine that? And where is the actual practical
> > problem? I mean every patch still has my signed-off-by tag with the
> > correct
> > email address ending up in git history.
>
> Yes, this only breaks Author: if the patch is applied from the list.
>
> > The cause for this issue is that the domain is configured to require DKIM
> > signatures for all outgoing emails. That's why mailman replaces my address
> > by "Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>"
> > placeholder since it could not provide a valid signature.
> >
> > There were good reasons for enabling DKIM and it is a good thing for all
> > domains in general, since that ensures that (i.e. foreign) email addresses
> > cannot be used as sender address if the actual sender is not authorized
> > for
> > sending emails with that address.
>
> Don't know much about DKIM but google seems to confirm what you say.
When you view the source of my emails you'll always see a "DKIM-Signature:"
field in the email header, which is a signature of the email's body and the
specific email header fields listed in the "DKIM-Signature:" block, so the
original server can choose by itself which header fields to include ("h=...")
for signing, but the standard requires the From: field must always be
included.
A receiving server then obtains the public key from the domain's DNS records
and checks if the DKIM signature of the email was correct:
https://en.wikipedia.org/wiki/DomainKeys_Identified_Mail
Additionally the receiving server obtains the so called "DMARC" entry from the
domain's DNS records. The "DMARC" DNS entry contains the domain's general
policies how receiving email servers shall handle verification of emails of
this domain. For instance the DMARC entry may list SMTP servers (e.g. IPs, DNS
names) eligble to send emails on behalf of the domain at all, and it defines
what receiving email servers shall do with emails which were identified of not
being from an eligible source (e.g. sender IP not listed in DMARC entry or
missing or wrong DKIM signature in email header). For instance if the policy
is "quarantine" in the DMARC entry then receiving servers are advised to
simply drop invalid emails: https://en.wikipedia.org/wiki/DMARC
> So, this means that patchew will complain each time you post if we can't
> find a proper way to address that... :-\
Well, mailman is handling this correctly. It replaces the "From:" field with a
placeholder and instead adds my actual email address as "Reply-To:" field.
That's the common way to handle this on mailing lists, as also mentioned here:
https://en.wikipedia.org/wiki/DMARC#From:_rewriting
So IMO patchew should automatically use the value of "Reply-To:" in that case
as author of patches instead.
Reducing security cannot be the solution.
> > What I changed in the meantime though is that you should get all my
> > patches
> > directly to your personal address, not only from the list. Or did you
> > receive v6 again just from the list?
>
> I've received the patches in my mailbox but I prefer to use the patchwork's
> pwclient CLI to apply patches... and patchwork captures the patches from
> the list, so I end up having to patch the authorship manually anyway.
>
> How are you sending patches ? With git send-email ? If so, maybe you can
> pass something like --from='"Christian Schoenebeck"
> <qemu_oss@crudebyte.com>'. Since this is a different string, git will
> assume you're sending someone else's patch : it will automatically add an
> extra From: made out of the commit Author as recorded in the git tree.
I use "git format-patch ..." to dump the invidiual emails as raw email sources
and then I'll send those raw emails from the command line. So I have even more
control of what is exactly sent out and could of course also add custom email
header fields if required, if that would solve the situation somehow, i.e.
manually as first test and later in automated way. That's not the issue here.
The problem however is that there is probably not any header field I could add
that would solve the problem. Because I guess patchew is really just taking
the first "From:" field as author, period.
I think the source files are available, so I could check that.
> > > Other warnings/errors should also be fixed but they look trivial.
> >
> > Yeah, they are trivial. *But* there is one thing ...
> >
> > > > Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>
> > > >
> > > > ERROR: space prohibited after that open parenthesis '('
> > > > #92: FILE: hw/9pfs/9p.c:586:
> > > > + return ((uint64_t)mirror8bit( value & 0xff) << 56) |
> > > >
> > > > ERROR: space prohibited before that close parenthesis ')'
> > > > #98: FILE: hw/9pfs/9p.c:592:
> > > > + ((uint64_t)mirror8bit((value >> 48) & 0xff) << 8 ) |
> > > >
> > > > ERROR: space prohibited before that close parenthesis ')'
> > > > #99: FILE: hw/9pfs/9p.c:593:
> > > > + ((uint64_t)mirror8bit((value >> 56) & 0xff) ) ;
> >
> > ... I would like to ignore this specific bot whining, because that
> > particular function looks much more readable the way it is (in that
> > patch) right now.
> Prettier certainly but...
>
> /* Same as mirror8bit() just for a 64 bit data type instead for a byte. */
> static inline uint64_t mirror64bit(uint64_t value)
> {
> return ((uint64_t)mirror8bit(value & 0xff) << 56) |
> ((uint64_t)mirror8bit((value >> 8) & 0xff) << 48) |
> ((uint64_t)mirror8bit((value >> 16) & 0xff) << 40) |
> ((uint64_t)mirror8bit((value >> 24) & 0xff) << 32) |
> ((uint64_t)mirror8bit((value >> 32) & 0xff) << 24) |
> ((uint64_t)mirror8bit((value >> 40) & 0xff) << 16) |
> ((uint64_t)mirror8bit((value >> 48) & 0xff) << 8) |
> ((uint64_t)mirror8bit((value >> 56) & 0xff));
> }
>
> ... is readable enough IMHO and makes checkpatch happy :)
Well, okay :)
On 9/2/19 5:29 PM, Christian Schoenebeck via Qemu-devel wrote: >>>>> === OUTPUT BEGIN === >>>>> 1/4 Checking commit bb69de63f788 (9p: Treat multiple devices on one >>>>> export >>>>> as an error) ERROR: Author email address is mangled by the mailing >>>>> list >>>>> #2: >>>>> Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org> >>>> >>>> This is problematic since it ends up in the Author: field in git. Please >>>> find a way to fix that. >>> >>> Like in which way do you imagine that? And where is the actual practical >>> problem? I mean every patch still has my signed-off-by tag with the >>> correct >>> email address ending up in git history. >> >> Yes, this only breaks Author: if the patch is applied from the list. Except that many maintainers DO apply mail from the list (thanks to 'git am'). Fixing patchew to unmunge things is an appealing idea, but would not fix the problem for maintainers not cloning from patchew, so even if patchew avoids the problem locally, it should still continue to warn about the problem. >> >>> The cause for this issue is that the domain is configured to require DKIM >>> signatures for all outgoing emails. That's why mailman replaces my address >>> by "Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>" >>> placeholder since it could not provide a valid signature. And when you know that mailman is going to munge your address, the fix is to configure git to output 'From: correct name <correct@example.com>' as the first line of the BODY of the message, since 'git am' favors the unmunged From: from the body over the munged From: from the headers. > >> So, this means that patchew will complain each time you post if we can't >> find a proper way to address that... :-\ > > Well, mailman is handling this correctly. It replaces the "From:" field with a > placeholder and instead adds my actual email address as "Reply-To:" field. > That's the common way to handle this on mailing lists, as also mentioned here: > https://en.wikipedia.org/wiki/DMARC#From:_rewriting > > So IMO patchew should automatically use the value of "Reply-To:" in that case > as author of patches instead. > > Reducing security cannot be the solution. No, there's no need to reduce security. Just change your local git configuration to produce a 'From:' line in the commit body.. >> How are you sending patches ? With git send-email ? If so, maybe you can >> pass something like --from='"Christian Schoenebeck" >> <qemu_oss@crudebyte.com>'. Since this is a different string, git will >> assume you're sending someone else's patch : it will automatically add an >> extra From: made out of the commit Author as recorded in the git tree. I think it is probably as simple as a 'git config' command to tell git to always put a 'From:' in the body of self-authored patches when using git format-patch; however, as I don't suffer from munged emails, I haven't actually tested what that setting would be. > > I use "git format-patch ..." to dump the invidiual emails as raw email sources > and then I'll send those raw emails from the command line. So I have even more > control of what is exactly sent out and could of course also add custom email > header fields if required, if that would solve the situation somehow, i.e. > manually as first test and later in automated way. That's not the issue here. Working around the problem does not require munging email headers, but adding a line to the email body. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Dienstag, 3. September 2019 21:38:15 CEST Eric Blake wrote: > On 9/2/19 5:29 PM, Christian Schoenebeck via Qemu-devel wrote: > >>>>> === OUTPUT BEGIN === > >>>>> 1/4 Checking commit bb69de63f788 (9p: Treat multiple devices on one > >>>>> export > >>>>> as an error) ERROR: Author email address is mangled by the mailing > >>>>> list > >>>>> #2: > >>>>> Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org> > >>>> > >>>> This is problematic since it ends up in the Author: field in git. > >>>> Please > >>>> find a way to fix that. > >>> > >>> Like in which way do you imagine that? And where is the actual practical > >>> problem? I mean every patch still has my signed-off-by tag with the > >>> correct > >>> email address ending up in git history. > >> > >> Yes, this only breaks Author: if the patch is applied from the list. > > Except that many maintainers DO apply mail from the list (thanks to 'git > am'). Fixing patchew to unmunge things is an appealing idea, but would > not fix the problem for maintainers not cloning from patchew, so even if > patchew avoids the problem locally, it should still continue to warn > about the problem. > > >>> The cause for this issue is that the domain is configured to require > >>> DKIM > >>> signatures for all outgoing emails. That's why mailman replaces my > >>> address > >>> by "Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>" > >>> placeholder since it could not provide a valid signature. > > And when you know that mailman is going to munge your address, the fix > is to configure git to output 'From: correct name <correct@example.com>' > as the first line of the BODY of the message, since 'git am' favors the > unmunged From: from the body over the munged From: from the headers. Ah I see, I will try that with the next 9p patch set round (probably tomorrow). Thanks for the hint! I actually had a quick glimpse on the patchew sources yesterday to see if there was some undocumented alternative header like "X-git-author:" or something like that, but could not find one. > > Well, mailman is handling this correctly. It replaces the "From:" field > > with a placeholder and instead adds my actual email address as > > "Reply-To:" field. That's the common way to handle this on mailing lists, > > as also mentioned here: > > https://en.wikipedia.org/wiki/DMARC#From:_rewriting > > > > So IMO patchew should automatically use the value of "Reply-To:" in that > > case as author of patches instead. > > > > Reducing security cannot be the solution. > > No, there's no need to reduce security. Just change your local git > configuration to produce a 'From:' line in the commit body.. Got it. :) > >> How are you sending patches ? With git send-email ? If so, maybe you can > >> pass something like --from='"Christian Schoenebeck" > >> <qemu_oss@crudebyte.com>'. Since this is a different string, git will > >> assume you're sending someone else's patch : it will automatically add an > >> extra From: made out of the commit Author as recorded in the git tree. > > I think it is probably as simple as a 'git config' command to tell git > to always put a 'From:' in the body of self-authored patches when using > git format-patch; however, as I don't suffer from munged emails, I > haven't actually tested what that setting would be. > > > I use "git format-patch ..." to dump the invidiual emails as raw email > > sources and then I'll send those raw emails from the command line. So I > > have even more control of what is exactly sent out and could of course > > also add custom email header fields if required, if that would solve the > > situation somehow, i.e. manually as first test and later in automated > > way. That's not the issue here. > Working around the problem does not require munging email headers, but > adding a line to the email body.
On Mittwoch, 4. September 2019 15:02:30 CEST Christian Schoenebeck wrote: > > > Well, mailman is handling this correctly. It replaces the "From:" field > > > with a placeholder and instead adds my actual email address as > > > "Reply-To:" field. That's the common way to handle this on mailing > > > lists, > > > as also mentioned here: > > > https://en.wikipedia.org/wiki/DMARC#From:_rewriting > > > > > > So IMO patchew should automatically use the value of "Reply-To:" in that > > > case as author of patches instead. > > > > > > Reducing security cannot be the solution. > > > > No, there's no need to reduce security. Just change your local git > > configuration to produce a 'From:' line in the commit body.. > > Got it. :) > > > >> How are you sending patches ? With git send-email ? If so, maybe you > > >> can > > >> pass something like --from='"Christian Schoenebeck" > > >> <qemu_oss@crudebyte.com>'. Since this is a different string, git will > > >> assume you're sending someone else's patch : it will automatically add > > >> an > > >> extra From: made out of the commit Author as recorded in the git tree. > > > > I think it is probably as simple as a 'git config' command to tell git > > to always put a 'From:' in the body of self-authored patches when using > > git format-patch; however, as I don't suffer from munged emails, I > > haven't actually tested what that setting would be. Well, I tried that Eric. The expected solution would be enabling this git setting: git config [--global] format.from true https://git-scm.com/docs/git-config#Documentation/git-config.txt-formatfrom But as you can already read from the manual, the overall behaviour of git regarding a separate "From:" line in the email body was intended solely for the use case sender != author. So in practice (at least in my git version) git always makes a raw string comparison between sender (name and email) string and author string and only adds the separate From: line to the body if they differ. Hence also "git format-patch --from=" only works here if you use a different author string (name and email) there, otherwise on a perfect string match it is simply ignored and you end up with only one "From:" in the email header. So eventually I added one extra character in my name for now and removed it manually in the dumped emails subsequently (see today's "[PATCH v7 0/3] 9p: Fix file ID collisions"). Besides that direct string comparison restriction; git also seems to have a bug here. Because even if you have sender != author, then git falsely uses author as sender of the cover letter, whereas the emails of the individual patches are encoded correctly. Best regards, Christian Schoenebeck
On Thu, 05 Sep 2019 14:25:13 +0200 Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > On Mittwoch, 4. September 2019 15:02:30 CEST Christian Schoenebeck wrote: > > > > Well, mailman is handling this correctly. It replaces the "From:" field > > > > with a placeholder and instead adds my actual email address as > > > > "Reply-To:" field. That's the common way to handle this on mailing > > > > lists, > > > > as also mentioned here: > > > > https://en.wikipedia.org/wiki/DMARC#From:_rewriting > > > > > > > > So IMO patchew should automatically use the value of "Reply-To:" in that > > > > case as author of patches instead. > > > > > > > > Reducing security cannot be the solution. > > > > > > No, there's no need to reduce security. Just change your local git > > > configuration to produce a 'From:' line in the commit body.. > > > > Got it. :) > > > > > >> How are you sending patches ? With git send-email ? If so, maybe you > > > >> can > > > >> pass something like --from='"Christian Schoenebeck" > > > >> <qemu_oss@crudebyte.com>'. Since this is a different string, git will > > > >> assume you're sending someone else's patch : it will automatically add > > > >> an > > > >> extra From: made out of the commit Author as recorded in the git tree. > > > > > > I think it is probably as simple as a 'git config' command to tell git > > > to always put a 'From:' in the body of self-authored patches when using > > > git format-patch; however, as I don't suffer from munged emails, I > > > haven't actually tested what that setting would be. > > Well, I tried that Eric. The expected solution would be enabling this git > setting: > > git config [--global] format.from true > https://git-scm.com/docs/git-config#Documentation/git-config.txt-formatfrom > > But as you can already read from the manual, the overall behaviour of git > regarding a separate "From:" line in the email body was intended solely for > the use case sender != author. So in practice (at least in my git version) git > always makes a raw string comparison between sender (name and email) string > and author string and only adds the separate From: line to the body if they > differ. > > Hence also "git format-patch --from=" only works here if you use a different > author string (name and email) there, otherwise on a perfect string match it > is simply ignored and you end up with only one "From:" in the email header. > > So eventually I added one extra character in my name for now and removed it > manually in the dumped emails subsequently (see today's > "[PATCH v7 0/3] 9p: Fix file ID collisions"). > Hence my proposal in some other mail to pass a different string to git send-email, but I guess this also works for git format-patch. eg, adding double quotes around your "firstname name" --from='"Christian Schoenebeck" <qemu_oss@crudebyte.com>' > Besides that direct string comparison restriction; git also seems to have a > bug here. Because even if you have sender != author, then git falsely uses > author as sender of the cover letter, whereas the emails of the individual > patches are encoded correctly. > > Best regards, > Christian Schoenebeck > >
On Donnerstag, 5. September 2019 14:59:31 CEST Greg Kurz wrote: > On Thu, 05 Sep 2019 14:25:13 +0200 > > Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > > On Mittwoch, 4. September 2019 15:02:30 CEST Christian Schoenebeck wrote: > > > > > Well, mailman is handling this correctly. It replaces the "From:" > > > > > field > > > > > with a placeholder and instead adds my actual email address as > > > > > "Reply-To:" field. That's the common way to handle this on mailing > > > > > lists, > > > > > as also mentioned here: > > > > > https://en.wikipedia.org/wiki/DMARC#From:_rewriting > > > > > > > > > > So IMO patchew should automatically use the value of "Reply-To:" in > > > > > that > > > > > case as author of patches instead. > > > > > > > > > > Reducing security cannot be the solution. > > > > > > > > No, there's no need to reduce security. Just change your local git > > > > configuration to produce a 'From:' line in the commit body.. > > > > > > Got it. :) > > > > > > > >> How are you sending patches ? With git send-email ? If so, maybe > > > > >> you > > > > >> can > > > > >> pass something like --from='"Christian Schoenebeck" > > > > >> <qemu_oss@crudebyte.com>'. Since this is a different string, git > > > > >> will > > > > >> assume you're sending someone else's patch : it will automatically > > > > >> add > > > > >> an > > > > >> extra From: made out of the commit Author as recorded in the git > > > > >> tree. > > > > > > > > I think it is probably as simple as a 'git config' command to tell git > > > > to always put a 'From:' in the body of self-authored patches when > > > > using > > > > git format-patch; however, as I don't suffer from munged emails, I > > > > haven't actually tested what that setting would be. > > > > Well, I tried that Eric. The expected solution would be enabling this git > > setting: > > > > git config [--global] format.from true > > https://git-scm.com/docs/git-config#Documentation/git-config.txt-formatfro > > m > > > > But as you can already read from the manual, the overall behaviour of git > > regarding a separate "From:" line in the email body was intended solely > > for > > the use case sender != author. So in practice (at least in my git version) > > git always makes a raw string comparison between sender (name and email) > > string and author string and only adds the separate From: line to the > > body if they differ. > > > > Hence also "git format-patch --from=" only works here if you use a > > different author string (name and email) there, otherwise on a perfect > > string match it is simply ignored and you end up with only one "From:" in > > the email header. > > > > So eventually I added one extra character in my name for now and removed > > it > > manually in the dumped emails subsequently (see today's > > "[PATCH v7 0/3] 9p: Fix file ID collisions"). > > Hence my proposal in some other mail to pass a different string to > git send-email, but I guess this also works for git format-patch. > > eg, adding double quotes around your "firstname name" > > --from='"Christian Schoenebeck" <qemu_oss@crudebyte.com>' Yeah, I will use that for now, since it just works^TM (I tested it). Because for some reason my emails are still mangled on this list. Probably I still have to drop more header fields from dkim's "h=..." setting. We'll see.
[adding git list] On 9/5/19 7:25 AM, Christian Schoenebeck wrote: >>>>> How are you sending patches ? With git send-email ? If so, maybe you >>>>> can >>>>> pass something like --from='"Christian Schoenebeck" >>>>> <qemu_oss@crudebyte.com>'. Since this is a different string, git will >>>>> assume you're sending someone else's patch : it will automatically add >>>>> an >>>>> extra From: made out of the commit Author as recorded in the git tree. >>> >>> I think it is probably as simple as a 'git config' command to tell git >>> to always put a 'From:' in the body of self-authored patches when using >>> git format-patch; however, as I don't suffer from munged emails, I >>> haven't actually tested what that setting would be. > > Well, I tried that Eric. The expected solution would be enabling this git > setting: > > git config [--global] format.from true > https://git-scm.com/docs/git-config#Documentation/git-config.txt-formatfrom > > But as you can already read from the manual, the overall behaviour of git > regarding a separate "From:" line in the email body was intended solely for > the use case sender != author. So in practice (at least in my git version) git > always makes a raw string comparison between sender (name and email) string > and author string and only adds the separate From: line to the body if they > differ. > > Hence also "git format-patch --from=" only works here if you use a different > author string (name and email) there, otherwise on a perfect string match it > is simply ignored and you end up with only one "From:" in the email header. git folks: How hard would it be to improve 'git format-patch'/'git send-email' to have an option to ALWAYS output a From: line in the body, even when the sender is the author, for the case of a mailing list that munges the mail headers due to DMARC/DKIM reasons? > > So eventually I added one extra character in my name for now and removed it > manually in the dumped emails subsequently (see today's > "[PATCH v7 0/3] 9p: Fix file ID collisions"). > > Besides that direct string comparison restriction; git also seems to have a > bug here. Because even if you have sender != author, then git falsely uses > author as sender of the cover letter, whereas the emails of the individual > patches are encoded correctly. At any rate, I'm glad that you have figured out a workaround, even if painful, while we wait for git to provide what we really need. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Mon, Sep 09, 2019 at 09:05:45AM -0500, Eric Blake wrote:
> > But as you can already read from the manual, the overall behaviour of git
> > regarding a separate "From:" line in the email body was intended solely for
> > the use case sender != author. So in practice (at least in my git version) git
> > always makes a raw string comparison between sender (name and email) string
> > and author string and only adds the separate From: line to the body if they
> > differ.
> >
> > Hence also "git format-patch --from=" only works here if you use a different
> > author string (name and email) there, otherwise on a perfect string match it
> > is simply ignored and you end up with only one "From:" in the email header.
>
> git folks:
>
> How hard would it be to improve 'git format-patch'/'git send-email' to
> have an option to ALWAYS output a From: line in the body, even when the
> sender is the author, for the case of a mailing list that munges the
> mail headers due to DMARC/DKIM reasons?
It wouldn't be very hard to ask format-patch to just handle this
unconditionally. Something like:
diff --git a/pretty.c b/pretty.c
index e4ed14effe..9cf79d7874 100644
--- a/pretty.c
+++ b/pretty.c
@@ -451,7 +451,8 @@ void pp_user_info(struct pretty_print_context *pp,
map_user(pp->mailmap, &mailbuf, &maillen, &namebuf, &namelen);
if (cmit_fmt_is_mail(pp->fmt)) {
- if (pp->from_ident && ident_cmp(pp->from_ident, &ident)) {
+ if (pp->always_use_in_body_from ||
+ (pp->from_ident && ident_cmp(pp->from_ident, &ident))) {
struct strbuf buf = STRBUF_INIT;
strbuf_addstr(&buf, "From: ");
but most of the work would be ferrying that option from the command line
down to the pretty-print code.
That would work in conjunction with "--from" to avoid a duplicate. It
might require send-email learning about the option to avoid doing its
own in-body-from management. If you only care about send-email, it might
be easier to just add the option there.
-Peff
On Montag, 9. September 2019 16:25:12 CEST Jeff King wrote:
> On Mon, Sep 09, 2019 at 09:05:45AM -0500, Eric Blake wrote:
> > > But as you can already read from the manual, the overall behaviour of
> > > git
> > > regarding a separate "From:" line in the email body was intended solely
> > > for
> > > the use case sender != author. So in practice (at least in my git
> > > version) git always makes a raw string comparison between sender (name
> > > and email) string and author string and only adds the separate From:
> > > line to the body if they differ.
> > >
> > > Hence also "git format-patch --from=" only works here if you use a
> > > different author string (name and email) there, otherwise on a perfect
> > > string match it is simply ignored and you end up with only one "From:"
> > > in the email header.>
> > git folks:
> >
> > How hard would it be to improve 'git format-patch'/'git send-email' to
> > have an option to ALWAYS output a From: line in the body, even when the
> > sender is the author, for the case of a mailing list that munges the
> > mail headers due to DMARC/DKIM reasons?
>
> It wouldn't be very hard to ask format-patch to just handle this
> unconditionally. Something like:
>
> diff --git a/pretty.c b/pretty.c
> index e4ed14effe..9cf79d7874 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -451,7 +451,8 @@ void pp_user_info(struct pretty_print_context *pp,
> map_user(pp->mailmap, &mailbuf, &maillen, &namebuf, &namelen);
>
> if (cmit_fmt_is_mail(pp->fmt)) {
> - if (pp->from_ident && ident_cmp(pp->from_ident, &ident)) {
> + if (pp->always_use_in_body_from ||
> + (pp->from_ident && ident_cmp(pp->from_ident, &ident))) {
> struct strbuf buf = STRBUF_INIT;
>
> strbuf_addstr(&buf, "From: ");
>
> but most of the work would be ferrying that option from the command line
> down to the pretty-print code.
>
> That would work in conjunction with "--from" to avoid a duplicate. It
> might require send-email learning about the option to avoid doing its
> own in-body-from management. If you only care about send-email, it might
> be easier to just add the option there.
Would it simplify the changes in git if that would be made a
"git config [--global]" setting only? That is, would that probably simplify
that task to one simple function call there in pretty.c?
On the other hand, considering the already existing --from argument and
"format.from" config option:
https://git-scm.com/docs/git-config#Documentation/git-config.txt-formatfrom
Wouldn't it make sense to just drop the currently existing sender != author
string comparison in git and simply always add the "From:" line to the email's
body if "format.from yes" is used, instead of introducing a suggested 2nd
(e.g. "always-from") option? I mean sure automatically removing redundant
information in the generated emails if sender == author sounds nice on first
thought, but does it address anything useful in practice to justify
introduction of a 2nd related option?
Best regards,
Christian Schoenebeck
On Mon, Sep 23, 2019 at 01:19:18PM +0200, Christian Schoenebeck wrote:
> > if (cmit_fmt_is_mail(pp->fmt)) {
> > - if (pp->from_ident && ident_cmp(pp->from_ident, &ident)) {
> > + if (pp->always_use_in_body_from ||
> > + (pp->from_ident && ident_cmp(pp->from_ident, &ident))) {
> > struct strbuf buf = STRBUF_INIT;
> >
> > strbuf_addstr(&buf, "From: ");
> >
> > but most of the work would be ferrying that option from the command line
> > down to the pretty-print code.
> >
> > That would work in conjunction with "--from" to avoid a duplicate. It
> > might require send-email learning about the option to avoid doing its
> > own in-body-from management. If you only care about send-email, it might
> > be easier to just add the option there.
>
> Would it simplify the changes in git if that would be made a
> "git config [--global]" setting only? That is, would that probably simplify
> that task to one simple function call there in pretty.c?
I think a config option would make sense, but we generally try to avoid
adding a config option that doesn't have a matching command-line option.
I also think saving implementation work there is orthogonal. You can as
easily make a global "always_use_in_body_from" as you can call a global
config_get_bool("format-patch.always_use_in_body_from"). :)
And anyway, it's not _that_ much work to pass it around. At least as
much would go into writing documentation and tests. One of the reasons I
left the patch above as a sketch is that I'm not 100% convinced this is
a useful feature. Somebody caring enough about it to make a real patch
would send a signal there.
> On the other hand, considering the already existing --from argument and
> "format.from" config option:
> https://git-scm.com/docs/git-config#Documentation/git-config.txt-formatfrom
>
> Wouldn't it make sense to just drop the currently existing sender != author
> string comparison in git and simply always add the "From:" line to the email's
> body if "format.from yes" is used, instead of introducing a suggested 2nd
> (e.g. "always-from") option? I mean sure automatically removing redundant
> information in the generated emails if sender == author sounds nice on first
> thought, but does it address anything useful in practice to justify
> introduction of a 2nd related option?
Yes, the resulting mail would be correct, in the sense that it could be
applied just fine by git-am. But I think it would be uglier. IOW, I
consider the presence of the in-body From to be a clue that something
interesting is going on (like forwarding somebody else's patch). So from
my perspective, it would just be useless noise. Other communities may
have different opinions, though (I think I have seen some kernel folks
always including all of the possible in-body headers, including Date).
But it seems like it makes sense to keep both possibilities.
-Peff
On Dienstag, 24. September 2019 00:24:15 CEST Jeff King wrote: > > On the other hand, considering the already existing --from argument and > > "format.from" config option: > > https://git-scm.com/docs/git-config#Documentation/git-config.txt-formatfro > > m > > > > Wouldn't it make sense to just drop the currently existing sender != > > author > > string comparison in git and simply always add the "From:" line to the > > email's body if "format.from yes" is used, instead of introducing a > > suggested 2nd (e.g. "always-from") option? I mean sure automatically > > removing redundant information in the generated emails if sender == > > author sounds nice on first thought, but does it address anything useful > > in practice to justify introduction of a 2nd related option? > > Yes, the resulting mail would be correct, in the sense that it could be > applied just fine by git-am. But I think it would be uglier. IOW, I > consider the presence of the in-body From to be a clue that something > interesting is going on (like forwarding somebody else's patch). So from > my perspective, it would just be useless noise. Other communities may > have different opinions, though (I think I have seen some kernel folks > always including all of the possible in-body headers, including Date). > But it seems like it makes sense to keep both possibilities. Exactly, current git behaviour is solely "prettier" (at first thought only though), but does not address anything useful in real life. Current git behaviour does cause real life problems though: Many email lists are munging emails of patch senders whose domain is configured for requiring domain's emails being DKIM signed and/or being subject to SPF rules (a.k.a DMARC). So original sender's From: header is then automatically replaced by an alias (by e.g. mailman): https://en.wikipedia.org/wiki/DMARC#From:_rewriting For instance the email header: From: "Bob Bold" <bold@foo.com> is automatically replaced by lists by something like From: "Bob Bold via Somelist" <somelist@gnu.org> And since git currently always drops the From: line from the email's body if sender == author, as a consequence maintainers applying patches from such lists, always need to rewrite git history subsequently and have to replace patch author's identity manually for each commit to have their correct, real email address and real name in git history instead of something like "Bob Bold via Somelist" <somelist@gnu.org> So what do you find "uglier"? I prefer key info not being lost as default behaviour. :-) Best regards, Christian Schoenebeck
On Tue, Sep 24, 2019 at 11:03:38AM +0200, Christian Schoenebeck wrote: > > Yes, the resulting mail would be correct, in the sense that it could be > > applied just fine by git-am. But I think it would be uglier. IOW, I > > consider the presence of the in-body From to be a clue that something > > interesting is going on (like forwarding somebody else's patch). So from > > my perspective, it would just be useless noise. Other communities may > > have different opinions, though (I think I have seen some kernel folks > > always including all of the possible in-body headers, including Date). > > But it seems like it makes sense to keep both possibilities. > > Exactly, current git behaviour is solely "prettier" (at first thought only > though), but does not address anything useful in real life. I wouldn't agree with that. By being pretty, it also is functionally more useful (I can tell at a glance whether somebody is sending a patch from another author). > Current git behaviour does cause real life problems though: Many email lists > are munging emails of patch senders whose domain is configured for requiring > domain's emails being DKIM signed and/or being subject to SPF rules (a.k.a > DMARC). So original sender's From: header is then automatically replaced by an > alias (by e.g. mailman): https://en.wikipedia.org/wiki/DMARC#From:_rewriting > > For instance the email header: > > From: "Bob Bold" <bold@foo.com> > > is automatically replaced by lists by something like > > From: "Bob Bold via Somelist" <somelist@gnu.org> > > And since git currently always drops the From: line from the email's body if > sender == author, as a consequence maintainers applying patches from such > lists, always need to rewrite git history subsequently and have to replace > patch author's identity manually for each commit to have their correct, real > email address and real name in git history instead of something like > "Bob Bold via Somelist" <somelist@gnu.org> > > So what do you find "uglier"? I prefer key info not being lost as default > behaviour. :-) Sure, for your list that munges From headers, always including an in-body From is way better. But for those of us _not_ on such lists, I'd much prefer not to force the in-body version on them. -Peff
Eric Blake <eblake@redhat.com> writes: > How hard would it be to improve 'git format-patch'/'git send-email' to > have an option to ALWAYS output a From: line in the body, even when the > sender is the author, for the case of a mailing list that munges the > mail headers due to DMARC/DKIM reasons? I'd say that it shouldn't be so hard to implement than realizing what ahd why it is needed, designing what the end-user interaction would be (i.e. command line options? configuration variables? should it be per send-email destination?) and stating all of the above clearly in the documentation and the proposed commit log message. The reason you are asking is...? Am I smelling a volunteer?
At FSF, we've been working on this issue recently. I was planning to
send a general message to qemu-devel, but someone brought it up in a
thread below, so I'm doing it now.
Currently, a message sent to qemu-devel from a domain that publishes a
strict DMARC policy gets what mailman calls "Munge From". For example,
for a message sent to the list:
From: Anne Example Person <exampleperson@examplepersonsdomain>
Is modified my Mailman and sent to subscribers as:
From: Anne Example Person via Qemu-devel <qemu-devel@nongnu.org>
Reply-To: Anne Example Person <exampleperson@examplepersonsdomain>
We've recently made possible an alternative solution that does not need
munging and I call the unmodified message fix. Currently, mailman adds
"[Qemu-devel] " to the subject of messages. Modifying the message breaks
DKIM message signature and thus DMARC. In short: turn that off, and we
can stop from munging. Many lists are already this way, including most
popular @gnu and @nongnu lists, and this week we are doing a mass
conversion of lists which never touched DMARC related list settings (not
qemu-devel). Instead of using the subject prefix to identify a list,
subscribers can use the List-Id, To, and Cc headers. List information
can also be be put in the welcome email to subscribers and the list
information page by list administrators.
Without going into all of the details, here's a few points about why we
concluded the unmodified message fix is better for discussion
lists. Email clients don't all treat munged messages the same way as
unmunged, and humans read these headers so it can confuse people,
causing messages not to be sent to the expected recipients. GNU Mailman
has an option to do "Munge From" always, but does not recommend using
it[1]. While we're not bound by what others do, it's worth noting that
other very large free software communities like Debian GNU/Linux have
adopted the unmodified message fix[2]. The unmodified messages fix
avoids breaking DKIM cryptographic signatures, which show the message
was authorized by the signing domain, which seems generally better for
security. Additionally, patchew has problems, as seen in the below
thread, subject was "[PATCH v6 0/4] 9p: Fix file ID collisions".
There is a small additional wrinkle. Very rarely, someone will send a
message to the list with a bad DKIM signature and publish a strict DMARC
policy, and in that case, we are forced to munge. I've searched all
messages posted to nongnu and gnu lists and, its always by someone
sending via their own mail server, or small enough to consider it that,
so its reasonable to ask them fix their DKIM signatures or turn off
their strict DMARC. I plan to setup an autoresponder to do that
automatically. Another case is if someone sends an html only email,
qemu-devel is configured to convert it to plaintext. That modifies the
message, and if its from a strict DMARC domain, the from munging is
done. Again, you can tell them to stop sending html only email.
I don't know who has the Qemu-devel list admin password, but whoever has
it can adopt the unmodified message fix by changing
dmarc_moderation_action to Accept here:
https://lists.nongnu.org/mailman/admin/qemu-devel/privacy/sender and
remove subject_prefix here
https://lists.nongnu.org/mailman/admin/qemu-devel/general
If the list admins went missing, email mailman@gnu.org and we can sort
out new ones eventually.
A few additional notes for completeness. We announced some of this at
https://lists.gnu.org/archive/html/savannah-hackers-public/2019-06/msg00018.html,
which includes information about other kinds of lists. For the unusual
cases of munging I described, we do from munging through exim because
mailman is not smart enough to only munge in these edge cases, and I'll
document that soon here[1].
[1]: https://wiki.list.org/DEV/DMARC
[2]: https://lists.debian.org/debian-devel-announce/2015/08/msg00003.html
Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org> writes:
> On Montag, 2. September 2019 17:34:32 CEST Greg Kurz wrote:
>> On Sun, 01 Sep 2019 21:28:45 +0200
>>
>> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
>> > On Donnerstag, 29. August 2019 19:02:34 CEST Greg Kurz wrote:
>> > > On Thu, 22 Aug 2019 15:18:54 -0700 (PDT)
>> > >
>> > > no-reply@patchew.org wrote:
>> > > > Patchew URL:
>> > > > https://patchew.org/QEMU/cover.1566503584.git.qemu_oss@crudebyte.com/
>> > > >
>> > > >
>> > > >
>> > > > Hi,
>> > > >
>> > > > This series seems to have some coding style problems. See output below
>> > > > for
>> >
>> > > > more information:
>> > [snip]
>> >
>> > > > === OUTPUT BEGIN ===
>> > > > 1/4 Checking commit bb69de63f788 (9p: Treat multiple devices on one
>> > > > export
>> > > > as an error) ERROR: Author email address is mangled by the mailing
>> > > > list
>> > > > #2:
>> > > > Author: Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>
>> > >
>> > > This is problematic since it ends up in the Author: field in git. Please
>> > > find a way to fix that.
>> >
>> > Like in which way do you imagine that? And where is the actual practical
>> > problem? I mean every patch still has my signed-off-by tag with the
>> > correct
>> > email address ending up in git history.
>>
>> Yes, this only breaks Author: if the patch is applied from the list.
>>
>> > The cause for this issue is that the domain is configured to require DKIM
>> > signatures for all outgoing emails. That's why mailman replaces my address
>> > by "Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>"
>> > placeholder since it could not provide a valid signature.
>> >
>> > There were good reasons for enabling DKIM and it is a good thing for all
>> > domains in general, since that ensures that (i.e. foreign) email addresses
>> > cannot be used as sender address if the actual sender is not authorized
>> > for
>> > sending emails with that address.
>>
>> Don't know much about DKIM but google seems to confirm what you say.
>
> When you view the source of my emails you'll always see a "DKIM-Signature:"
> field in the email header, which is a signature of the email's body and the
> specific email header fields listed in the "DKIM-Signature:" block, so the
> original server can choose by itself which header fields to include ("h=...")
> for signing, but the standard requires the From: field must always be
> included.
>
> A receiving server then obtains the public key from the domain's DNS records
> and checks if the DKIM signature of the email was correct:
> https://en.wikipedia.org/wiki/DomainKeys_Identified_Mail
>
> Additionally the receiving server obtains the so called "DMARC" entry from the
> domain's DNS records. The "DMARC" DNS entry contains the domain's general
> policies how receiving email servers shall handle verification of emails of
> this domain. For instance the DMARC entry may list SMTP servers (e.g. IPs, DNS
> names) eligble to send emails on behalf of the domain at all, and it defines
> what receiving email servers shall do with emails which were identified of not
> being from an eligible source (e.g. sender IP not listed in DMARC entry or
> missing or wrong DKIM signature in email header). For instance if the policy
> is "quarantine" in the DMARC entry then receiving servers are advised to
> simply drop invalid emails: https://en.wikipedia.org/wiki/DMARC
>
>> So, this means that patchew will complain each time you post if we can't
>> find a proper way to address that... :-\
>
> Well, mailman is handling this correctly. It replaces the "From:" field with a
> placeholder and instead adds my actual email address as "Reply-To:" field.
> That's the common way to handle this on mailing lists, as also mentioned here:
> https://en.wikipedia.org/wiki/DMARC#From:_rewriting
>
> So IMO patchew should automatically use the value of "Reply-To:" in that case
> as author of patches instead.
>
> Reducing security cannot be the solution.
>
>> > What I changed in the meantime though is that you should get all my
>> > patches
>> > directly to your personal address, not only from the list. Or did you
>> > receive v6 again just from the list?
>>
>> I've received the patches in my mailbox but I prefer to use the patchwork's
>> pwclient CLI to apply patches... and patchwork captures the patches from
>> the list, so I end up having to patch the authorship manually anyway.
>>
>> How are you sending patches ? With git send-email ? If so, maybe you can
>> pass something like --from='"Christian Schoenebeck"
>> <qemu_oss@crudebyte.com>'. Since this is a different string, git will
>> assume you're sending someone else's patch : it will automatically add an
>> extra From: made out of the commit Author as recorded in the git tree.
>
> I use "git format-patch ..." to dump the invidiual emails as raw email sources
> and then I'll send those raw emails from the command line. So I have even more
> control of what is exactly sent out and could of course also add custom email
> header fields if required, if that would solve the situation somehow, i.e.
> manually as first test and later in automated way. That's not the issue here.
>
> The problem however is that there is probably not any header field I could add
> that would solve the problem. Because I guess patchew is really just taking
> the first "From:" field as author, period.
>
> I think the source files are available, so I could check that.
>
On Tue, 3 Sep 2019 at 20:11, Ian Kelling <iank@fsf.org> wrote: > I don't know who has the Qemu-devel list admin password, but whoever has > it can adopt the unmodified message fix by changing > dmarc_moderation_action to Accept here: > https://lists.nongnu.org/mailman/admin/qemu-devel/privacy/sender and > remove subject_prefix here > https://lists.nongnu.org/mailman/admin/qemu-devel/general I'm one of the list admins, at least for the main qemu-devel list; some of the sublists have different admins (and perhaps different settings -- there's no way to conveniently say "manage all 5 of these lists with the same policies, so it's easy for them to get out of sync, deliberately or accidentally). I have been considering whether we change how we're handling the DMARC problem for the list. I picked munge-the-email initially because I think we didn't really understand the consequences in terms of patchmail, and also because there was a group of subscribers who complained that they liked the [qemu-devel] tag, used it for filtering email, etc. I think overall my opinion has shifted to thinking that the downsides of munge-the-email are too great and we should indeed switch to not modifying the message at all. thanks -- PMM
On Wed, Sep 4, 2019 at 4:30 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Tue, 3 Sep 2019 at 20:11, Ian Kelling <iank@fsf.org> wrote: > > I don't know who has the Qemu-devel list admin password, but whoever has > > it can adopt the unmodified message fix by changing > > dmarc_moderation_action to Accept here: > > https://lists.nongnu.org/mailman/admin/qemu-devel/privacy/sender and > > remove subject_prefix here > > https://lists.nongnu.org/mailman/admin/qemu-devel/general > > I'm one of the list admins, at least for the main qemu-devel > list; some of the sublists have different admins (and > perhaps different settings -- there's no way to conveniently > say "manage all 5 of these lists with the same policies, > so it's easy for them to get out of sync, deliberately > or accidentally). > > I have been considering whether we change how we're handling > the DMARC problem for the list. I picked munge-the-email > initially because I think we didn't really understand the > consequences in terms of patchmail, and also because there > was a group of subscribers who complained that they liked > the [qemu-devel] tag, used it for filtering email, etc. > I think overall my opinion has shifted to thinking that > the downsides of munge-the-email are too great and we should > indeed switch to not modifying the message at all. Yes, I think so too. Thanks for notifying us, Ian! Stefan
Peter Maydell <peter.maydell@linaro.org> writes: > On Tue, 3 Sep 2019 at 20:11, Ian Kelling <iank@fsf.org> wrote: >> I don't know who has the Qemu-devel list admin password, but whoever has >> it can adopt the unmodified message fix by changing >> dmarc_moderation_action to Accept here: >> https://lists.nongnu.org/mailman/admin/qemu-devel/privacy/sender and >> remove subject_prefix here >> https://lists.nongnu.org/mailman/admin/qemu-devel/general > > I'm one of the list admins, at least for the main qemu-devel > list; some of the sublists have different admins (and > perhaps different settings -- there's no way to conveniently > say "manage all 5 of these lists with the same policies, > so it's easy for them to get out of sync, deliberately > or accidentally). > > I have been considering whether we change how we're handling > the DMARC problem for the list. I picked munge-the-email > initially because I think we didn't really understand the > consequences in terms of patchmail, and also because there > was a group of subscribers who complained that they liked > the [qemu-devel] tag, used it for filtering email, etc. > I think overall my opinion has shifted to thinking that > the downsides of munge-the-email are too great and we should > indeed switch to not modifying the message at all. Yes, please.
On Tue, Sep 03, 2019 at 03:11:08PM -0400, Ian Kelling wrote: > At FSF, we've been working on this issue recently. I was planning to > send a general message to qemu-devel, but someone brought it up in a > thread below, so I'm doing it now. > > Currently, a message sent to qemu-devel from a domain that publishes a > strict DMARC policy gets what mailman calls "Munge From". For example, > for a message sent to the list: > > From: Anne Example Person <exampleperson@examplepersonsdomain> > > Is modified my Mailman and sent to subscribers as: > > From: Anne Example Person via Qemu-devel <qemu-devel@nongnu.org> > Reply-To: Anne Example Person <exampleperson@examplepersonsdomain> > > We've recently made possible an alternative solution that does not need > munging and I call the unmodified message fix. Currently, mailman adds > "[Qemu-devel] " to the subject of messages. Modifying the message breaks > DKIM message signature and thus DMARC. In short: turn that off, and we > can stop from munging. Many lists are already this way, including most > popular @gnu and @nongnu lists, and this week we are doing a mass > conversion of lists which never touched DMARC related list settings (not > qemu-devel). Instead of using the subject prefix to identify a list, > subscribers can use the List-Id, To, and Cc headers. List information > can also be be put in the welcome email to subscribers and the list > information page by list administrators. > > Without going into all of the details, here's a few points about why we > concluded the unmodified message fix is better for discussion > lists. Email clients don't all treat munged messages the same way as > unmunged, and humans read these headers so it can confuse people, > causing messages not to be sent to the expected recipients. GNU Mailman > has an option to do "Munge From" always, but does not recommend using > it[1]. While we're not bound by what others do, it's worth noting that > other very large free software communities like Debian GNU/Linux have > adopted the unmodified message fix[2]. The unmodified messages fix > avoids breaking DKIM cryptographic signatures, which show the message > was authorized by the signing domain, which seems generally better for > security. Additionally, patchew has problems, as seen in the below > thread, subject was "[PATCH v6 0/4] 9p: Fix file ID collisions". > > There is a small additional wrinkle. Very rarely, someone will send a > message to the list with a bad DKIM signature and publish a strict DMARC > policy, and in that case, we are forced to munge. I've searched all > messages posted to nongnu and gnu lists and, its always by someone > sending via their own mail server, or small enough to consider it that, > so its reasonable to ask them fix their DKIM signatures or turn off > their strict DMARC. I plan to setup an autoresponder to do that > automatically. Another case is if someone sends an html only email, > qemu-devel is configured to convert it to plaintext. That modifies the > message, and if its from a strict DMARC domain, the from munging is > done. Again, you can tell them to stop sending html only email. I think we should change mailman settings to *NOT* convert HTML to plain text. It is pretty easy to setup mail clients to do this conversion when viewing instead, which will avoid the DMARC problems. eg with mutt you can add auto_view text/html alternative_order text/plain text/html and in $HOME/.mailcap something like text/html; elinks -dump -localhost 1 -no-connect 1 -default-mime-type text/html %s; needsterminal; copiousoutput; > > I don't know who has the Qemu-devel list admin password, but whoever has > it can adopt the unmodified message fix by changing > dmarc_moderation_action to Accept here: > https://lists.nongnu.org/mailman/admin/qemu-devel/privacy/sender and > remove subject_prefix here > https://lists.nongnu.org/mailman/admin/qemu-devel/general > > If the list admins went missing, email mailman@gnu.org and we can sort > out new ones eventually. > > A few additional notes for completeness. We announced some of this at > https://lists.gnu.org/archive/html/savannah-hackers-public/2019-06/msg00018.html, > which includes information about other kinds of lists. For the unusual > cases of munging I described, we do from munging through exim because > mailman is not smart enough to only munge in these edge cases, and I'll > document that soon here[1]. > > [1]: https://wiki.list.org/DEV/DMARC > [2]: https://lists.debian.org/debian-devel-announce/2015/08/msg00003.html Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Daniel P. Berrangé <berrange@redhat.com> writes: > > I think we should change mailman settings to *NOT* convert HTML to > plain text. It is pretty easy to setup mail clients to do this > conversion when viewing instead, which will avoid the DMARC problems. > > eg with mutt you can add > > auto_view text/html > alternative_order text/plain text/html > > and in $HOME/.mailcap something like > > text/html; elinks -dump -localhost 1 -no-connect 1 -default-mime-type text/html %s; needsterminal; copiousoutput; > Both are reasonable, pick your poison. I use emacs gnus and mu4e, both convert to plain text by default afaik. Mailman simply calls out to lynx to do it. To change this, a list admin will just toggle convert_html_to_plaintext at https://lists.nongnu.org/mailman/admin/qemu-devel/contentfilter -- Ian Kelling | Senior Systems Administrator, Free Software Foundation GPG Key: B125 F60B 7B28 7FF6 A2B7 DF8F 170A F0E2 9542 95DF https://fsf.org | https://gnu.org
© 2016 - 2024 Red Hat, Inc.