Message ID | 20170808163039.GD4763@altlinux.org |
---|---|
State | New |
Headers | show |
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! :)
diff --git a/NEWS b/NEWS index 4b7e69a..484c467 100644 --- a/NEWS +++ b/NEWS @@ -13,7 +13,8 @@ Major new features: Deprecated and removed features, and other changes affecting compatibility: - [Add deprecations, removals and changes affecting compatibility here] +* On GNU/Linux, the obsolete Linux constant PTRACE_SEIZE_DEVEL is no longer + defined by <sys/ptrace.h>. Changes to build and runtime requirements: diff --git a/sysdeps/unix/sysv/linux/aarch64/sys/ptrace.h b/sysdeps/unix/sysv/linux/aarch64/sys/ptrace.h index c8ca9e3..479696d 100644 --- a/sysdeps/unix/sysv/linux/aarch64/sys/ptrace.h +++ b/sysdeps/unix/sysv/linux/aarch64/sys/ptrace.h @@ -141,12 +141,6 @@ enum __ptrace_request }; -/* Flag for PTRACE_LISTEN. */ -enum __ptrace_flags -{ - PTRACE_SEIZE_DEVEL = 0x80000000 -}; - /* Options set using PTRACE_SETOPTIONS. */ enum __ptrace_setoptions { diff --git a/sysdeps/unix/sysv/linux/ia64/sys/ptrace.h b/sysdeps/unix/sysv/linux/ia64/sys/ptrace.h index c77e6dc..681dc89 100644 --- a/sysdeps/unix/sysv/linux/ia64/sys/ptrace.h +++ b/sysdeps/unix/sysv/linux/ia64/sys/ptrace.h @@ -146,12 +146,6 @@ enum __ptrace_request }; -/* Flag for PTRACE_LISTEN. */ -enum __ptrace_flags -{ - PTRACE_SEIZE_DEVEL = 0x80000000 -}; - /* pt_all_user_regs is used for PTRACE_GETREGS/PTRACE_SETREGS. */ struct __pt_all_user_regs { diff --git a/sysdeps/unix/sysv/linux/powerpc/sys/ptrace.h b/sysdeps/unix/sysv/linux/powerpc/sys/ptrace.h index ed1ed63..b2296fa 100644 --- a/sysdeps/unix/sysv/linux/powerpc/sys/ptrace.h +++ b/sysdeps/unix/sysv/linux/powerpc/sys/ptrace.h @@ -133,12 +133,6 @@ enum __ptrace_request }; -/* Flag for PTRACE_LISTEN. */ -enum __ptrace_flags -{ - PTRACE_SEIZE_DEVEL = 0x80000000 -}; - /* Options set using PTRACE_SETOPTIONS. */ enum __ptrace_setoptions { diff --git a/sysdeps/unix/sysv/linux/s390/sys/ptrace.h b/sysdeps/unix/sysv/linux/s390/sys/ptrace.h index e913647..6c7d86b 100644 --- a/sysdeps/unix/sysv/linux/s390/sys/ptrace.h +++ b/sysdeps/unix/sysv/linux/s390/sys/ptrace.h @@ -210,12 +210,6 @@ enum __ptrace_request }; -/* Flag for PTRACE_LISTEN. */ -enum __ptrace_flags -{ - PTRACE_SEIZE_DEVEL = 0x80000000 -}; - /* Options set using PTRACE_SETOPTIONS. */ enum __ptrace_setoptions { diff --git a/sysdeps/unix/sysv/linux/sparc/sys/ptrace.h b/sysdeps/unix/sysv/linux/sparc/sys/ptrace.h index f605494..1fda17c 100644 --- a/sysdeps/unix/sysv/linux/sparc/sys/ptrace.h +++ b/sysdeps/unix/sysv/linux/sparc/sys/ptrace.h @@ -215,12 +215,6 @@ enum __ptrace_request }; -/* Flag for PTRACE_LISTEN. */ -enum __ptrace_flags -{ - PTRACE_SEIZE_DEVEL = 0x80000000 -}; - /* Options set using PTRACE_SETOPTIONS. */ enum __ptrace_setoptions { diff --git a/sysdeps/unix/sysv/linux/sys/ptrace.h b/sysdeps/unix/sysv/linux/sys/ptrace.h index 1daadd1..6ddd972 100644 --- a/sysdeps/unix/sysv/linux/sys/ptrace.h +++ b/sysdeps/unix/sysv/linux/sys/ptrace.h @@ -163,12 +163,6 @@ enum __ptrace_request }; -/* Flag for PTRACE_LISTEN. */ -enum __ptrace_flags -{ - PTRACE_SEIZE_DEVEL = 0x80000000 -}; - /* Options set using PTRACE_SETOPTIONS. */ enum __ptrace_setoptions {