Re: REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.

[George Dunlap <george.dunlap@citrix.com>]

On Wed, Apr 13, 2016 at 5:07 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> Jan Beulich writes ("Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane."):
>> George Dunlap <dunlapg@umich.edu> 04/12/16 11:58 AM >>>
>> >2. Use the existing hypercall but wedge in different calling semantics
>> >for the build-id subop.  We could have just that subop pay attention
>> >to an extra argument as a length, for example, and return an error if
>> >the length is too short.  Or we could make essentially a flag that
>> >asks, "How much space if I were to execute this subop for real?"
>>
>> A suitable variant of this is what I've been arguing for.
>
> Earlier I wrote:
>
>   It's clear that there are various options, most of which are
>   tolerable.  Buit if I'm trying to help referee a disagreement between
>   Andrew and Jan I would prefer to be choosing between Andrew's
>   preferred answer and Jan's preferred answer.
>
> So as I see it we have two options actually seriously proposed:
> Andrew's new hypercall, and Jan's additional argument (in the struct,
> seems to be what Jan is mainly suggesting).
>
> The new hypercall is neater but more new code - and involves a
> deprecation plan; the additional argument is more messy but less
> duplication.
>
> I think either of these options is tolerable.  I don't see the need to
> look further.
>
> Frankly, I find the choice difficult.  But the bikeshed has to be
> painted /some/ colour and we should make these decisions in a sensible
> way and that means I and George (who've been called on to help decide)
> need to put forward an opinion.
>
> On balance I think I prefer Jan's suggestion, mostly on the grounds
> that in case of dispute, disagreement, or uncertainty, it is (all
> other things being equal) better to make smaller changes.  And if this
> hypercall becomes _too_ much of a mess we can always replace it later
> along the lines that Andrew suggests.

I'm a bit torn here.  I think on the whole, I agree with Ian's
approach that if there is disagreement, then the more conservative
approach should be taken.  And if we were discussing an uncommitted
patch about which no consensus had been reached, I would second his
suggestion.

On the other hand, I think there's a bit of a faulty interpretation of
the procedure here.  Jan reviewed the patch thoroughly and then acked
it; on the basis of that, Konrad legitimately checked it in.  After it
was checked in Jan said, "I've changed my mind and withdraw my Ack";
and the assumption of the subsequent conversation was that an ack
*can* be withdrawn after it has been legitimately checked in, and that
if no other Ack is supplied, then it must be reverted.

I don't think that's a correct interpretation of the rules.  Reviewers
in general, and maintainers in particular, should make reasonably sure
that they mean the Ack before they give it; and if they change their
mind after it has been legitimately checked in, then it's now up to
them to make the change they want to see according to the regular
procedure.  That is, if Jan wants it reverted, he needs to post a
patch reverting it and get Acks from the appropriate maintainers; and
the discussion needs to be around Jan's reversion being accepted, not
about Konrad's original patch continuing to be accepted.  (Obvious
exceptions can be made in the case of emergencies like build
breakages.)

Normally it's best to try to come to agreement instead of falling back
to rule lawyering; and in any case it's always easier to talk about
the rules when we're not trying to sort out a specific situation.  But
since we've failed to reach a genuine agreement, in this case I think
the rule lawyering is probably a legitimate way to resolve the issue.

Thoughts?

 -George

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