Re: [Xen-devel] [v7][PATCH 00/16] Fix RMRR

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Thu, Jul 16, 2015 at 10:44 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 16.07.15 at 11:27, <George.Dunlap@xxxxxxxxxxxxx> wrote:
>> On Thu, Jul 16, 2015 at 9:26 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>>> On 16.07.15 at 10:13, <tiejun.chen@xxxxxxxxx> wrote:
>>>>>>>> It looks like most of the libxl/libxc patches have been acked.  It
>>>>>>>> seems to me that most of the hypervisor patches (1-3, 14-15) are
>>>>>>>> either ready to go in or pretty close.
>>>>>>>
>>>>>>> Now that I looked over v8 I have to admit that if I was a tools
>>>>>>> maintainer I wouldn't want to see some of the tools patches in
>>>>>>> with just an ack, but without any review.
>>>>>>
>>>>>> I'm somewhat confused at this point.
>>>>>>
>>>>>> Acked-by: is often used by the maintainer of the affected code when that
>>>>>> maintainer neither contributed to nor forwarded the patch. It is a
>>>>>> record that the acker has at least reviewed the patch and has indicated
>>>>>> acceptance.
>>>>>>
>>>>>> Does this imply this is already reviewed?
>>>>>
>>>>> No, that would be expressed by Reviewed-by. Acked-by merely
>>>>> means no objection by the maintainer for the change to go in.
>>>>>
>>>>
>>>> Sorry I'm trying to dig into this.
>>>>
>>>> If nobody would like to take a look at this, so isn't this the
>>>> associated maintainer's responsibility to review finally? In this case
>>>> isn't Acked-by fine enough?
>>>
>>> Acked-by is good enough for a patch to go in, yes. Note that I
>>> didn't make this a requirement (as I'm not the maintainer), I just
>>> said that if I was the maintainer, I would for at least some of the
>>> tools patches.
>>
>> There does seem to be a disconnect between how "Reviewed-by" and
>> "Acked-by" are used on the tools side vs the hypervisor side.  (We
>> just stumbled across this in an internal discussion about commit stats
>> actually.)
>>
>> But in any case, it's the maintainers' responsibility to determine if
>> something has had sufficient review, and it's their responsibility not
>> to give an Ack unless they really mean "As far as I'm concerned, this
>> is ready to go in."  The fact that there were Acks on the toolstack
>> side ought to mean that this judgement had already been made.
>
> Hmm, that's a different view than I take: To me Reviewed-by implies
> Acked-by, but not the other way around. And I view it as the
> committer's responsibility to ensure a patch has all necessary acks,
> but not the maintainer to give an ack only when reviews were done.

I'm not debating the meaning of Reviewed-by and Acked-by in general;
I'm responding to your questioning earlier whether the tools side was
really ready to go in.

What I'm saying is, when the maintainer gives an "Acked-by", they
should have already either 1) reviewed the code themselves and
determined it's ready to go in (although perhaps in this case a
Reviewed-by would be preferred), or 2) have looked through the patch
and determined based on their trust of the other reviewers, on their
own previous review of the code, or their confidence in the author of
the patch, that the patch is ready to go in.

So your question shouldn't be, "Should we check this in given that
it's only been Acked"?  It should be, "Why did you Ack it when there
has apparently been no review?"  And the answer turns out to be,
"Because we tend to use Acked-by even when we have reviewed it."

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel