[PING] sys/ptrace.h: remove obsolete Linux PTRACE_SEIZE_DEVEL constant

  On Tue, Aug 08, 2017 at 09:20:17AM -0400, Carlos O'Donell wrote:
> On 08/07/2017 11:33 AM, Dmitry V. Levin wrote:
> > Hi,
> > 
> > Looks like among those few who care about sys/ptrace.h nobody feels
> > experienced enough to review this change, so I'll go forward and commit it.
> 
> Please tread carefully, and give the machine maintainer time to review, or
> directly TO: the machine maintainers and ask for review.
> 
> Lack of a response does not mean you can assume consensus. Follow up with
> machine maintainers, even one ACK from a maintainer goes a long way to
> knowing there is support for your change.

JFYI, PTRACE_SEIZE_DEVEL was an architecture-independent constant.

There is absolutely no hurry, though, so let's give machine maintainers
more time to review this obvious change.

From 31ab3850c951c6d4ac887efd82be2cf9b2523654 Mon Sep 17 00:00:00 2001
From: "Dmitry V. Levin" <ldv@altlinux.org>
Date: Tue, 18 Jul 2017 09:23:38 +0000
Subject: [PATCH] sys/ptrace.h: remove obsolete Linux PTRACE_SEIZE_DEVEL constant

Remove enum __ptrace_flags along with the only constant it contains,
PTRACE_SEIZE_DEVEL, from Linux's sys/ptrace.h files.

This flag constant was introduced in Linux by commit v3.1-rc1~308^2~28
as a part of new experimental PTRACE_SEIZE interface.
Later, as PTRACE_SEIZE had lost its experimental status,
this flag was removed from Linux by commit v3.4-rc1~109^2~20.

* sysdeps/unix/sysv/linux/sys/ptrace.h (enum __ptrace_flags,
PTRACE_SEIZE_DEVEL): Remove.
* sysdeps/unix/sysv/linux/aarch64/sys/ptrace.h: Likewise.
* sysdeps/unix/sysv/linux/ia64/sys/ptrace.h: Likewise.
* sysdeps/unix/sysv/linux/powerpc/sys/ptrace.h: Likewise.
* sysdeps/unix/sysv/linux/s390/sys/ptrace.h: Likewise.
* sysdeps/unix/sysv/linux/sparc/sys/ptrace.h: Likewise.
---
 ChangeLog                                    | 10 ++++++++++
 NEWS                                         |  3 ++-
 sysdeps/unix/sysv/linux/aarch64/sys/ptrace.h |  6 ------
 sysdeps/unix/sysv/linux/ia64/sys/ptrace.h    |  6 ------
 sysdeps/unix/sysv/linux/powerpc/sys/ptrace.h |  6 ------
 sysdeps/unix/sysv/linux/s390/sys/ptrace.h    |  6 ------
 sysdeps/unix/sysv/linux/sparc/sys/ptrace.h   |  6 ------
 sysdeps/unix/sysv/linux/sys/ptrace.h         |  6 ------
 8 files changed, 12 insertions(+), 37 deletions(-)
  

On 08/08/2017 12:30 PM, Dmitry V. Levin wrote:
> On Tue, Aug 08, 2017 at 09:20:17AM -0400, Carlos O'Donell wrote:
>> On 08/07/2017 11:33 AM, Dmitry V. Levin wrote:
>>> Hi,
>>>
>>> Looks like among those few who care about sys/ptrace.h nobody feels
>>> experienced enough to review this change, so I'll go forward and commit it.
>>
>> Please tread carefully, and give the machine maintainer time to review, or
>> directly TO: the machine maintainers and ask for review.
>>
>> Lack of a response does not mean you can assume consensus. Follow up with
>> machine maintainers, even one ACK from a maintainer goes a long way to
>> knowing there is support for your change.
> 
> JFYI, PTRACE_SEIZE_DEVEL was an architecture-independent constant.

Agreed, but the patch still touches machine-specific headers.
 
> There is absolutely no hurry, though, so let's give machine maintainers
> more time to review this obvious change.

In this case I think it is perfectly acceptable to track down reviews from
other developers, enough to get consensus that the generic change is OK.
For example if a couple of non-machine maintainers reviewed this an thought
it sensible, then you'd be OK to commit. With machine maintainers perhaps
tweaking this later.
  

On Tue, Aug 08, 2017 at 03:07:13PM -0400, Carlos O'Donell wrote:
> On 08/08/2017 12:30 PM, Dmitry V. Levin wrote:
> > On Tue, Aug 08, 2017 at 09:20:17AM -0400, Carlos O'Donell wrote:
> >> On 08/07/2017 11:33 AM, Dmitry V. Levin wrote:
> >>> Hi,
> >>>
> >>> Looks like among those few who care about sys/ptrace.h nobody feels
> >>> experienced enough to review this change, so I'll go forward and commit it.
> >>
> >> Please tread carefully, and give the machine maintainer time to review, or
> >> directly TO: the machine maintainers and ask for review.
> >>
> >> Lack of a response does not mean you can assume consensus. Follow up with
> >> machine maintainers, even one ACK from a maintainer goes a long way to
> >> knowing there is support for your change.
> > 
> > JFYI, PTRACE_SEIZE_DEVEL was an architecture-independent constant.
> 
> Agreed, but the patch still touches machine-specific headers.

