* [Patch 1/2] ia32: use generic sys_pipe() @ 2009-07-22 9:08 Amerigo Wang 2009-07-22 9:08 ` [Patch 2/2] xtensa: " Amerigo Wang 2009-08-04 12:24 ` [Patch 1/2] ia32: " Ingo Molnar 0 siblings, 2 replies; 11+ messages in thread From: Amerigo Wang @ 2009-07-22 9:08 UTC (permalink / raw) To: linux-kernel; +Cc: akpm, mingo, Al Viro, Amerigo Wang, Linus Torvalds As suggested by Al, it's better to use the generic sys_pipe() for ia32. Signed-off-by: WANG Cong <amwang@redhat.com> Cc: Ingo Molnar <mingo@elte.hu> Cc: Al Viro <viro@zeniv.linux.org.uk> CC: Linus Torvalds <torvalds@linux-foundation.org> --- diff --git a/arch/x86/ia32/ia32entry.S b/arch/x86/ia32/ia32entry.S index e590261..ba331bf 100644 --- a/arch/x86/ia32/ia32entry.S +++ b/arch/x86/ia32/ia32entry.S @@ -537,7 +537,7 @@ ia32_sys_call_table: .quad sys_mkdir .quad sys_rmdir /* 40 */ .quad sys_dup - .quad sys32_pipe + .quad sys_pipe .quad compat_sys_times .quad quiet_ni_syscall /* old prof syscall holder */ .quad sys_brk /* 45 */ diff --git a/arch/x86/ia32/sys_ia32.c b/arch/x86/ia32/sys_ia32.c index 085a8c3..9f55271 100644 --- a/arch/x86/ia32/sys_ia32.c +++ b/arch/x86/ia32/sys_ia32.c @@ -189,20 +189,6 @@ asmlinkage long sys32_mprotect(unsigned long start, size_t len, return sys_mprotect(start, len, prot); } -asmlinkage long sys32_pipe(int __user *fd) -{ - int retval; - int fds[2]; - - retval = do_pipe_flags(fds, 0); - if (retval) - goto out; - if (copy_to_user(fd, fds, sizeof(fds))) - retval = -EFAULT; -out: - return retval; -} - asmlinkage long sys32_rt_sigaction(int sig, struct sigaction32 __user *act, struct sigaction32 __user *oact, unsigned int sigsetsize) ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Patch 2/2] xtensa: use generic sys_pipe() 2009-07-22 9:08 [Patch 1/2] ia32: use generic sys_pipe() Amerigo Wang @ 2009-07-22 9:08 ` Amerigo Wang 2009-07-22 10:10 ` Johannes Weiner 2009-08-04 12:24 ` [Patch 1/2] ia32: " Ingo Molnar 1 sibling, 1 reply; 11+ messages in thread From: Amerigo Wang @ 2009-07-22 9:08 UTC (permalink / raw) To: linux-kernel; +Cc: akpm, Linus Torvalds, Al Viro, mingo, Amerigo Wang As suggested by Al, we can use the generic sys_pipe() instead of xtensa_pipe() for xtensa. Signed-off-by: WANG Cong <amwang@redhat.com> Cc: Al Viro <viro@zeniv.linux.org.uk> CC: Linus Torvalds <torvalds@linux-foundation.org> --- diff --git a/arch/xtensa/include/asm/syscall.h b/arch/xtensa/include/asm/syscall.h index 05cebf8..76a1fb8 100644 --- a/arch/xtensa/include/asm/syscall.h +++ b/arch/xtensa/include/asm/syscall.h @@ -12,7 +12,6 @@ struct pt_regs; struct sigaction; asmlinkage long xtensa_execve(char*, char**, char**, struct pt_regs*); asmlinkage long xtensa_clone(unsigned long, unsigned long, struct pt_regs*); -asmlinkage long xtensa_pipe(int __user *); asmlinkage long xtensa_mmap2(unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long); asmlinkage long xtensa_ptrace(long, long, long, long); diff --git a/arch/xtensa/include/asm/unistd.h b/arch/xtensa/include/asm/unistd.h index c092c8f..b6880c8 100644 --- a/arch/xtensa/include/asm/unistd.h +++ b/arch/xtensa/include/asm/unistd.h @@ -94,7 +94,7 @@ __SYSCALL( 35, sys_readlink, 3) #define __NR_mknod 36 __SYSCALL( 36, sys_mknod, 3) #define __NR_pipe 37 -__SYSCALL( 37, xtensa_pipe, 1) +__SYSCALL(37, sys_pipe, 1) #define __NR_unlink 38 __SYSCALL( 38, sys_unlink, 1) #define __NR_rmdir 39 diff --git a/arch/xtensa/kernel/syscall.c b/arch/xtensa/kernel/syscall.c index ac15ecb..c7279be 100644 --- a/arch/xtensa/kernel/syscall.c +++ b/arch/xtensa/kernel/syscall.c @@ -39,24 +39,6 @@ syscall_t sys_call_table[__NR_syscall_count] /* FIXME __cacheline_aligned */= { #include <asm/unistd.h> }; -/* - * xtensa_pipe() is the normal C calling standard for creating a pipe. It's not - * the way unix traditional does this, though. - */ - -asmlinkage long xtensa_pipe(int __user *userfds) -{ - int fd[2]; - int error; - - error = do_pipe_flags(fd, 0); - if (!error) { - if (copy_to_user(userfds, fd, 2 * sizeof(int))) - error = -EFAULT; - } - return error; -} - asmlinkage long xtensa_mmap2(unsigned long addr, unsigned long len, unsigned long prot, unsigned long flags, ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Patch 2/2] xtensa: use generic sys_pipe() 2009-07-22 9:08 ` [Patch 2/2] xtensa: " Amerigo Wang @ 2009-07-22 10:10 ` Johannes Weiner 2009-07-23 9:47 ` Amerigo Wang 0 siblings, 1 reply; 11+ messages in thread From: Johannes Weiner @ 2009-07-22 10:10 UTC (permalink / raw) To: Amerigo Wang Cc: linux-kernel, akpm, Linus Torvalds, Al Viro, mingo, Chris Zankel [CC Chris] On Wed, Jul 22, 2009 at 05:08:42AM -0400, Amerigo Wang wrote: > > As suggested by Al, we can use the generic sys_pipe() instead of xtensa_pipe() > for xtensa. > > Signed-off-by: WANG Cong <amwang@redhat.com> > Cc: Al Viro <viro@zeniv.linux.org.uk> > CC: Linus Torvalds <torvalds@linux-foundation.org> Reviewed-by: Johannes Weiner <jw@emlix.com> > --- > diff --git a/arch/xtensa/include/asm/syscall.h b/arch/xtensa/include/asm/syscall.h > index 05cebf8..76a1fb8 100644 > --- a/arch/xtensa/include/asm/syscall.h > +++ b/arch/xtensa/include/asm/syscall.h > @@ -12,7 +12,6 @@ struct pt_regs; > struct sigaction; > asmlinkage long xtensa_execve(char*, char**, char**, struct pt_regs*); > asmlinkage long xtensa_clone(unsigned long, unsigned long, struct pt_regs*); > -asmlinkage long xtensa_pipe(int __user *); > asmlinkage long xtensa_mmap2(unsigned long, unsigned long, unsigned long, > unsigned long, unsigned long, unsigned long); > asmlinkage long xtensa_ptrace(long, long, long, long); > diff --git a/arch/xtensa/include/asm/unistd.h b/arch/xtensa/include/asm/unistd.h > index c092c8f..b6880c8 100644 > --- a/arch/xtensa/include/asm/unistd.h > +++ b/arch/xtensa/include/asm/unistd.h > @@ -94,7 +94,7 @@ __SYSCALL( 35, sys_readlink, 3) > #define __NR_mknod 36 > __SYSCALL( 36, sys_mknod, 3) > #define __NR_pipe 37 > -__SYSCALL( 37, xtensa_pipe, 1) > +__SYSCALL(37, sys_pipe, 1) It would have been nice to keep the spacing but that shouldn't be a show stopper.. > #define __NR_unlink 38 > __SYSCALL( 38, sys_unlink, 1) > #define __NR_rmdir 39 > diff --git a/arch/xtensa/kernel/syscall.c b/arch/xtensa/kernel/syscall.c > index ac15ecb..c7279be 100644 > --- a/arch/xtensa/kernel/syscall.c > +++ b/arch/xtensa/kernel/syscall.c > @@ -39,24 +39,6 @@ syscall_t sys_call_table[__NR_syscall_count] /* FIXME __cacheline_aligned */= { > #include <asm/unistd.h> > }; > > -/* > - * xtensa_pipe() is the normal C calling standard for creating a pipe. It's not > - * the way unix traditional does this, though. > - */ > - > -asmlinkage long xtensa_pipe(int __user *userfds) > -{ > - int fd[2]; > - int error; > - > - error = do_pipe_flags(fd, 0); > - if (!error) { > - if (copy_to_user(userfds, fd, 2 * sizeof(int))) > - error = -EFAULT; > - } > - return error; > -} > - > > asmlinkage long xtensa_mmap2(unsigned long addr, unsigned long len, > unsigned long prot, unsigned long flags, Hannes ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Patch 2/2] xtensa: use generic sys_pipe() 2009-07-22 10:10 ` Johannes Weiner @ 2009-07-23 9:47 ` Amerigo Wang 2009-07-23 11:18 ` Johannes Weiner 0 siblings, 1 reply; 11+ messages in thread From: Amerigo Wang @ 2009-07-23 9:47 UTC (permalink / raw) To: Johannes Weiner Cc: linux-kernel, akpm, Linus Torvalds, Al Viro, mingo, Chris Zankel Johannes Weiner wrote: > [CC Chris] > > On Wed, Jul 22, 2009 at 05:08:42AM -0400, Amerigo Wang wrote: > >> As suggested by Al, we can use the generic sys_pipe() instead of xtensa_pipe() >> for xtensa. >> >> Signed-off-by: WANG Cong <amwang@redhat.com> >> Cc: Al Viro <viro@zeniv.linux.org.uk> >> CC: Linus Torvalds <torvalds@linux-foundation.org> >> > > Reviewed-by: Johannes Weiner <jw@emlix.com> > > Thanks for your review. >> --- >> diff --git a/arch/xtensa/include/asm/syscall.h b/arch/xtensa/include/asm/syscall.h >> index 05cebf8..76a1fb8 100644 >> --- a/arch/xtensa/include/asm/syscall.h >> +++ b/arch/xtensa/include/asm/syscall.h >> @@ -12,7 +12,6 @@ struct pt_regs; >> struct sigaction; >> asmlinkage long xtensa_execve(char*, char**, char**, struct pt_regs*); >> asmlinkage long xtensa_clone(unsigned long, unsigned long, struct pt_regs*); >> -asmlinkage long xtensa_pipe(int __user *); >> asmlinkage long xtensa_mmap2(unsigned long, unsigned long, unsigned long, >> unsigned long, unsigned long, unsigned long); >> asmlinkage long xtensa_ptrace(long, long, long, long); >> diff --git a/arch/xtensa/include/asm/unistd.h b/arch/xtensa/include/asm/unistd.h >> index c092c8f..b6880c8 100644 >> --- a/arch/xtensa/include/asm/unistd.h >> +++ b/arch/xtensa/include/asm/unistd.h >> @@ -94,7 +94,7 @@ __SYSCALL( 35, sys_readlink, 3) >> #define __NR_mknod 36 >> __SYSCALL( 36, sys_mknod, 3) >> #define __NR_pipe 37 >> -__SYSCALL( 37, xtensa_pipe, 1) >> +__SYSCALL(37, sys_pipe, 1) >> > > It would have been nice to keep the spacing but that shouldn't be a > show stopper.. > > I did this, but checkpatch.pl complained about this... so I removed the space. Do I need to update and resend the patch? :) Thanks. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Patch 2/2] xtensa: use generic sys_pipe() 2009-07-23 9:47 ` Amerigo Wang @ 2009-07-23 11:18 ` Johannes Weiner 2009-07-24 9:04 ` Amerigo Wang 0 siblings, 1 reply; 11+ messages in thread From: Johannes Weiner @ 2009-07-23 11:18 UTC (permalink / raw) To: Amerigo Wang Cc: linux-kernel, akpm, Linus Torvalds, Al Viro, mingo, Chris Zankel On Thu, Jul 23, 2009 at 05:47:24PM +0800, Amerigo Wang wrote: > Johannes Weiner wrote: > >[CC Chris] > > > >On Wed, Jul 22, 2009 at 05:08:42AM -0400, Amerigo Wang wrote: > > > >>As suggested by Al, we can use the generic sys_pipe() instead of > >>xtensa_pipe() > >>for xtensa. > >> > >>Signed-off-by: WANG Cong <amwang@redhat.com> > >>Cc: Al Viro <viro@zeniv.linux.org.uk> > >>CC: Linus Torvalds <torvalds@linux-foundation.org> > >> > > > >Reviewed-by: Johannes Weiner <jw@emlix.com> > > > > > > Thanks for your review. > >>--- > >>diff --git a/arch/xtensa/include/asm/syscall.h > >>b/arch/xtensa/include/asm/syscall.h > >>index 05cebf8..76a1fb8 100644 > >>--- a/arch/xtensa/include/asm/syscall.h > >>+++ b/arch/xtensa/include/asm/syscall.h > >>@@ -12,7 +12,6 @@ struct pt_regs; > >> struct sigaction; > >> asmlinkage long xtensa_execve(char*, char**, char**, struct pt_regs*); > >> asmlinkage long xtensa_clone(unsigned long, unsigned long, struct > >> pt_regs*); > >>-asmlinkage long xtensa_pipe(int __user *); > >> asmlinkage long xtensa_mmap2(unsigned long, unsigned long, unsigned long, > >> unsigned long, unsigned long, unsigned > >> long); > >> asmlinkage long xtensa_ptrace(long, long, long, long); > >>diff --git a/arch/xtensa/include/asm/unistd.h > >>b/arch/xtensa/include/asm/unistd.h > >>index c092c8f..b6880c8 100644 > >>--- a/arch/xtensa/include/asm/unistd.h > >>+++ b/arch/xtensa/include/asm/unistd.h > >>@@ -94,7 +94,7 @@ __SYSCALL( 35, sys_readlink, 3) > >> #define __NR_mknod 36 > >> __SYSCALL( 36, sys_mknod, 3) > >> #define __NR_pipe 37 > >>-__SYSCALL( 37, xtensa_pipe, 1) > >>+__SYSCALL(37, sys_pipe, 1) > >> > > > >It would have been nice to keep the spacing but that shouldn't be a > >show stopper.. > > > > > > I did this, but checkpatch.pl complained about this... so I removed the > space. Yeah, but look at the file. checkpatch suggests alignment that is common for function calls, but this doesn't look like a sequence of function calls, rather like a table - and we align other tables (like fields in structure definitions) as well because you usually don't read them in a linear fashion but want to spot key values immediately. Please ignore checkpatch in this case. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Patch 2/2] xtensa: use generic sys_pipe() 2009-07-23 11:18 ` Johannes Weiner @ 2009-07-24 9:04 ` Amerigo Wang 0 siblings, 0 replies; 11+ messages in thread From: Amerigo Wang @ 2009-07-24 9:04 UTC (permalink / raw) To: Johannes Weiner Cc: Amerigo Wang, linux-kernel, akpm, Linus Torvalds, Al Viro, mingo, Chris Zankel On Thu, Jul 23, 2009 at 01:18:34PM +0200, Johannes Weiner wrote: >On Thu, Jul 23, 2009 at 05:47:24PM +0800, Amerigo Wang wrote: >> Johannes Weiner wrote: <snip> >> >>--- a/arch/xtensa/include/asm/unistd.h >> >>+++ b/arch/xtensa/include/asm/unistd.h >> >>@@ -94,7 +94,7 @@ __SYSCALL( 35, sys_readlink, 3) >> >> #define __NR_mknod 36 >> >> __SYSCALL( 36, sys_mknod, 3) >> >> #define __NR_pipe 37 >> >>-__SYSCALL( 37, xtensa_pipe, 1) >> >>+__SYSCALL(37, sys_pipe, 1) >> >> >> > >> >It would have been nice to keep the spacing but that shouldn't be a >> >show stopper.. >> > >> > >> >> I did this, but checkpatch.pl complained about this... so I removed the >> space. > >Yeah, but look at the file. checkpatch suggests alignment that is >common for function calls, but this doesn't look like a sequence of >function calls, rather like a table - and we align other tables (like >fields in structure definitions) as well because you usually don't >read them in a linear fashion but want to spot key values immediately. > >Please ignore checkpatch in this case. Ok, thanks! Below is it. ---------------------------> As suggested by Al, we can use the generic sys_pipe() instead of xtensa_pipe() for xtensa. Signed-off-by: WANG Cong <amwang@redhat.com> Reviewed-by: Johannes Weiner <jw@emlix.com> Cc: Al Viro <viro@zeniv.linux.org.uk> CC: Linus Torvalds <torvalds@linux-foundation.org> --- diff --git a/arch/xtensa/include/asm/syscall.h b/arch/xtensa/include/asm/syscall.h index 05cebf8..76a1fb8 100644 --- a/arch/xtensa/include/asm/syscall.h +++ b/arch/xtensa/include/asm/syscall.h @@ -12,7 +12,6 @@ struct pt_regs; struct sigaction; asmlinkage long xtensa_execve(char*, char**, char**, struct pt_regs*); asmlinkage long xtensa_clone(unsigned long, unsigned long, struct pt_regs*); -asmlinkage long xtensa_pipe(int __user *); asmlinkage long xtensa_mmap2(unsigned long, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long); asmlinkage long xtensa_ptrace(long, long, long, long); diff --git a/arch/xtensa/include/asm/unistd.h b/arch/xtensa/include/asm/unistd.h index c092c8f..b6880c8 100644 --- a/arch/xtensa/include/asm/unistd.h +++ b/arch/xtensa/include/asm/unistd.h @@ -94,7 +94,7 @@ __SYSCALL( 35, sys_readlink, 3) #define __NR_mknod 36 __SYSCALL( 36, sys_mknod, 3) #define __NR_pipe 37 -__SYSCALL( 37, xtensa_pipe, 1) +__SYSCALL( 37, sys_pipe, 1) #define __NR_unlink 38 __SYSCALL( 38, sys_unlink, 1) #define __NR_rmdir 39 diff --git a/arch/xtensa/kernel/syscall.c b/arch/xtensa/kernel/syscall.c index ac15ecb..c7279be 100644 --- a/arch/xtensa/kernel/syscall.c +++ b/arch/xtensa/kernel/syscall.c @@ -39,24 +39,6 @@ syscall_t sys_call_table[__NR_syscall_count] /* FIXME __cacheline_aligned */= { #include <asm/unistd.h> }; -/* - * xtensa_pipe() is the normal C calling standard for creating a pipe. It's not - * the way unix traditional does this, though. - */ - -asmlinkage long xtensa_pipe(int __user *userfds) -{ - int fd[2]; - int error; - - error = do_pipe_flags(fd, 0); - if (!error) { - if (copy_to_user(userfds, fd, 2 * sizeof(int))) - error = -EFAULT; - } - return error; -} - asmlinkage long xtensa_mmap2(unsigned long addr, unsigned long len, unsigned long prot, unsigned long flags, ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Patch 1/2] ia32: use generic sys_pipe() 2009-07-22 9:08 [Patch 1/2] ia32: use generic sys_pipe() Amerigo Wang 2009-07-22 9:08 ` [Patch 2/2] xtensa: " Amerigo Wang @ 2009-08-04 12:24 ` Ingo Molnar 2009-08-05 11:38 ` Amerigo Wang 2009-09-12 13:30 ` Al Viro 1 sibling, 2 replies; 11+ messages in thread From: Ingo Molnar @ 2009-08-04 12:24 UTC (permalink / raw) To: Amerigo Wang; +Cc: linux-kernel, akpm, Al Viro, Linus Torvalds * Amerigo Wang <amwang@redhat.com> wrote: > As suggested by Al, it's better to use the generic sys_pipe() for > ia32. > > Signed-off-by: WANG Cong <amwang@redhat.com> > Cc: Ingo Molnar <mingo@elte.hu> > Cc: Al Viro <viro@zeniv.linux.org.uk> > CC: Linus Torvalds <torvalds@linux-foundation.org> > --- a/arch/x86/ia32/sys_ia32.c > +++ b/arch/x86/ia32/sys_ia32.c > @@ -189,20 +189,6 @@ asmlinkage long sys32_mprotect(unsigned long start, size_t len, > return sys_mprotect(start, len, prot); > } > > -asmlinkage long sys32_pipe(int __user *fd) > -{ > - int retval; > - int fds[2]; > - > - retval = do_pipe_flags(fds, 0); > - if (retval) > - goto out; > - if (copy_to_user(fd, fds, sizeof(fds))) > - retval = -EFAULT; > -out: > - return retval; > -} Please _ALWAYS_ mention the change in behavior in the changelog, just in case someone ends up bisecting it. I only found out when i reviewed the two syscalls out of caution. The syscall you remove kept stale fd's around in case of -EFAULT from copy_to_user(). The generic version does an explicit close of those files: sys_close(fd[0]); sys_close(fd[1]); error = -EFAULT; The generic version looks like the better choice to me but this difference should be mentioned in the changelog nevertheless, just in case some buggy app runs into this issue. Ingo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Patch 1/2] ia32: use generic sys_pipe() 2009-08-04 12:24 ` [Patch 1/2] ia32: " Ingo Molnar @ 2009-08-05 11:38 ` Amerigo Wang 2009-09-12 13:30 ` Al Viro 1 sibling, 0 replies; 11+ messages in thread From: Amerigo Wang @ 2009-08-05 11:38 UTC (permalink / raw) To: Ingo Molnar; +Cc: linux-kernel, akpm, Al Viro, Linus Torvalds Ingo Molnar wrote: > * Amerigo Wang <amwang@redhat.com> wrote: > > >> As suggested by Al, it's better to use the generic sys_pipe() for >> ia32. >> >> Signed-off-by: WANG Cong <amwang@redhat.com> >> Cc: Ingo Molnar <mingo@elte.hu> >> Cc: Al Viro <viro@zeniv.linux.org.uk> >> CC: Linus Torvalds <torvalds@linux-foundation.org> >> > > >> --- a/arch/x86/ia32/sys_ia32.c >> +++ b/arch/x86/ia32/sys_ia32.c >> @@ -189,20 +189,6 @@ asmlinkage long sys32_mprotect(unsigned long start, size_t len, >> return sys_mprotect(start, len, prot); >> } >> >> -asmlinkage long sys32_pipe(int __user *fd) >> -{ >> - int retval; >> - int fds[2]; >> - >> - retval = do_pipe_flags(fds, 0); >> - if (retval) >> - goto out; >> - if (copy_to_user(fd, fds, sizeof(fds))) >> - retval = -EFAULT; >> -out: >> - return retval; >> -} >> > > Please _ALWAYS_ mention the change in behavior in the changelog, > just in case someone ends up bisecting it. I only found out when i > reviewed the two syscalls out of caution. > > The syscall you remove kept stale fd's around in case of -EFAULT > from copy_to_user(). The generic version does an explicit close of > those files: > > sys_close(fd[0]); > sys_close(fd[1]); > error = -EFAULT; > > The generic version looks like the better choice to me but this > difference should be mentioned in the changelog nevertheless, just > in case some buggy app runs into this issue. > Hi, yes, exactly. Thank you! I will be careful next time. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Patch 1/2] ia32: use generic sys_pipe() 2009-08-04 12:24 ` [Patch 1/2] ia32: " Ingo Molnar 2009-08-05 11:38 ` Amerigo Wang @ 2009-09-12 13:30 ` Al Viro 2009-09-12 14:48 ` Al Viro 2009-09-18 9:46 ` Ingo Molnar 1 sibling, 2 replies; 11+ messages in thread From: Al Viro @ 2009-09-12 13:30 UTC (permalink / raw) To: Ingo Molnar; +Cc: Amerigo Wang, linux-kernel, akpm, Linus Torvalds On Tue, Aug 04, 2009 at 02:24:32PM +0200, Ingo Molnar wrote: > Please _ALWAYS_ mention the change in behavior in the changelog, > just in case someone ends up bisecting it. I only found out when i > reviewed the two syscalls out of caution. > > The syscall you remove kept stale fd's around in case of -EFAULT > from copy_to_user(). The generic version does an explicit close of > those files: > > sys_close(fd[0]); > sys_close(fd[1]); > error = -EFAULT; > The generic version looks like the better choice to me but this > difference should be mentioned in the changelog nevertheless, just > in case some buggy app runs into this issue. It's not a matter of QOI, actually - sys32_pipe() is supposed to do what sys_pipe() would do on i386 host. So any difference in handling of an error case is simply wrong. Whether we want those sys_close() in sys_pipe() or not is a separate question, but we definitely want the same behaviour when 32bit process is run natively and when it's run on amd64. So sys32_pipe() has no business existing at all. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Patch 1/2] ia32: use generic sys_pipe() 2009-09-12 13:30 ` Al Viro @ 2009-09-12 14:48 ` Al Viro 2009-09-18 9:46 ` Ingo Molnar 1 sibling, 0 replies; 11+ messages in thread From: Al Viro @ 2009-09-12 14:48 UTC (permalink / raw) To: Ingo Molnar; +Cc: Amerigo Wang, linux-kernel, akpm, Linus Torvalds On Sat, Sep 12, 2009 at 02:30:14PM +0100, Al Viro wrote: > On Tue, Aug 04, 2009 at 02:24:32PM +0200, Ingo Molnar wrote: > > > Please _ALWAYS_ mention the change in behavior in the changelog, > > just in case someone ends up bisecting it. I only found out when i > > reviewed the two syscalls out of caution. [snip] > > The generic version looks like the better choice to me but this > > difference should be mentioned in the changelog nevertheless, just > > in case some buggy app runs into this issue. > > It's not a matter of QOI, actually - sys32_pipe() is supposed to do what > sys_pipe() would do on i386 host. So any difference in handling of an > error case is simply wrong. > > Whether we want those sys_close() in sys_pipe() or not is a separate > question, but we definitely want the same behaviour when 32bit process is > run natively and when it's run on amd64. So sys32_pipe() has no business > existing at all. PS: said that, mentioning the change in commit message is a Good Idea(tm), obviously... ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Patch 1/2] ia32: use generic sys_pipe() 2009-09-12 13:30 ` Al Viro 2009-09-12 14:48 ` Al Viro @ 2009-09-18 9:46 ` Ingo Molnar 1 sibling, 0 replies; 11+ messages in thread From: Ingo Molnar @ 2009-09-18 9:46 UTC (permalink / raw) To: Al Viro; +Cc: Amerigo Wang, linux-kernel, akpm, Linus Torvalds * Al Viro <viro@ZenIV.linux.org.uk> wrote: > On Tue, Aug 04, 2009 at 02:24:32PM +0200, Ingo Molnar wrote: > > > Please _ALWAYS_ mention the change in behavior in the changelog, > > just in case someone ends up bisecting it. I only found out when i > > reviewed the two syscalls out of caution. > > > > The syscall you remove kept stale fd's around in case of -EFAULT > > from copy_to_user(). The generic version does an explicit close of > > those files: > > > > sys_close(fd[0]); > > sys_close(fd[1]); > > error = -EFAULT; > > > The generic version looks like the better choice to me but this > > difference should be mentioned in the changelog nevertheless, just > > in case some buggy app runs into this issue. > > It's not a matter of QOI, actually - sys32_pipe() is supposed to do > what sys_pipe() would do on i386 host. So any difference in handling > of an error case is simply wrong. > > Whether we want those sys_close() in sys_pipe() or not is a separate > question, but we definitely want the same behaviour when 32bit process > is run natively and when it's run on amd64. So sys32_pipe() has no > business existing at all. I generally agree except the notion that we 'must' do this change. This is a (small) past ABI sin of ours and we have to live with the consequences without breaking apps. We didnt break any it appears so it's all good. Ingo ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2009-09-18 9:46 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-07-22 9:08 [Patch 1/2] ia32: use generic sys_pipe() Amerigo Wang 2009-07-22 9:08 ` [Patch 2/2] xtensa: " Amerigo Wang 2009-07-22 10:10 ` Johannes Weiner 2009-07-23 9:47 ` Amerigo Wang 2009-07-23 11:18 ` Johannes Weiner 2009-07-24 9:04 ` Amerigo Wang 2009-08-04 12:24 ` [Patch 1/2] ia32: " Ingo Molnar 2009-08-05 11:38 ` Amerigo Wang 2009-09-12 13:30 ` Al Viro 2009-09-12 14:48 ` Al Viro 2009-09-18 9:46 ` Ingo Molnar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox