Re: [Qemu-devel] [PATCH v6 0/4] 9p: Fix file ID collisions

[Christian Schoenebeck]

On Montag, 2. September 2019 17:34:32 CEST Greg Kurz wrote:
> On Sun, 01 Sep 2019 21:28:45 +0200
> 
> Christian Schoenebeck <address@hidden> wrote:
> > On Donnerstag, 29. August 2019 19:02:34 CEST Greg Kurz wrote:
> > > On Thu, 22 Aug 2019 15:18:54 -0700 (PDT)
> > > 
> > > address@hidden wrote:
> > > > Patchew URL:
> > > > https://patchew.org/QEMU/address@hidden/
> > > > 
> > > > 
> > > > 
> > > > 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 <address@hidden>
> > > 
> > > 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 <address@hidden>"
> > 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"
> <address@hidden>'. 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 <address@hidden>
> > > > 
> > > > 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 :)