Yes, in exactly the same way as the generic header.

> > There is absolutely no hurry, though, so let's give machine maintainers
> > more time to review this obvious change.
> 
> In this case I think it is perfectly acceptable to track down reviews from
> other developers, enough to get consensus that the generic change is OK.
> For example if a couple of non-machine maintainers reviewed this an thought
> it sensible, then you'd be OK to commit. With machine maintainers perhaps
> tweaking this later.

OK, just don't let the bug slip into glibc-2.27, please.

For those who are not familiar with PTRACE_SEIZE API peculiarities,
here is some background information.

This PTRACE_SEIZE_DEVEL flag shouldn't have been added to sys/ptrace.h
in the first place.

It was added to linux/ptrace.h for the only purpose of testing
PTRACE_SEIZE API which was in an experimental status.
From the very first linux commit (v3.1-rc1~308^2~28) till the very last one
(v3.4-rc1~109^2~20) the definition of PTRACE_SEIZE_DEVEL in linux/ptrace.h
remained unchanged:

#define PTRACE_SEIZE_DEVEL	0x80000000 /* temp flag for development */

Yes, it was just a temporary flag for development.  Those who play
with experimental kernel interfaces marked this way, e.g. strace,
surely did not, do not, and should not rely on sys/ptrace.h
as the source of definitions of ptrace constants.

The person who blindly updated sys/ptrace.h constants by commit
glibc-2.15~430 did the wrong thing.  The presence of PTRACE_SEIZE_DEVEL
in sys/ptrace.h is a bug, it has to be fixed and maybe even backported
to stable branches.

I've created a bug #21928 to track this.
  

On 08/08/2017 09:07 PM, Carlos O'Donell wrote:
> On 08/08/2017 12:30 PM, Dmitry V. Levin wrote:
>> On Tue, Aug 08, 2017 at 09:20:17AM -0400, Carlos O'Donell wrote:
>>> On 08/07/2017 11:33 AM, Dmitry V. Levin wrote:
>>>> Hi,
>>>>
>>>> Looks like among those few who care about sys/ptrace.h nobody feels
>>>> experienced enough to review this change, so I'll go forward and commit it.
>>>
>>> Please tread carefully, and give the machine maintainer time to review, or
>>> directly TO: the machine maintainers and ask for review.
>>>
>>> Lack of a response does not mean you can assume consensus. Follow up with
>>> machine maintainers, even one ACK from a maintainer goes a long way to
>>> knowing there is support for your change.
>>
>> JFYI, PTRACE_SEIZE_DEVEL was an architecture-independent constant.
> 
> Agreed, but the patch still touches machine-specific headers.

I don't think our maintainer process confers exclusive code ownership,
quite the opposite actually.  It's unblock-by-default for the
maintainer, not block-everyone-else.

I looked at Dmitry's changes, and they look good to me.  Please
reconsider your opposition.

Thanks,
Florian
  

On Wed, Aug 9, 2017 at 6:39 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 08/08/2017 09:07 PM, Carlos O'Donell wrote:
>> On 08/08/2017 12:30 PM, Dmitry V. Levin wrote:
>>> On Tue, Aug 08, 2017 at 09:20:17AM -0400, Carlos O'Donell wrote:
>>>> On 08/07/2017 11:33 AM, Dmitry V. Levin wrote:
>>>>> Hi,
>>>>>
>>>>> Looks like among those few who care about sys/ptrace.h nobody feels
>>>>> experienced enough to review this change, so I'll go forward and commit it.
>>>>
>>>> Please tread carefully, and give the machine maintainer time to review, or
>>>> directly TO: the machine maintainers and ask for review.
>>>>
>>>> Lack of a response does not mean you can assume consensus. Follow up with
>>>> machine maintainers, even one ACK from a maintainer goes a long way to
>>>> knowing there is support for your change.
>>>
>>> JFYI, PTRACE_SEIZE_DEVEL was an architecture-independent constant.
>>
>> Agreed, but the patch still touches machine-specific headers.
>
> I don't think our maintainer process confers exclusive code ownership,
> quite the opposite actually.  It's unblock-by-default for the
> maintainer, not block-everyone-else.
>
> I looked at Dmitry's changes, and they look good to me.  Please
> reconsider your opposition.

I also think this patch is good (and appropriate for backport to at
least the 2.26 release branch as well), based on the following checks:

