Re: [PATCH v2 02/11] xen/gnttab: Rework resource acquisition

From: Andrew Cooper <amc96@cam.ac.uk>
To: Jan Beulich <jbeulich@suse.com>
Cc: "'George Dunlap'" <George.Dunlap@eu.citrix.com>,
	"'Ian Jackson'" <iwj@xenproject.org>,
	"'Stefano Stabellini'" <sstabellini@kernel.org>,
	"'Wei Liu'" <wl@xen.org>, "'Julien Grall'" <julien@xen.org>,
	"'Michał Leszczyński'" <michal.leszczynski@cert.pl>,
	"'Hubert Jasudowicz'" <hubert.jasudowicz@cert.pl>,
	"'Tamas K Lengyel'" <tamas@tklengyel.com>,
	'Xen-devel' <xen-devel@lists.xenproject.org>,
	paul@xen.org
Subject: Re: [PATCH v2 02/11] xen/gnttab: Rework resource acquisition
Date: Tue, 12 Jan 2021 20:06:38 +0000	[thread overview]
Message-ID: <bfe0f28b-4e16-fb39-7aee-b8f538b6f979@cam.ac.uk> (raw)
In-Reply-To: <6eb3bf50-d1dc-e010-16d0-261f7562a140@suse.com>

On 12/01/2021 08:23, Jan Beulich wrote:
> On 11.01.2021 22:22, Andrew Cooper wrote:
>> On 24/09/2020 10:51, Paul Durrant wrote:
>>>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
>>>> index a5d3ed8bda..912f07be47 100644
>>>> --- a/xen/common/grant_table.c
>>>> +++ b/xen/common/grant_table.c
>>>> @@ -4013,6 +4013,81 @@ static int gnttab_get_shared_frame_mfn(struct domain *d,
>>>>      return 0;
>>>>  }
>>>>
>>>> +int gnttab_acquire_resource(
>>>> +    struct domain *d, unsigned int id, unsigned long frame,
>>>> +    unsigned int nr_frames, xen_pfn_t mfn_list[])
>>>> +{
>>>> +    struct grant_table *gt = d->grant_table;
>>>> +    unsigned int i = nr_frames, tot_frames;
>>>> +    mfn_t tmp;
>>>> +    void **vaddrs = NULL;
>>>> +    int rc;
>>>> +
>>>> +    /* Input sanity. */
>>> Nit: inconsistency with full stops on single line comments.
>> The whole point of relaxing the style was because feedback over minutia
>> such as this was deemed detrimental to the community.
>>
>> If I ever see feedback like this, I will commit commit the patch there
>> and then.  This is the only way upstream Xen is going to turn into a
>> less toxic environment for contributors.
> Paul had clearly marked this as "nit". ./CODING_STYLE explicitly
> allows omission as well as presence of a full stop here. So while
> I agree with you that you'd be okay ignoring such a remark, I
> also agree with Paul giving the remark if he's found a nearby
> comment using the opposite variant.

A reply like that means "this needs changing".  This is how it is read
by people, even if it is not how it was intended.

Contributors do not know CODING_STYLE off by heart, and even if they
did, it is the responsibility of a reviewer not to mislead the
contributor, not the contributors responsibility to challenge incorrect
feedback.

As it stands, the reply requires change of something which is explicitly
permitted under CODING_STYLE.  The change to CODING_STYLE was made
specially to prevent feedback of this form to begin with.

This is not an ok use of anyone's time, nor is repeating the argument
which lead to CODING_STYLE being changed to begin with.

~Andrew