RE: [PATCH v6 0/3] XSA-343 followup patches

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of Julien 
> Grall
> Sent: 18 November 2020 14:18
> To: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; George Dunlap 
> <george.dunlap@xxxxxxxxxx>; Ian Jackson
> <iwj@xxxxxxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Wei Liu 
> <wl@xxxxxxx>; Roger Pau
> Monné <roger.pau@xxxxxxxxxx>; Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>; 
> committers@xxxxxxxxxxxxxx; xen-
> devel@xxxxxxxxxxxxxxxxxxxx; Juergen Gross <jgross@xxxxxxxx>
> Subject: Re: [PATCH v6 0/3] XSA-343 followup patches
> 
> Hi Jan,
> 
> On 18/11/2020 08:22, Jan Beulich wrote:
> > On 17.11.2020 19:13, Julien Grall wrote:
> >> On 09/11/2020 16:38, Juergen Gross wrote:
> >>> Juergen Gross (3):
> >>>     xen/events: access last_priority and last_vcpu_id together
> >>>     xen/evtchn: rework per event channel lock
> >>>     xen/evtchn: revert 52e1fc47abc3a0123
> >>
> >> While looking at the list of commits, I noticed that the first patch
> >> hasn't been committed. They were all acked/reviewed, so I am a bit
> >> puzzled why this was omitted...
> >>
> >> I have nearly missed as I was expecting the 3 patches to be committed
> >> together. May I suggest that in the future we reply to the cover letter
> >> and mention which patches are (or not) committed?
> >>
> >> Regarding patch #1, I will commit it tomorrow unless there are strong
> >> objections against.
> >
> > Without a clear outline of what would break with the present logic,
> > I had previously indicated I'm not convinced of the change. This
> > isn't a strong objection, no, but I still wouldn't want to see my
> > name associated with it in such a case.
> 
> I was under the impression that the committer job is mostly mechanical
> (i.e. collecting tags and applying patches). There are no rules in
> MAINTAINERS that mention committers can decide what gets committed as
> long as maintainers approved it and there are no strong objections.
> 
> > Furthermore I clearly view
> > this as not a backporting candidate, while the other two are (as I
> > did previously indicate). Hence the latter two patches wanted
> > re-basing ahead of the first one anyway, to ease the backports.
> 
> I understand the backport concern. However, if there were clash, then it
> means you had to resolve them on commit to staging. Surely, they were
> quite minimal otherwise you would have sent an e-mail on xen-devel, right?
> 
> > While I will accept there are different views possible here, I also
> > don't think sending mail in such a case is a good use of resources.
> > The information what was or was not committed is readily available. > 
> > Personally I view such mails
> as at least very close to spam.
> 
> This is a matter of perspective. It helps to confirm with the
> contributor that it was fine to merge only part of the series (multiple
> pair of eyes is always better than one...) or mentioning any clash/reworked.
> 
> It also makes easier for reviewers to figure out what was committed as
> it can be difficult to know if a patch was merged because commit title
> can be altered (even simply dropping the prefix "xen/" can take a coule
> of more minutes to pinpoint commit).
> 
> Therefore, I think there is a value for such e-mail to be sent out. It
> will likely improve coordination among the member of the community.

+1

Knowing that part of a long series has already been committed would be useful. 
When I do the usual pull/rebase dance prior to re-submitting I have, more than 
once, been surprised by rebase failures because some of the patches had been 
committed but were modified in doing do. It's not a massive problem but an 
email would have at least made me aware I needed to be a bit more careful.

  Paul

> 
> I would be happy to consider different approach if it fullfills that goal.
> 
> Cheers,
> 
> --
> Julien Grall