- PTRACE_SEIZE_DEVEL appears nowhere in the current Linux kernel
sources, and lxr.free-electrons.com confirms Dmitry's statement that
it was already gone in version 3.4.0.
- The oldest kernel still supported by glibc (version 3.2.0) already
defines PTRACE_SEIZE (with a different numeric value).
- codesearch.debian.net finds no *uses* whatsoever of this identifier.
The only hits are the definitions in glibc's various versions of
sys/ptrace.h, similar sets of definitions in uClibc's sys/ptrace.h and
what appears to be the equivalent of sys/ptrace.h for the Go language,
a thing called "kernel-patch-viewos" that appears to have a complete
copy of kernel 3.1.5 embedded in it, and a program called "criu" that
deemed it necessary to wrap sys/ptrace.h with its own header that
makes sure every last constant from (some version of) linux/ptrace.h
is available, PTRACE_SEIZE_DEVEL included; the definition is the only
appearance of that identifier in that package.

I also observe that the only reason we have architecture-specific
sys/ptrace.h for Linux is that we haven't gotten around to doing the
work required to pull these constants from uapi/{linux,asm}/ptrace.h.
That doesn't look like it would be hard, only tedious.

I don't know Carlos's mind, but it seems likely to me that he didn't
mean to issue a hard NAK, he just wanted the flood of "nobody reacted
to this patch during the freeze so I'm going to take that as assent"
check-ins to slow down.  I'm'a send another message about that.

zw
  

On 08/09/2017 09:10 AM, Zack Weinberg wrote:
> On Wed, Aug 9, 2017 at 6:39 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> On 08/08/2017 09:07 PM, Carlos O'Donell wrote:
>>> On 08/08/2017 12:30 PM, Dmitry V. Levin wrote:
>>>> On Tue, Aug 08, 2017 at 09:20:17AM -0400, Carlos O'Donell wrote:
>>>>> On 08/07/2017 11:33 AM, Dmitry V. Levin wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Looks like among those few who care about sys/ptrace.h nobody feels
>>>>>> experienced enough to review this change, so I'll go forward and commit it.
>>>>>
>>>>> Please tread carefully, and give the machine maintainer time to review, or
>>>>> directly TO: the machine maintainers and ask for review.
>>>>>
>>>>> Lack of a response does not mean you can assume consensus. Follow up with
>>>>> machine maintainers, even one ACK from a maintainer goes a long way to
>>>>> knowing there is support for your change.
>>>>
>>>> JFYI, PTRACE_SEIZE_DEVEL was an architecture-independent constant.
>>>
>>> Agreed, but the patch still touches machine-specific headers.
>>
>> I don't think our maintainer process confers exclusive code ownership,
>> quite the opposite actually.  It's unblock-by-default for the
>> maintainer, not block-everyone-else.
>>
>> I looked at Dmitry's changes, and they look good to me.  Please
>> reconsider your opposition.

> I don't know Carlos's mind, but it seems likely to me that he didn't
> mean to issue a hard NAK, he just wanted the flood of "nobody reacted
> to this patch during the freeze so I'm going to take that as assent"
> check-ins to slow down.  I'm'a send another message about that.

Exactly.

The maintainer process does not confer exclusive ownership, and if it
sounded like I was advocating for that, then that is not at all the
message I intended.

What I wanted to avoid was the impression that silence implied
consensus, and that caution should be taken when implying such
consensus for patches that touch multiple machines.

I want Dmitry to find *other* reviewers to look over the patch and
give cosensus over the changes.

I'm happy for Dmitry to commit them now that both you (Zack) and
Florian have looked at the patches and consider the changes OK.
Three developers is a good belt-and-suspenders peer review for
multiple-machine header changes.
  

On Wed, Aug 09, 2017 at 10:19:30AM -0400, Carlos O'Donell wrote:
> On 08/09/2017 09:10 AM, Zack Weinberg wrote:
> > On Wed, Aug 9, 2017 at 6:39 AM, Florian Weimer wrote:
> >> On 08/08/2017 09:07 PM, Carlos O'Donell wrote:
> >>> On 08/08/2017 12:30 PM, Dmitry V. Levin wrote:
> >>>> On Tue, Aug 08, 2017 at 09:20:17AM -0400, Carlos O'Donell wrote:
> >>>>> On 08/07/2017 11:33 AM, Dmitry V. Levin wrote:
[...]
> I'm happy for Dmitry to commit them now that both you (Zack) and
> Florian have looked at the patches and consider the changes OK.
> Three developers is a good belt-and-suspenders peer review for
> multiple-machine header changes.

Wow, that was fast, thanks!
Committed.

FIY, here is the timeline of this patch:
Tue, 18 Jul 2017: patch submitted
Wed, 02 Aug 2017: 2.26 released
Wed, 02 Aug 2017: patch ping
Mon, 07 Aug 2017: commit heads-up
Tue, 08 Aug 2017: first objection raised
Wed, 09 Aug 2017: first and second reviews
Wed, 09 Aug 2017: first objection removed
Wed, 09 Aug 2017: patch committed

Note that the ping after release attracted no attention for 5 days,
unlike the commit heads-up that caused all this discussion;
as result, the patch has been reviewed and committed.

Please do not consider this case as an example how one can expedite
a patch review! :)