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

[Greg Kurz]

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. 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" <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.

> > 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 :)

> 
> > > 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.