Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
>>> George Dunlap 04/15/16 1:23 PM >>> >On Thu, Apr 14, 2016 at 6:01 PM, Jan Beulich wrote: >>>Sure, mistakes happen; but I hope it's not being to controversial to >>>say that in general, the procedure should be arranged such that the >>>person who makes the mistake is the one who has to do deal with the >>>most consequences from the mistake, not the people around him. I >>>mean, if you asked a bartender for a bottle of beer, and after he'd >>>already opened it you said, "Oh sorry, I actually wanted a cider", it >>>would be fair enough for the bartender to ask you to pay for the beer, >>>rather than eating it*, wouldn't it? :-) >> >> Sure. And I think that's what I've done. To be fair, I think this was too broad a statement: I had indeed asked Konrad to collect the replacement ack. Yet on second thought I really can't see what's wrong with withdraing an ack - just like a patch can be withdrawn, that should be possible for an ack too. And if the patch has already gone in, no matter whether the patch itself or the ack got withdrawn, the patch imo should then be reverted. I agree that's not something spelled out anywhere, so my opinion here is based on solely general considerations of mine. >When Konrad asked for further input on the patch and then didn't get >any in a few days, you responded, "It looks like it will have to be >reverted then." As I've said, I think that's the wrong way round -- >it's not that the commit is reverted unless someone else acks it; it's >that the commit stays unless someone acks your reversion. If you had >posted a patch (probably RFC) requesting to revert the change in favor >of a different one, then it would have been more obvious that the >burden was on you to get the reversion Acked, rather than on Konrad to >get the existing commit re-acked. I agree on the process aspect (albeit among the few cases where things needed reverting, I think there was a non-negligible amount of such where the revert was requested quite informally), but that's relatively moot with me not agreeing on the premises it builds upon. >>>Well Ian and I have already given our opinions -- Ian thinks moving to >>>a clean interface and deprecating the old one is in general a good >>>thing, and doesn't look too painful in this case. I don't really see >>>a problem with using a large fixed size, but I don't fundamentally see >>>a problem with moving to a new clean interface either. Given that >>>Andy has a strong aversion to the way things are, if you had only a >>>mild distaste rather than a strong objection to the new hypercall, it >>>would probably make sense to go with the new hypercall. >>> >>>If you do have a strong objection, then maybe we could try to convince >>>Andy to accept adding subops with different calling semantics to the >>>existing hypercall. But I would still think the burden of persuasion >>>is primarily on you. >> >> I do not have a strong objection, or else I would have converted my ack >> into a nak instead of just withdrawing it. I just dislike the duplication, >> and >> hence I'm not happy with me now being the one having approved it to go >> in. Hence the request for a replacement ack (or whatever else referee >> decision). > >Well this makes it sound like you're saying that you were asking us to >save you from having to appear to have approved of a patch that you >didn't like. Which doesn't sound very nice. :-) I'm sorry if it's being understood that way. My intention really was to avoid the revert in case a replacement ack can be obtained. Otherwise, following my way of thinking outlined above, the patch should have been reverted right away. Yet I seem to be the only one here thinking this way... >But I wonder if something slightly different is going on. Forgive me >for trying to guess at motivations here, but I think it may be >helpful. I'm often in the situation where my gut objects to a patch >that other coders I respect think is fine; and often a few years down >the road, I look back and agree that it's fine as well. In other >words, I know that sometimes my own objections turn out to be >unreasonable; and that in any case, working with other people you >sometimes have to compromise. But on the other hand, I've also had >the experience of giving in and accepting patches that later I regret, >or that other people come back and say, "This was a terrible idea, you >should have stood up for it more." > >So in the moment -- particularly, as you say, under time pressure -- >how do you determine if objecting to the patch is being unreasonable >and obstructive, or if assenting to the patch is failing your duty as >a maintainer to prevent bad code, and is a decision you'll regret >later? > >Was it perhaps actually the case that your internal reasoning was >along the lines of, "Actually, adding a new hypercall seems like a bad >idea. But since Andrew strongly disagrees, maybe I'm being >unreasonable. Or, maybe it really is a bad idea and he's being >unreasonable. W
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane. [and 1 more messages]
>>> Ian Jackson 04/14/16 8:12 PM >>>
>Jan Beulich writes ("Re: [Xen-devel] REST MAINTAINERS feedback requested Was:
>> And btw., considering that Konrad has already posted a revert patch,
>> and I have ack-ed that one, this could now go in right away (and the
>> discussion could either be settled or start over).
>
>I don't see that patch you describe in my inbox, but maybe I have
>missed it.
Patches 1 and 2 of the most recent v8.1 series.
>If that reversion is proposed, following a request for a 2nd/3rd
>opinion from me and George, and given the discussion so far, I think
>that patch ought to have been CC'd to me and George.
>
>I don't think it would be appropriate to commit a revert except as
>part of a series which introduces an replacement way of providing the
>needed functionality - at least, enough functionality that in practice
>a plausibly long build-id can be retrieved.
For one, the revert wouldn't revert that functionality, as that didn't even go
in yet.
>If you want the original reverted, I think it is up to you, Jan, to
>provide (or procure) such a replacement.
And with that (albeit not just because of it not being in yet at all), I don't
see
why this would be the case. I think there's some general disagreement here,
which with the hackathon around the corner we probably should just get
sorted out in person there.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
On Thu, Apr 14, 2016 at 6:01 PM, Jan Beulich wrote: >>Sure, mistakes happen; but I hope it's not being to controversial to >>say that in general, the procedure should be arranged such that the >>person who makes the mistake is the one who has to do deal with the >>most consequences from the mistake, not the people around him. I >>mean, if you asked a bartender for a bottle of beer, and after he'd >>already opened it you said, "Oh sorry, I actually wanted a cider", it >>would be fair enough for the bartender to ask you to pay for the beer, >>rather than eating it*, wouldn't it? :-) > > Sure. And I think that's what I've done. I suggested to revert the thing, > collecting opinions either direction. I just didn't post a revert patch, as I > think that makes little sense - a revert is a mechanical operation, which > doesn't need people looking at the actual patch. You don't need people to review the content of the actual patch, but it provides a standardized way to have a discussion about the patch, and it makes it clear what the procedure is for having it applied. You don't need any technical code review for adding or removing maintainers either, but we ask people to send patches to the MAINTAINERS file anyway, because it provides a structured way to have an open discussion and make a decision. When Konrad asked for further input on the patch and then didn't get any in a few days, you responded, "It looks like it will have to be reverted then." As I've said, I think that's the wrong way round -- it's not that the commit is reverted unless someone else acks it; it's that the commit stays unless someone acks your reversion. If you had posted a patch (probably RFC) requesting to revert the change in favor of a different one, then it would have been more obvious that the burden was on you to get the reversion Acked, rather than on Konrad to get the existing commit re-acked. >>Well Ian and I have already given our opinions -- Ian thinks moving to >>a clean interface and deprecating the old one is in general a good >>thing, and doesn't look too painful in this case. I don't really see >>a problem with using a large fixed size, but I don't fundamentally see >>a problem with moving to a new clean interface either. Given that >>Andy has a strong aversion to the way things are, if you had only a >>mild distaste rather than a strong objection to the new hypercall, it >>would probably make sense to go with the new hypercall. >> >>If you do have a strong objection, then maybe we could try to convince >>Andy to accept adding subops with different calling semantics to the >>existing hypercall. But I would still think the burden of persuasion >>is primarily on you. > > I do not have a strong objection, or else I would have converted my ack > into a nak instead of just withdrawing it. I just dislike the duplication, and > hence I'm not happy with me now being the one having approved it to go > in. Hence the request for a replacement ack (or whatever else referee > decision). Well this makes it sound like you're saying that you were asking us to save you from having to appear to have approved of a patch that you didn't like. Which doesn't sound very nice. :-) But I wonder if something slightly different is going on. Forgive me for trying to guess at motivations here, but I think it may be helpful. I'm often in the situation where my gut objects to a patch that other coders I respect think is fine; and often a few years down the road, I look back and agree that it's fine as well. In other words, I know that sometimes my own objections turn out to be unreasonable; and that in any case, working with other people you sometimes have to compromise. But on the other hand, I've also had the experience of giving in and accepting patches that later I regret, or that other people come back and say, "This was a terrible idea, you should have stood up for it more." So in the moment -- particularly, as you say, under time pressure -- how do you determine if objecting to the patch is being unreasonable and obstructive, or if assenting to the patch is failing your duty as a maintainer to prevent bad code, and is a decision you'll regret later? Was it perhaps actually the case that your internal reasoning was along the lines of, "Actually, adding a new hypercall seems like a bad idea. But since Andrew strongly disagrees, maybe I'm being unreasonable. Or, maybe it really is a bad idea and he's being unreasonable. Why don't I ask the REST maintainers to look at it; if they Ack it, then I'm probably being too conservative; if they don't, then I'm probably justified in objecting to it"? If so, we should probably find a better way for maintainers to ask for additional feedback in those situations. :-) I'd certainly appreciate that at times. If that's not the case, and you genuinely only have a mild dislike for the hypercall, then there are a couple of things to say about that. The first is that given the circumstances, it
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane. [and 1 more messages]
On Thu, Apr 14, 2016 at 07:11:46PM +0100, Ian Jackson wrote:
> George Dunlap writes ("Re: [Xen-devel] REST MAINTAINERS feedback requested
> Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring
> XENVER_ but sane."):
> > 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.
>
> For the record, I agree completely with George here. I was expecting
> that the next step would be to for Jan to post patches to revert the
> extra hypercall and replace it with something else.
>
> Jan Beulich writes ("Re: [Xen-devel] REST MAINTAINERS feedback requested Was:
> > And btw., considering that Konrad has already posted a revert patch,
> > and I have ack-ed that one, this could now go in right away (and the
> > discussion could either be settled or start over).
>
> I don't see that patch you describe in my inbox, but maybe I have
> missed it.
It is part of my series. This is the revert (there are two of them)
http://lists.xen.org/archives/html/xen-devel/2016-04/msg01913.html
http://lists.xen.org/archives/html/xen-devel/2016-04/msg01926.html
And then these two patches add build-id using the XENVER hypercall:
http://lists.xen.org/archives/html/xen-devel/2016-04/msg01923.html
http://lists.xen.org/archives/html/xen-devel/2016-04/msg01902.html
>
> If that reversion is proposed, following a request for a 2nd/3rd
> opinion from me and George, and given the discussion so far, I think
> that patch ought to have been CC'd to me and George.
Argh, I probably missed you and George on them. My apologies!
>
> I don't think it would be appropriate to commit a revert except as
> part of a series which introduces an replacement way of providing the
> needed functionality - at least, enough functionality that in practice
> a plausibly long build-id can be retrieved.
>
> If you want the original reverted, I think it is up to you, Jan, to
> provide (or procure) such a replacement.
>
> Thanks,
> Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane. [and 1 more messages]
George Dunlap writes ("Re: [Xen-devel] REST MAINTAINERS feedback requested
Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_
but sane."):
> 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.
For the record, I agree completely with George here. I was expecting
that the next step would be to for Jan to post patches to revert the
extra hypercall and replace it with something else.
Jan Beulich writes ("Re: [Xen-devel] REST MAINTAINERS feedback requested Was:
> And btw., considering that Konrad has already posted a revert patch,
> and I have ack-ed that one, this could now go in right away (and the
> discussion could either be settled or start over).
I don't see that patch you describe in my inbox, but maybe I have
missed it.
If that reversion is proposed, following a request for a 2nd/3rd
opinion from me and George, and given the discussion so far, I think
that patch ought to have been CC'd to me and George.
I don't think it would be appropriate to commit a revert except as
part of a series which introduces an replacement way of providing the
needed functionality - at least, enough functionality that in practice
a plausibly long build-id can be retrieved.
If you want the original reverted, I think it is up to you, Jan, to
provide (or procure) such a replacement.
Thanks,
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
On Thu, Apr 14, 2016 at 4:59 PM, Jan Beulich wrote: George Dunlap 04/14/16 5:16 PM >>> >>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.) > > Fundamentally I agree, but I think there's more to this than just following > a set of rules. For example, please don't forget the time pressure due to > the (at that time) rapidly approaching freeze date. And then, mistakes > happen, and so I made a mistake here by sending the ack a few hours too > early. Sure, mistakes happen; but I hope it's not being to controversial to say that in general, the procedure should be arranged such that the person who makes the mistake is the one who has to do deal with the most consequences from the mistake, not the people around him. I mean, if you asked a bartender for a bottle of beer, and after he'd already opened it you said, "Oh sorry, I actually wanted a cider", it would be fair enough for the bartender to ask you to pay for the beer, rather than eating it*, wouldn't it? :-) > What is really hard to understand to me is why it is so difficult to just > get a refereeing opinion on the actual interface change. IMO we don't > really need to discuss rules and processes, the question is as simple > as "Do we want/need this new interface?" Well Ian and I have already given our opinions -- Ian thinks moving to a clean interface and deprecating the old one is in general a good thing, and doesn't look too painful in this case. I don't really see a problem with using a large fixed size, but I don't fundamentally see a problem with moving to a new clean interface either. Given that Andy has a strong aversion to the way things are, if you had only a mild distaste rather than a strong objection to the new hypercall, it would probably make sense to go with the new hypercall. If you do have a strong objection, then maybe we could try to convince Andy to accept adding subops with different calling semantics to the existing hypercall. But I would still think the burden of persuasion is primarily on you. -George * In this context "eating" something means just accepting the loss of the thing yourself. For instance, if you bought two tickets to a concert but couldn't convince anyone to go with you, you'd say you had to "eat the other ticket". Here it means the bartender throws the beer away and accepts the loss himself. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
>>> George Dunlap 04/14/16 6:20 PM >>> >On Thu, Apr 14, 2016 at 4:59 PM, Jan Beulich wrote: > George Dunlap 04/14/16 5:16 PM >>> >>>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.) >> >> Fundamentally I agree, but I think there's more to this than just following >> a set of rules. For example, please don't forget the time pressure due to >> the (at that time) rapidly approaching freeze date. And then, mistakes >> happen, and so I made a mistake here by sending the ack a few hours too >> early. > >Sure, mistakes happen; but I hope it's not being to controversial to >say that in general, the procedure should be arranged such that the >person who makes the mistake is the one who has to do deal with the >most consequences from the mistake, not the people around him. I >mean, if you asked a bartender for a bottle of beer, and after he'd >already opened it you said, "Oh sorry, I actually wanted a cider", it >would be fair enough for the bartender to ask you to pay for the beer, >rather than eating it*, wouldn't it? :-) Sure. And I think that's what I've done. I suggested to revert the thing, collecting opinions either direction. I just didn't post a revert patch, as I think that makes little sense - a revert is a mechanical operation, which doesn't need people looking at the actual patch. >Well Ian and I have already given our opinions -- Ian thinks moving to >a clean interface and deprecating the old one is in general a good >thing, and doesn't look too painful in this case. I don't really see >a problem with using a large fixed size, but I don't fundamentally see >a problem with moving to a new clean interface either. Given that >Andy has a strong aversion to the way things are, if you had only a >mild distaste rather than a strong objection to the new hypercall, it >would probably make sense to go with the new hypercall. > >If you do have a strong objection, then maybe we could try to convince >Andy to accept adding subops with different calling semantics to the >existing hypercall. But I would still think the burden of persuasion >is primarily on you. I do not have a strong objection, or else I would have converted my ack into a nak instead of just withdrawing it. I just dislike the duplication, and hence I'm not happy with me now being the one having approved it to go in. Hence the request for a replacement ack (or whatever else referee decision). And btw., considering that Konrad has already posted a revert patch, and I have ack-ed that one, this could now go in right away (and the discussion could either be settled or start over). Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
>>> George Dunlap 04/14/16 5:16 PM >>> >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.) Fundamentally I agree, but I think there's more to this than just following a set of rules. For example, please don't forget the time pressure due to the (at that time) rapidly approaching freeze date. And then, mistakes happen, and so I made a mistake here by sending the ack a few hours too early. What is really hard to understand to me is why it is so difficult to just get a refereeing opinion on the actual interface change. IMO we don't really need to discuss rules and processes, the question is as simple as "Do we want/need this new interface?" Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
On Wed, Apr 13, 2016 at 5:07 PM, Ian Jackson 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 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
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
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 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 look forward to hearing from George.
Thanks,
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
>>> Ian Jackson 04/12/16 6:47 PM >>>
>George Dunlap writes ("Re: [Xen-devel] REST MAINTAINERS feedback requested
>Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_
>>but sane."):
>> Well we know which option Andy prefers, but are there other options
>> that Andy is not absolutely opposed to? And we don't know anything
>> about which option Jan prefers at all, except that it's not #4.
>
>Let me go a bit further than George.
>
>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.
>
>Jan: AFAICT it's clear that you would still like the current patch
>reverted. Can you please say what, if anything, you would like to
>replace it with ?
That patch doesn't need replacing by anything. It's the follow-up patch adding
support to retrieve the build-id which would need a replacement, and several
options have been put on the table. As mentioned before, I'd prefer the variant
of the new sub-op getting added to the existing version hypercall, with the
needed length argument passed either via a structure element, with the
structure pointed to by the 2nd hypercall argument, or with the high bits of
the first hypercall argument re-purposed to allow (and for this sub-op require)
holding a length. Which of these two sub-options is chosen I don't really care
much.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
George Dunlap writes ("Re: [Xen-devel] REST MAINTAINERS feedback requested
Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_
but sane."):
> Well we know which option Andy prefers, but are there other options
> that Andy is not absolutely opposed to? And we don't know anything
> about which option Jan prefers at all, except that it's not #4.
Let me go a bit further than George.
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.
Jan: AFAICT it's clear that you would still like the current patch
reverted. Can you please say what, if anything, you would like to
replace it with ?
Thanks,
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
>>> George Dunlap 04/12/16 4:38 PM >>> >On Tue, Apr 12, 2016 at 2:56 PM, Konrad Rzeszutek Wilk > wrote: >> options (1-4) seem perfectly fine to me. FWIW my preferred color >> would probably be 1 because it's the easiest and least inconsistent >> with the current state of things. My least favorite would be 3, >> because although each individual piece of information is only in one >> place, the path to get there is duplicated; both the kernel developer >> and the hypervisor developer are forced to continue to deal with both. > > The state is that 1-3 have been Nacked by Andrew, 4 has been > Nacked by Jan. And 5) (the original way) was way way back Nacked > as well. No, I only withdrew my ack. Hence an ack from another REST maintainer would suffice for it to stay in as it is now. >There's a difference between "I think this other way is better" and "I >am absolutely opposed to this". > >Well we know which option Andy prefers, but are there other options >that Andy is not absolutely opposed to? And we don't know anything >about which option Jan prefers at all, except that it's not #4. As said in another reply - a suitable variant of 2. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
On Tue, Apr 12, 2016 at 09:17:29AM -0600, Jan Beulich wrote: > >>> George Dunlap 04/12/16 11:58 AM >>> > >On Mon, Apr 11, 2016 at 6:46 PM, Jan Beulich wrote: > >>> ISTM that the lower duplication (which in principle is an advantage > >>> which will be time limited if we are ever able to completely remove > >>> teh old hypercall) comes with the cost of (in the long term) increased > >>> mess in this particular subop. > >> > >> We cannot, ever, remove a not toolstack limited hypercall completely > >> (or if, then only by Kconfig option so that people knowing none of the > >> guests they care about use such deprecated ones). > > > >We need to keep the hypercall around and functioning for ABI > >compatibility, but we could at least remove it from the public headers > >and point people to using the new one instead. (Discussion about the > >merit of this idea below.) > > Please don't forget that guests use this to be deprecated hypercall as one of > the > cheapest possible ways to call into the hypervisor just to get events > delivered. > I'm afraid the new hypercall would mean (slightly) more overhead. In fact I'm > afraid the addition of the XSM call to the old one already had some of this > effect > namely when XSM is enabled: Konrad - was this considered when you added > that? Yes, and I raised it up as well :-) However, now that we dropped the XSM checks for some of the sub-ops, it is pointless to do the XSM checks for them (they just do function functions calls that end up with return 0). I can most certainly add back the mask of flags - so only specific sub-ops go through the XSM check. Let me write a patch for this and send it out for review shortly. > > >OK, so here are the options I see: > > > >1. Use the existing hypercall with the existing call semantics for > >build-id -- i.e., require the caller to have a large but fixed-length > >buffer (maybe 1024 or 2048). > > I think it was explained even on this thread that a fixed size may indeed not > be suitable here. > > >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. > > Jan > ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
>>> George Dunlap 04/12/16 11:58 AM >>> >On Mon, Apr 11, 2016 at 6:46 PM, Jan Beulich wrote: >>> ISTM that the lower duplication (which in principle is an advantage >>> which will be time limited if we are ever able to completely remove >>> teh old hypercall) comes with the cost of (in the long term) increased >>> mess in this particular subop. >> >> We cannot, ever, remove a not toolstack limited hypercall completely >> (or if, then only by Kconfig option so that people knowing none of the >> guests they care about use such deprecated ones). > >We need to keep the hypercall around and functioning for ABI >compatibility, but we could at least remove it from the public headers >and point people to using the new one instead. (Discussion about the >merit of this idea below.) Please don't forget that guests use this to be deprecated hypercall as one of the cheapest possible ways to call into the hypervisor just to get events delivered. I'm afraid the new hypercall would mean (slightly) more overhead. In fact I'm afraid the addition of the XSM call to the old one already had some of this effect namely when XSM is enabled: Konrad - was this considered when you added that? >OK, so here are the options I see: > >1. Use the existing hypercall with the existing call semantics for >build-id -- i.e., require the caller to have a large but fixed-length >buffer (maybe 1024 or 2048). I think it was explained even on this thread that a fixed size may indeed not be suitable here. >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. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
On Tue, Apr 12, 2016 at 03:38:31PM +0100, George Dunlap wrote: > On Tue, Apr 12, 2016 at 2:56 PM, Konrad Rzeszutek Wilk > wrote: > >> 3. We could use a new hypercall only for new functionality, with the > >> proposed new semantics. This would at minimum be build-id, but > >> probably also extraversion, compileinfo, changeset, maybe > >> capabilities_info. > >> > >> 4. Have the new hypercall replace the old hypercall. The new > >> hypercall will duplicate all the functionality of the old hypercall. > >> Deprecate the hypercall for a release or two, then remove it from the > >> public headers (although keep the code, because we need to maintain > >> backwards compatibility). > > > > 5). Stick the build-id in the xsplice sysctl. Or just in the sysctl. > > Which is somewhat similar to #3, except that instead of being in its > own hypercall, it's with a bunch of other related functionality. I'd > be OK with that color too; it's not a great color but I think it's > better than 3. :-) > > >> This does seem to me an awful lot like a bike shed. :-) All of the > > > > This is really past bike shedding - all the bikes shed have been > > already built (for all those options). > > You mean, the shed already has 3 layers of paint, a layer of > wallpaper, and a layer of plastic siding? :-) Yes. Very much so! > > >> options (1-4) seem perfectly fine to me. FWIW my preferred color > >> would probably be 1 because it's the easiest and least inconsistent > >> with the current state of things. My least favorite would be 3, > >> because although each individual piece of information is only in one > >> place, the path to get there is duplicated; both the kernel developer > >> and the hypervisor developer are forced to continue to deal with both. > >> > > > > > > The state is that 1-3 have been Nacked by Andrew, 4 has been > > Nacked by Jan. And 5) (the original way) was way way back Nacked > > as well. > > There's a difference between "I think this other way is better" and "I > am absolutely opposed to this". > > Well we know which option Andy prefers, but are there other options > that Andy is not absolutely opposed to? And we don't know anything > about which option Jan prefers at all, except that it's not #4. /me nods. Thank you for pointing that out. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
On Tue, Apr 12, 2016 at 2:56 PM, Konrad Rzeszutek Wilk wrote: >> 3. We could use a new hypercall only for new functionality, with the >> proposed new semantics. This would at minimum be build-id, but >> probably also extraversion, compileinfo, changeset, maybe >> capabilities_info. >> >> 4. Have the new hypercall replace the old hypercall. The new >> hypercall will duplicate all the functionality of the old hypercall. >> Deprecate the hypercall for a release or two, then remove it from the >> public headers (although keep the code, because we need to maintain >> backwards compatibility). > > 5). Stick the build-id in the xsplice sysctl. Or just in the sysctl. Which is somewhat similar to #3, except that instead of being in its own hypercall, it's with a bunch of other related functionality. I'd be OK with that color too; it's not a great color but I think it's better than 3. :-) >> This does seem to me an awful lot like a bike shed. :-) All of the > > This is really past bike shedding - all the bikes shed have been > already built (for all those options). You mean, the shed already has 3 layers of paint, a layer of wallpaper, and a layer of plastic siding? :-) >> options (1-4) seem perfectly fine to me. FWIW my preferred color >> would probably be 1 because it's the easiest and least inconsistent >> with the current state of things. My least favorite would be 3, >> because although each individual piece of information is only in one >> place, the path to get there is duplicated; both the kernel developer >> and the hypervisor developer are forced to continue to deal with both. >> > > > The state is that 1-3 have been Nacked by Andrew, 4 has been > Nacked by Jan. And 5) (the original way) was way way back Nacked > as well. There's a difference between "I think this other way is better" and "I am absolutely opposed to this". Well we know which option Andy prefers, but are there other options that Andy is not absolutely opposed to? And we don't know anything about which option Jan prefers at all, except that it's not #4. -George ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
On Tue, Apr 12, 2016 at 10:58:09AM +0100, George Dunlap wrote: > On Mon, Apr 11, 2016 at 6:46 PM, Jan Beulich wrote: > [snip] > >> ISTM that the lower duplication (which in principle is an advantage > >> which will be time limited if we are ever able to completely remove > >> teh old hypercall) comes with the cost of (in the long term) increased > >> mess in this particular subop. > > > > We cannot, ever, remove a not toolstack limited hypercall completely > > (or if, then only by Kconfig option so that people knowing none of the > > guests they care about use such deprecated ones). > > We need to keep the hypercall around and functioning for ABI > compatibility, but we could at least remove it from the public headers > and point people to using the new one instead. (Discussion about the > merit of this idea below.) > > [snip] > >> ISTM that the lower duplication (which in principle is an advantage > >> which will be time limited if we are ever able to completely remove > >> teh old hypercall) comes with the cost of (in the long term) increased > >> mess in this particular subop. > >> > >> Is that right ? Obviously this cost is not very significant. But > >> maybe the duplication isn't that significant either. I was kind of > >> expecting to find stronger arguments on both sides in this > >> discussion... > > > > If, btw, the cost of having to read the length argument from guest > > memory was deemed undesirable, we'd certainly have the option > > of specifying it to be passed through, say, the high half of the > > current "cmd" parameter. > > OK, so here are the options I see: > > 1. Use the existing hypercall with the existing call semantics for > build-id -- i.e., require the caller to have a large but fixed-length > buffer (maybe 1024 or 2048). > > 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?" You can't wedge in the extra argument. Tried that an Jan pointed out that we clobber it (specifically we clobber any non-used arguments). > > 3. We could use a new hypercall only for new functionality, with the > proposed new semantics. This would at minimum be build-id, but > probably also extraversion, compileinfo, changeset, maybe > capabilities_info. > > 4. Have the new hypercall replace the old hypercall. The new > hypercall will duplicate all the functionality of the old hypercall. > Deprecate the hypercall for a release or two, then remove it from the > public headers (although keep the code, because we need to maintain > backwards compatibility). 5). Stick the build-id in the xsplice sysctl. Or just in the sysctl. > > Honestly I still don't quite understand what the problem is with #1 -- > if build-id is mainly meant to be a UUID or a hash of the build to > make sure that you're applying the right hotfix (please correct me if > I'm wrong here -- I haven't had time to actually follow the patch > series), 256 bytes should be enough for a properly hashed build, and > 2048 should be more than enough. Requiring the caller to have a > 2048-byte buffer before calling doesn't really seem like that much of > a hardship to me. Basically: > > a. It's nicer to have arbitrary-length strings (2-4), but reasonably > large fixed-length ones aren't awful (1) > > b. It's nicer for hypercall consumers to have a single hypercall with > consistent semantics (1,4), but having two hypercalls (3) or a single > one with inconsistent semantics (2) aren't that bad either. > > c. It's nicer for hypervisor maintainers to have a single place to > support any given bit of functionality (1-3), but having a slightly > duplicated functionality that only has to be supported in an ABI > backwards-compatible manner isn't that bad either (4). > > This does seem to me an awful lot like a bike shed. :-) All of the This is really past bike shedding - all the bikes shed have been already built (for all those options). > options (1-4) seem perfectly fine to me. FWIW my preferred color > would probably be 1 because it's the easiest and least inconsistent > with the current state of things. My least favorite would be 3, > because although each individual piece of information is only in one > place, the path to get there is duplicated; both the kernel developer > and the hypervisor developer are forced to continue to deal with both. > The state is that 1-3 have been Nacked by Andrew, 4 has been Nacked by Jan. And 5) (the original way) was way way back Nacked as well. I believe this conversation is to break a tie-breaker between maintainers. (See http://www.xenproject.org/governance.html, Refereeing). That is any of the REST maintainers or Project Leads will override the Nack/Acks. Granted, Jan, me, and Andrew are exclud
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
On Mon, Apr 11, 2016 at 6:46 PM, Jan Beulich wrote: [snip] >> ISTM that the lower duplication (which in principle is an advantage >> which will be time limited if we are ever able to completely remove >> teh old hypercall) comes with the cost of (in the long term) increased >> mess in this particular subop. > > We cannot, ever, remove a not toolstack limited hypercall completely > (or if, then only by Kconfig option so that people knowing none of the > guests they care about use such deprecated ones). We need to keep the hypercall around and functioning for ABI compatibility, but we could at least remove it from the public headers and point people to using the new one instead. (Discussion about the merit of this idea below.) [snip] >> ISTM that the lower duplication (which in principle is an advantage >> which will be time limited if we are ever able to completely remove >> teh old hypercall) comes with the cost of (in the long term) increased >> mess in this particular subop. >> >> Is that right ? Obviously this cost is not very significant. But >> maybe the duplication isn't that significant either. I was kind of >> expecting to find stronger arguments on both sides in this >> discussion... > > If, btw, the cost of having to read the length argument from guest > memory was deemed undesirable, we'd certainly have the option > of specifying it to be passed through, say, the high half of the > current "cmd" parameter. OK, so here are the options I see: 1. Use the existing hypercall with the existing call semantics for build-id -- i.e., require the caller to have a large but fixed-length buffer (maybe 1024 or 2048). 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?" 3. We could use a new hypercall only for new functionality, with the proposed new semantics. This would at minimum be build-id, but probably also extraversion, compileinfo, changeset, maybe capabilities_info. 4. Have the new hypercall replace the old hypercall. The new hypercall will duplicate all the functionality of the old hypercall. Deprecate the hypercall for a release or two, then remove it from the public headers (although keep the code, because we need to maintain backwards compatibility). Honestly I still don't quite understand what the problem is with #1 -- if build-id is mainly meant to be a UUID or a hash of the build to make sure that you're applying the right hotfix (please correct me if I'm wrong here -- I haven't had time to actually follow the patch series), 256 bytes should be enough for a properly hashed build, and 2048 should be more than enough. Requiring the caller to have a 2048-byte buffer before calling doesn't really seem like that much of a hardship to me. Basically: a. It's nicer to have arbitrary-length strings (2-4), but reasonably large fixed-length ones aren't awful (1) b. It's nicer for hypercall consumers to have a single hypercall with consistent semantics (1,4), but having two hypercalls (3) or a single one with inconsistent semantics (2) aren't that bad either. c. It's nicer for hypervisor maintainers to have a single place to support any given bit of functionality (1-3), but having a slightly duplicated functionality that only has to be supported in an ABI backwards-compatible manner isn't that bad either (4). This does seem to me an awful lot like a bike shed. :-) All of the options (1-4) seem perfectly fine to me. FWIW my preferred color would probably be 1 because it's the easiest and least inconsistent with the current state of things. My least favorite would be 3, because although each individual piece of information is only in one place, the path to get there is duplicated; both the kernel developer and the hypervisor developer are forced to continue to deal with both. -George ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
>>> On 11.04.16 at 19:13, wrote:
> Jan Beulich writes ("Re: REST MAINTAINERS feedback requested Was:Re:
> [Xen-devel] [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring
> XENVER_ but sane."):
>> On 11.04.16 at 18:25, wrote:
>> > But any improvement from an old API to a new one necessarily involves
>> > providing a dual facility during a transition period.
>>
>> Except that, at least for most sub-ops, the new one doesn't really
>> provide much advantage, and hence dealing with the lack of size
>> for those sub-ops where it matters within the existing hypercall
>> (perhaps by adding one or two new sub-ops) would limit duplication
>> quite a bit.
>
> ISTM that the lower duplication (which in principle is an advantage
> which will be time limited if we are ever able to completely remove
> teh old hypercall) comes with the cost of (in the long term) increased
> mess in this particular subop.
>
> Is that right ? Obviously this cost is not very significant. But
> maybe the duplication isn't that significant either. I was kind of
> expecting to find stronger arguments on both sides in this
> discussion...
If, btw, the cost of having to read the length argument from guest
memory was deemed undesirable, we'd certainly have the option
of specifying it to be passed through, say, the high half of the
current "cmd" parameter.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
>>> On 11.04.16 at 19:13, wrote:
> Jan Beulich writes ("Re: REST MAINTAINERS feedback requested Was:Re:
> [Xen-devel] [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring
> XENVER_ but sane."):
>> On 11.04.16 at 18:25, wrote:
>> > But any improvement from an old API to a new one necessarily involves
>> > providing a dual facility during a transition period.
>>
>> Except that, at least for most sub-ops, the new one doesn't really
>> provide much advantage, and hence dealing with the lack of size
>> for those sub-ops where it matters within the existing hypercall
>> (perhaps by adding one or two new sub-ops) would limit duplication
>> quite a bit.
>
> ISTM that the lower duplication (which in principle is an advantage
> which will be time limited if we are ever able to completely remove
> teh old hypercall) comes with the cost of (in the long term) increased
> mess in this particular subop.
We cannot, ever, remove a not toolstack limited hypercall completely
(or if, then only by Kconfig option so that people knowing none of the
guests they care about use such deprecated ones).
>> > I don't see an explicit deprecation in the patch that is in tree, but
>> > it seems to me to be intended (and, perhaps, implied). Certainly if
>> > we are going to permit these strings etc. to be bigger than fits in
>> > the old hypercall, the old hypercall needs to be deprecated on the
>> > grounds that it can provide incomplete or inaccurate information.
>> >
>> > Does this way of looking at it help ?
>>
>> If that means you approve of the introduction of the new hypercall,
>> yes. After all the goal of this whole discussion is to determine
>> whether another maintainer is willing to provide a replacement ack
>> for the withdrawn one.
>
> Err. Well, I am still asking questions. When I said "does that help"
> I was meaning something like "does that help bring you closer to
> agreement with Andrew". But I don't want to focus on procedure here.
Oh, well, in that case the answer is "no". Bike shed issue or not,
having to maintain two pieces of code with similar functionality is
undesirable. And it's not like we didn't have any XSA for that
(apparently trivial) piece of code already.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
Jan Beulich writes ("Re: REST MAINTAINERS feedback requested Was:Re:
[Xen-devel] [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring
XENVER_ but sane."):
> On 11.04.16 at 18:25, wrote:
> > But any improvement from an old API to a new one necessarily involves
> > providing a dual facility during a transition period.
>
> Except that, at least for most sub-ops, the new one doesn't really
> provide much advantage, and hence dealing with the lack of size
> for those sub-ops where it matters within the existing hypercall
> (perhaps by adding one or two new sub-ops) would limit duplication
> quite a bit.
ISTM that the lower duplication (which in principle is an advantage
which will be time limited if we are ever able to completely remove
teh old hypercall) comes with the cost of (in the long term) increased
mess in this particular subop.
Is that right ? Obviously this cost is not very significant. But
maybe the duplication isn't that significant either. I was kind of
expecting to find stronger arguments on both sides in this
discussion...
Andrew, can you please confirm what you think of Jan's suggestion to
instead add a new sub-op where the sub-op structure contains a
guest-provided length field ? Are there other reasons to want to
introduce a new hypercall ?
> > I don't see an explicit deprecation in the patch that is in tree, but
> > it seems to me to be intended (and, perhaps, implied). Certainly if
> > we are going to permit these strings etc. to be bigger than fits in
> > the old hypercall, the old hypercall needs to be deprecated on the
> > grounds that it can provide incomplete or inaccurate information.
> >
> > Does this way of looking at it help ?
>
> If that means you approve of the introduction of the new hypercall,
> yes. After all the goal of this whole discussion is to determine
> whether another maintainer is willing to provide a replacement ack
> for the withdrawn one.
Err. Well, I am still asking questions. When I said "does that help"
I was meaning something like "does that help bring you closer to
agreement with Andrew". But I don't want to focus on procedure here.
If we're going to have the argument over this bike shed I'd like to
figure out what colour I myself prefer :-). But I need to know what
the reasons are behind people's preferences.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
>>> On 11.04.16 at 18:53, wrote:
> On Mon, Apr 11, 2016 at 05:25:04PM +0100, Ian Jackson wrote:
>> Jan Beulich writes ("Re: REST MAINTAINERS feedback requested Was:Re:
>> Certainly if
>> we are going to permit these strings etc. to be bigger than fits in
>> the old hypercall, the old hypercall needs to be deprecated on the
>> grounds that it can provide incomplete or inaccurate information.
>
> The build-id in Config.mk is set to use sha1. Which produces 20 bytes.
> You (or anybody else) can modify Config.mk to modify --build-id
> as per man ld (there is an uuid or md5 or):
>
> "0xhexstring" to use a chosen bit string specified as an even number of
> hexadecimal
> digits ("-" and ":" characters between digit pairs are ignored)."
>
> which does not impose any limits.
This has nothing to do with the discussion here, imo. The new
build-id sub-op can, as said numerous times before, easily have
a buffer length communicated.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
>>> On 11.04.16 at 18:25, wrote:
> Jan Beulich writes ("Re: REST MAINTAINERS feedback requested Was:Re:
> [Xen-devel] [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring
> XENVER_ but sane."):
>> On 11.04.16 at 16:22, wrote:
>> > But to an extent some of this conversation seems to be on matters of
>> > taste.
>>
>> Agreed.
>>
>> > Jan, what is the downside of introducing a new hypercall ?
>>
>> Duplicate code effectively doing the same thing.
>
> I agree that duplication is bad, all other things being equal.
>
> But any improvement from an old API to a new one necessarily involves
> providing a dual facility during a transition period.
Except that, at least for most sub-ops, the new one doesn't really
provide much advantage, and hence dealing with the lack of size
for those sub-ops where it matters within the existing hypercall
(perhaps by adding one or two new sub-ops) would limit duplication
quite a bit.
> I don't see an explicit deprecation in the patch that is in tree, but
> it seems to me to be intended (and, perhaps, implied). Certainly if
> we are going to permit these strings etc. to be bigger than fits in
> the old hypercall, the old hypercall needs to be deprecated on the
> grounds that it can provide incomplete or inaccurate information.
>
> Does this way of looking at it help ?
If that means you approve of the introduction of the new hypercall,
yes. After all the goal of this whole discussion is to determine
whether another maintainer is willing to provide a replacement ack
for the withdrawn one.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
On Mon, Apr 11, 2016 at 05:25:04PM +0100, Ian Jackson wrote:
> Jan Beulich writes ("Re: REST MAINTAINERS feedback requested Was:Re:
> [Xen-devel] [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring
> XENVER_ but sane."):
> > On 11.04.16 at 16:22, wrote:
> > > But to an extent some of this conversation seems to be on matters of
> > > taste.
> >
> > Agreed.
> >
> > > Jan, what is the downside of introducing a new hypercall ?
> >
> > Duplicate code effectively doing the same thing.
>
> I agree that duplication is bad, all other things being equal.
>
> But any improvement from an old API to a new one necessarily involves
> providing a dual facility during a transition period.
>
> I don't see an explicit deprecation in the patch that is in tree, but
> it seems to me to be intended (and, perhaps, implied). Certainly if
I tried it at some point by adding the suffix 'compat' to it.
The compat layer did not like the extra compat string and all kinds of
compilation issues arose. I put it on the backburner.
> we are going to permit these strings etc. to be bigger than fits in
> the old hypercall, the old hypercall needs to be deprecated on the
> grounds that it can provide incomplete or inaccurate information.
The build-id in Config.mk is set to use sha1. Which produces 20 bytes.
You (or anybody else) can modify Config.mk to modify --build-id
as per man ld (there is an uuid or md5 or):
"0xhexstring" to use a chosen bit string specified as an even number of
hexadecimal
digits ("-" and ":" characters between digit pairs are ignored)."
which does not impose any limits. Albeit 2967 characters of 0xdeadbeef is all I
seem to be able
jam on the line. Weird. Anyhow:
build_id :
deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdead
is possible.
>
> Does this way of looking at it help ?
>
> Thanks,
> Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
Jan Beulich writes ("Re: REST MAINTAINERS feedback requested Was:Re:
[Xen-devel] [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring
XENVER_ but sane."):
> On 11.04.16 at 16:22, wrote:
> > But to an extent some of this conversation seems to be on matters of
> > taste.
>
> Agreed.
>
> > Jan, what is the downside of introducing a new hypercall ?
>
> Duplicate code effectively doing the same thing.
I agree that duplication is bad, all other things being equal.
But any improvement from an old API to a new one necessarily involves
providing a dual facility during a transition period.
I don't see an explicit deprecation in the patch that is in tree, but
it seems to me to be intended (and, perhaps, implied). Certainly if
we are going to permit these strings etc. to be bigger than fits in
the old hypercall, the old hypercall needs to be deprecated on the
grounds that it can provide incomplete or inaccurate information.
Does this way of looking at it help ?
Thanks,
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
>>> On 11.04.16 at 16:22, wrote:
> Konrad Rzeszutek Wilk writes ("Re: REST MAINTAINERS feedback requested
> Was:Re: [Xen-devel] [PATCH v5 01/28] HYPERCALL_version_op. New hypercall
> mirroring XENVER_ but sane."):
>> On Mon, Apr 11, 2016 at 11:50:25AM +0100, Ian Jackson wrote:
>> > I don't think I would be content with simply adding a new sub-op with
>> > bigger fixed-length fields.
>>
>> It was variable-ish.
> ...
>> /* Return value is the number of bytes written, or XEN_Exx on error.
>> * Calling with empty parameter returns the size of build_id. */
> ...
>> #define XENVER_build_id 10
>> struct xen_build_id {
>> uint32_tlen; /* IN: size of buf[]. */
>> #if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L
>> unsigned char buf[];
>
> This is pretty ugly but tolerable.
>
> The comment introducing the new HYPERCALL_version_op mentions some
> other differences with HYPERCALL_xen_version, which seem to suggest
> other deficiencies in the latter. Those deficiencies, together with
> the ugliness of the above, would tend to suggest to me that a cleaner
> new interface is warranted.
>
> But to an extent some of this conversation seems to be on matters of
> taste.
Agreed.
> Jan, what is the downside of introducing a new hypercall ?
Duplicate code effectively doing the same thing.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
Konrad Rzeszutek Wilk writes ("Re: REST MAINTAINERS feedback requested Was:Re:
[Xen-devel] [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring
XENVER_ but sane."):
> On Mon, Apr 11, 2016 at 11:50:25AM +0100, Ian Jackson wrote:
> > I don't think I would be content with simply adding a new sub-op with
> > bigger fixed-length fields.
>
> It was variable-ish.
...
> /* Return value is the number of bytes written, or XEN_Exx on error.
> * Calling with empty parameter returns the size of build_id. */
...
> #define XENVER_build_id 10
> struct xen_build_id {
> uint32_tlen; /* IN: size of buf[]. */
> #if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L
> unsigned char buf[];
This is pretty ugly but tolerable.
The comment introducing the new HYPERCALL_version_op mentions some
other differences with HYPERCALL_xen_version, which seem to suggest
other deficiencies in the latter. Those deficiencies, together with
the ugliness of the above, would tend to suggest to me that a cleaner
new interface is warranted.
But to an extent some of this conversation seems to be on matters of
taste.
Jan, what is the downside of introducing a new hypercall ?
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
On Mon, Apr 11, 2016 at 11:50:25AM +0100, Ian Jackson wrote:
> Jan Beulich writes ("Re: REST MAINTAINERS feedback requested Was:Re:
> [Xen-devel] [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring
> XENVER_ but sane."):
> > On 08.04.16 at 19:41, wrote:
> > > The interface for the old version was sufficiently useless that build_id
> > > can't be added to it. (Specifically, there is no ability to return
> > > varialble length data).
> >
> > This is simply not true: The hypercall being passed a void handle,
> > everything can be arranged for without introducing a new
> > hypercall.
>
> I'm trying to understand what your alternative suggestion is. Could
> you please be more explicit ?
>
> I'm reading xen/include/public/version.h. (Sadly it doesn't have the
> API doc markup.) AFAICT there is a XENVER_ sub-op namespace which
> permits extensions to the XENVER hypercall.
>
> But does that permit the caller to specify their buffer size ?
Only via the sub-op parameters itself. As in you cannot expand the
amount of parameters the XENVER_hypercall can do (which is two - subops
number and the pointer to arguments).
>
> > > The new hypercall has a ration interface where you don't blindly trust
> > > that the caller passed you a pointer to a suitably-sized structure.
>
> Ie I think ^ this is for me a key question.
>
> > While the new one is indeed slightly neater, that's not sufficient
> > for such redundancy imo. That's the whole reason for withdrawing
> > my ack _without_ making it an explicit NAK.
>
> I don't think I would be content with simply adding a new sub-op with
> bigger fixed-length fields.
It was variable-ish.
The new-subop (xsplice.v3) ended up 'lets-try-this-size' subop. Meaning:
/* Return value is the number of bytes written, or XEN_Exx on error.
* Calling with empty parameter returns the size of build_id. */
#define XENVER_build_id 10
struct xen_build_id {
uint32_tlen; /* IN: size of buf[]. */
#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L
unsigned char buf[];
#elif defined(__GNUC__)
unsigned char buf[1]; /* OUT: Variable length buffer with build_id. */
#endif
};
When constructing this in libxl we could do:
xen_build_id_t *build_id;
ret= xc_version(ctx->xch, XENVER_build_id, NULL);
if ( ret != -ENOSYS && ret > 0 )
build_id = libxl__zalloc(gc, ret + sizeof(*build_id));
build_id->len = info->pagesize - sizeof(*build_id);
ret= xc_version(ctx->xch, XENVER_build_id, build_id);
.. parse it...
}
For the default case, ret would be 20, which meant the structure had
to be 24 bytes. 4 bytes were for 'len' (which had the value of 20)
and the rest inside the build_id were to be filled with the hypervisor.
For glory details:
http://xenbits.xen.org/gitweb/?p=people/konradwilk/xen.git;a=commitdiff;h=63681414ede6e38c1910077d7a225e0d67f0ff2e;hp=37efc2ac64b9ecf0cd49fb65aa7c7659467c9318
and
http://xenbits.xen.org/gitweb/?p=people/konradwilk/xen.git;a=commitdiff;h=176c63f3c0db430c70c28fe81cb8d039ae459c66;hp=63681414ede6e38c1910077d7a225e0d67f0ff2e
(albeit we first tried with the default 1020 bytes we have on the stack).
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
Jan Beulich writes ("Re: REST MAINTAINERS feedback requested Was:Re:
[Xen-devel] [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring
XENVER_ but sane."):
> On 08.04.16 at 19:41, wrote:
> > The interface for the old version was sufficiently useless that build_id
> > can't be added to it. (Specifically, there is no ability to return
> > varialble length data).
>
> This is simply not true: The hypercall being passed a void handle,
> everything can be arranged for without introducing a new
> hypercall.
I'm trying to understand what your alternative suggestion is. Could
you please be more explicit ?
I'm reading xen/include/public/version.h. (Sadly it doesn't have the
API doc markup.) AFAICT there is a XENVER_ sub-op namespace which
permits extensions to the XENVER hypercall.
But does that permit the caller to specify their buffer size ?
> > The new hypercall has a ration interface where you don't blindly trust
> > that the caller passed you a pointer to a suitably-sized structure.
Ie I think ^ this is for me a key question.
> While the new one is indeed slightly neater, that's not sufficient
> for such redundancy imo. That's the whole reason for withdrawing
> my ack _without_ making it an explicit NAK.
I don't think I would be content with simply adding a new sub-op with
bigger fixed-length fields.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
>>> On 08.04.16 at 19:41, wrote:
> On 08/04/16 18:21, Ian Jackson wrote:
>> Konrad Rzeszutek Wilk writes ("Re: REST MAINTAINERS feedback requested
> Was:Re: [Xen-devel] [PATCH v5 01/28] HYPERCALL_version_op. New hypercall
> mirroring XENVER_ but sane."):
>>> On Fri, Apr 08, 2016 at 10:33:33AM -0600, Jan Beulich wrote:
Yet nothing has happened, so I think the patch needs to be
reverted (at least for the time being).
>>> Wait what?!
>> I'm sorry that I didn't understand that we were being asked for a
>> second opinion about this disagreement. I'm afriad that the original
>> email wasn't really comprehensible to me as a summary of the
>> disagreement.
>>
>> Would someone please summarise ? Especially, since Jan is AFAICT
>> saying that this new hypercall is not needed, it would be helpful to
>> know why those who think it is needed want it.
>
> The new hypercall is very definitely needed, which is why I requested it
> during earlier revisions of the xsplice series.
>
> The interface for the old version was sufficiently useless that build_id
> can't be added to it. (Specifically, there is no ability to return
> varialble length data).
This is simply not true: The hypercall being passed a void handle,
everything can be arranged for without introducing a new
hypercall.
> Also, by its design, it has some
> unreasonably-short limits on extraversion and changesetinfo, both of
> which could do with being longer for distros trying to encode "delta
> from upstream" information.
>
> The new hypercall has a ration interface where you don't blindly trust
> that the caller passed you a pointer to a suitably-sized structure.
While the new one is indeed slightly neater, that's not sufficient
for such redundancy imo. That's the whole reason for withdrawing
my ack _without_ making it an explicit NAK.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
On 08/04/16 18:21, Ian Jackson wrote:
> Konrad Rzeszutek Wilk writes ("Re: REST MAINTAINERS feedback requested
> Was:Re: [Xen-devel] [PATCH v5 01/28] HYPERCALL_version_op. New hypercall
> mirroring XENVER_ but sane."):
>> On Fri, Apr 08, 2016 at 10:33:33AM -0600, Jan Beulich wrote:
>>> Yet nothing has happened, so I think the patch needs to be
>>> reverted (at least for the time being).
>> Wait what?!
> I'm sorry that I didn't understand that we were being asked for a
> second opinion about this disagreement. I'm afriad that the original
> email wasn't really comprehensible to me as a summary of the
> disagreement.
>
> Would someone please summarise ? Especially, since Jan is AFAICT
> saying that this new hypercall is not needed, it would be helpful to
> know why those who think it is needed want it.
The new hypercall is very definitely needed, which is why I requested it
during earlier revisions of the xsplice series.
The interface for the old version was sufficiently useless that build_id
can't be added to it. (Specifically, there is no ability to return
varialble length data). Also, by its design, it has some
unreasonably-short limits on extraversion and changesetinfo, both of
which could do with being longer for distros trying to encode "delta
from upstream" information.
The new hypercall has a ration interface where you don't blindly trust
that the caller passed you a pointer to a suitably-sized structure.
I am very much +10 keep to the patch.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
On Fri, Apr 08, 2016 at 01:23:23PM -0400, Konrad Rzeszutek Wilk wrote: > On Fri, Apr 08, 2016 at 06:21:27PM +0100, Wei Liu wrote: > > On Fri, Apr 08, 2016 at 11:13:08AM -0600, Jan Beulich wrote: > > > >>> On 08.04.16 at 19:09, wrote: > > > > On Fri, Apr 08, 2016 at 10:33:33AM -0600, Jan Beulich wrote: > > > >> >>> On 31.03.16 at 15:28, wrote: > > > >> > On Thu, Mar 31, 2016 at 06:07:58AM -0600, Jan Beulich wrote: > > > >> >> >>> On 31.03.16 at 13:43, wrote: > > > >> >> > On Thu, Mar 31, 2016 at 12:30:09AM -0600, Jan Beulich wrote: > > > >> >> >> >>> On 30.03.16 at 17:43, wrote: > > > >> >> >> > Since they're all cosmetic, if you take care of all of them, > > > >> >> >> > feel free > > > >> >> >> > to stick my ack on the result. > > > >> >> >> > > > >> >> >> Actually - no, please don't. While the patch is fine content wise > > > >> >> >> then from my perspective, I'm still lacking a convincing argument > > > >> >> >> of why this new hypercall is needed in the first place. If others > > > >> >> >> are convinced by the argumentation between (mostly, iirc) you > > > >> >> >> and Andrew, I'm not going to stand in the way, but I'm also not > > > >> >> >> going to approve of the code addition without being myself > > > >> >> >> convinced. > > > >> >> > > > > >> >> > Damm. I pushed the patch in yesterday in 'staging'! > > > >> >> > > > > >> >> > We can always revert them.. > > > >> >> > > > > >> >> > "Others" being other maintainers I presume? > > > >> >> > > > >> >> Any one of the REST maintainers, yes. > > > >> > > > > >> > Changing the title to get their attention. > > > >> > > > >> Yet nothing has happened, so I think the patch needs to be > > > >> reverted (at least for the time being). > > > > > > > > Wait what?! > > > > > > > > The natural consensus mechanism we use is lazy. If nobody > > > > objects then it is Acked. > > > > > > Since when can patches go in without any ack(s) of relevant > > > maintainers? > > > > > > > Urgh, at the risk of pointing out the obvious -- it does seem to have > > your ack... > > Which was given at night before Jan retracted it in the morning. > > This feels like one of those Catch-22 :-) Ah, OK then, so Jan has a point. This needs to be properly discussed and refereed. Wei. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
On Fri, Apr 08, 2016 at 06:21:27PM +0100, Wei Liu wrote: > On Fri, Apr 08, 2016 at 11:13:08AM -0600, Jan Beulich wrote: > > >>> On 08.04.16 at 19:09, wrote: > > > On Fri, Apr 08, 2016 at 10:33:33AM -0600, Jan Beulich wrote: > > >> >>> On 31.03.16 at 15:28, wrote: > > >> > On Thu, Mar 31, 2016 at 06:07:58AM -0600, Jan Beulich wrote: > > >> >> >>> On 31.03.16 at 13:43, wrote: > > >> >> > On Thu, Mar 31, 2016 at 12:30:09AM -0600, Jan Beulich wrote: > > >> >> >> >>> On 30.03.16 at 17:43, wrote: > > >> >> >> > Since they're all cosmetic, if you take care of all of them, > > >> >> >> > feel free > > >> >> >> > to stick my ack on the result. > > >> >> >> > > >> >> >> Actually - no, please don't. While the patch is fine content wise > > >> >> >> then from my perspective, I'm still lacking a convincing argument > > >> >> >> of why this new hypercall is needed in the first place. If others > > >> >> >> are convinced by the argumentation between (mostly, iirc) you > > >> >> >> and Andrew, I'm not going to stand in the way, but I'm also not > > >> >> >> going to approve of the code addition without being myself > > >> >> >> convinced. > > >> >> > > > >> >> > Damm. I pushed the patch in yesterday in 'staging'! > > >> >> > > > >> >> > We can always revert them.. > > >> >> > > > >> >> > "Others" being other maintainers I presume? > > >> >> > > >> >> Any one of the REST maintainers, yes. > > >> > > > >> > Changing the title to get their attention. > > >> > > >> Yet nothing has happened, so I think the patch needs to be > > >> reverted (at least for the time being). > > > > > > Wait what?! > > > > > > The natural consensus mechanism we use is lazy. If nobody > > > objects then it is Acked. > > > > Since when can patches go in without any ack(s) of relevant > > maintainers? > > > > Urgh, at the risk of pointing out the obvious -- it does seem to have > your ack... Which was given at night before Jan retracted it in the morning. This feels like one of those Catch-22 :-) ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
Konrad Rzeszutek Wilk writes ("Re: REST MAINTAINERS feedback requested Was:Re:
[Xen-devel] [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring
XENVER_ but sane."):
> On Fri, Apr 08, 2016 at 10:33:33AM -0600, Jan Beulich wrote:
> > Yet nothing has happened, so I think the patch needs to be
> > reverted (at least for the time being).
>
> Wait what?!
I'm sorry that I didn't understand that we were being asked for a
second opinion about this disagreement. I'm afriad that the original
email wasn't really comprehensible to me as a summary of the
disagreement.
Would someone please summarise ? Especially, since Jan is AFAICT
saying that this new hypercall is not needed, it would be helpful to
know why those who think it is needed want it.
In the meantime I think it would be best to avoid churn by leaving the
patch in tree for now. I promise that I won't let those "facts on the
ground" influence my views about whether this hypercall is needed.
Thanks,
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
On Fri, Apr 08, 2016 at 11:13:08AM -0600, Jan Beulich wrote: > >>> On 08.04.16 at 19:09, wrote: > > On Fri, Apr 08, 2016 at 10:33:33AM -0600, Jan Beulich wrote: > >> >>> On 31.03.16 at 15:28, wrote: > >> > On Thu, Mar 31, 2016 at 06:07:58AM -0600, Jan Beulich wrote: > >> >> >>> On 31.03.16 at 13:43, wrote: > >> >> > On Thu, Mar 31, 2016 at 12:30:09AM -0600, Jan Beulich wrote: > >> >> >> >>> On 30.03.16 at 17:43, wrote: > >> >> >> > Since they're all cosmetic, if you take care of all of them, feel > >> >> >> > free > >> >> >> > to stick my ack on the result. > >> >> >> > >> >> >> Actually - no, please don't. While the patch is fine content wise > >> >> >> then from my perspective, I'm still lacking a convincing argument > >> >> >> of why this new hypercall is needed in the first place. If others > >> >> >> are convinced by the argumentation between (mostly, iirc) you > >> >> >> and Andrew, I'm not going to stand in the way, but I'm also not > >> >> >> going to approve of the code addition without being myself > >> >> >> convinced. > >> >> > > >> >> > Damm. I pushed the patch in yesterday in 'staging'! > >> >> > > >> >> > We can always revert them.. > >> >> > > >> >> > "Others" being other maintainers I presume? > >> >> > >> >> Any one of the REST maintainers, yes. > >> > > >> > Changing the title to get their attention. > >> > >> Yet nothing has happened, so I think the patch needs to be > >> reverted (at least for the time being). > > > > Wait what?! > > > > The natural consensus mechanism we use is lazy. If nobody > > objects then it is Acked. > > Since when can patches go in without any ack(s) of relevant > maintainers? > Urgh, at the risk of pointing out the obvious -- it does seem to have your ack... Wei. > Jan > ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
On Fri, Apr 08, 2016 at 10:33:33AM -0600, Jan Beulich wrote: > >>> On 31.03.16 at 15:28, wrote: > > On Thu, Mar 31, 2016 at 06:07:58AM -0600, Jan Beulich wrote: > >> >>> On 31.03.16 at 13:43, wrote: > >> > On Thu, Mar 31, 2016 at 12:30:09AM -0600, Jan Beulich wrote: > >> >> >>> On 30.03.16 at 17:43, wrote: > >> >> > Since they're all cosmetic, if you take care of all of them, feel free > >> >> > to stick my ack on the result. > >> >> > >> >> Actually - no, please don't. While the patch is fine content wise > >> >> then from my perspective, I'm still lacking a convincing argument > >> >> of why this new hypercall is needed in the first place. If others > >> >> are convinced by the argumentation between (mostly, iirc) you > >> >> and Andrew, I'm not going to stand in the way, but I'm also not > >> >> going to approve of the code addition without being myself > >> >> convinced. > >> > > >> > Damm. I pushed the patch in yesterday in 'staging'! > >> > > >> > We can always revert them.. > >> > > >> > "Others" being other maintainers I presume? > >> > >> Any one of the REST maintainers, yes. > > > > Changing the title to get their attention. > > Yet nothing has happened, so I think the patch needs to be > reverted (at least for the time being). Wait what?! The natural consensus mechanism we use is lazy. If nobody objects then it is Acked. Anyhow pinged Ian Jackson on IRC. > > Jan > ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.
>>> On 08.04.16 at 19:09, wrote: > On Fri, Apr 08, 2016 at 10:33:33AM -0600, Jan Beulich wrote: >> >>> On 31.03.16 at 15:28, wrote: >> > On Thu, Mar 31, 2016 at 06:07:58AM -0600, Jan Beulich wrote: >> >> >>> On 31.03.16 at 13:43, wrote: >> >> > On Thu, Mar 31, 2016 at 12:30:09AM -0600, Jan Beulich wrote: >> >> >> >>> On 30.03.16 at 17:43, wrote: >> >> >> > Since they're all cosmetic, if you take care of all of them, feel >> >> >> > free >> >> >> > to stick my ack on the result. >> >> >> >> >> >> Actually - no, please don't. While the patch is fine content wise >> >> >> then from my perspective, I'm still lacking a convincing argument >> >> >> of why this new hypercall is needed in the first place. If others >> >> >> are convinced by the argumentation between (mostly, iirc) you >> >> >> and Andrew, I'm not going to stand in the way, but I'm also not >> >> >> going to approve of the code addition without being myself >> >> >> convinced. >> >> > >> >> > Damm. I pushed the patch in yesterday in 'staging'! >> >> > >> >> > We can always revert them.. >> >> > >> >> > "Others" being other maintainers I presume? >> >> >> >> Any one of the REST maintainers, yes. >> > >> > Changing the title to get their attention. >> >> Yet nothing has happened, so I think the patch needs to be >> reverted (at least for the time being). > > Wait what?! > > The natural consensus mechanism we use is lazy. If nobody > objects then it is Acked. Since when can patches go in without any ack(s) of relevant maintainers? Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
