Re: [PATCH 1/4] nptl: Fix Race conditions in pthread cancellation, (BZ#12683)

["Joseph S. Myers" <joseph at codesourcery dot com>]

On Mon, 22 Sep 2014, Adhemerval Zanella wrote:

> >>  #include <sysdep.h>
> >>  
> >> -
> >>  int
> >> -pthread_cancel (th)
> >> -     pthread_t th;
> >> +pthread_cancel (pthread_t th)
> >>  {
> > Likewise, send any such changes of style for function definitions 
> > separately.
> 
> I sincerely don't think we need *another* patch for such changes.  They 
> are fairly mechanical and I think is is safe to use this kinda of 
> extensive patch set to correct this.

The main changes - the ones that actually change how the library behaves, 
and need nontrivial architecture-specific changes, and are the trickiest 
to review - should be kept as small as possible.  That means that anything 
that can reasonably be submitted on its own as a preliminary cleanup 
should be.

> >> diff --git a/sysdeps/unix/sysv/linux/accept4.c b/sysdeps/unix/sysv/linux/accept4.c
> >> index a017224..a02dc35 100644
> >> --- a/sysdeps/unix/sysv/linux/accept4.c
> >> +++ b/sysdeps/unix/sysv/linux/accept4.c
> >> @@ -37,16 +37,8 @@
> >>  int
> >>  accept4 (int fd, __SOCKADDR_ARG addr, socklen_t *addr_len, int flags)
> >>  {
> >> -  if (SINGLE_THREAD_P)
> >> -    return INLINE_SYSCALL (accept4, 4, fd, addr.__sockaddr__, addr_len, flags);
> >> -
> >> -  int oldtype = LIBC_CANCEL_ASYNC ();
> >> -
> >> -  int result = INLINE_SYSCALL (accept4, 4, fd, addr.__sockaddr__, addr_len,
> >> +  int result = SYSCALL_CANCEL (accept4, fd, addr.__sockaddr__, addr_len,
> >>  			       flags);
> >> -
> >> -  LIBC_CANCEL_RESET (oldtype);
> >> -
> > For cases like this, it seems to me that you should be able to do a 
> > preliminary patch that defines SYSCALL_CANCEL in a way that expands to the 
> > existing code using LIBC_CANCEL_ASYNC, INLINE_SYSCALL and 
> > LIBC_CANCEL_RESET.  If you can define some of the new macros like that, 
> > then you can change most or all such code for all architectures to the new 
> > idiom (reasonably safely without being able to test on all the 
> > architectures), reducing the size of the per-architecture patches needed 
> > later and of the patches with the changes that actually fix the main 
> > issue.
> 
> I did think about it and I wasn't sure the direction required for such 
> extensive patch: should we correct only a subset of architecture and let 
> other arch maintainers fix them eventually? Or should we aim to fix all 
> architecture at once? I latter chose on later because it will create 
> less permutations.

The aim is to fix all architectures.  For an essentially mechanical change 
- converting to a new set of macros - that should be feasible.  It is also 
possible to do it bit by bit - because adding the macros doesn't break any 
files using the old macros, it isn't necessary to convery all files in the 
same commit, if you'd like architecture maintainers to test the 
conversions for their architectures.

> I see no problem on work to push for such patch, but it will require 
> more work, more testing, and more.  And I *really* would prefer to focus 
> such efforts on enable other arches for this new system and on the 
> review itself.  For instance, ppc32-fp took me just some hours and 16 
> files changed, 102 insertions(+), 340 deletions(-).

Again, the review is easier if as much as possible goes in preliminary 
patches that pave the way for the main changes.

-- 
Joseph S. Myers
joseph@codesourcery.com