Code review - Issue 600002: blocking on sys_waithttps://breakpad.appspot.com/2013-08-22T15:27:14+00:00rietveld
Message from unknown
2013-05-24T17:02:15+00:00issac.xworkurn:md5:60d083eeb73990815f777ab7265dd585
Message from issac.xwork@gmail.com
2013-05-24T17:02:15+00:00issac.xworkurn:md5:29e4a59a2a1c13245612907620f24097
Message from issac.xwork@gmail.com
2013-05-27T15:06:01+00:00issac.xworkurn:md5:496da8eb94cd55e2f05ca0ae443c55a0
On 2013/05/24 17:02:15, issac.xwork wrote:
anybody take a look at this issue please? or any comment?.
Message from ted.mielczarek@gmail.com
2013-06-06T13:53:35+00:00Ted Mielczarekurn:md5:93973ffe8c3b15851a5493189f468ac6
Hi Isaac,
This patch looks conceptually good, it just needs a little tweaking to be able to land.
https://breakpad.appspot.com/600002/diff/1/src/client/linux/handler/exception_handler.cc
File src/client/linux/handler/exception_handler.cc (right):
https://breakpad.appspot.com/600002/diff/1/src/client/linux/handler/exception_handler.cc#newcode385
src/client/linux/handler/exception_handler.cc:385: * handler for SIGCHLD will make wait() hang forever.
Generally we avoid first-person in comments, although we're not totally consistent about it. Also, capitalize the first word of sentences in comments.
https://breakpad.appspot.com/600002/diff/1/src/client/linux/handler/exception_handler.cc#newcode389
src/client/linux/handler/exception_handler.cc:389: if(sigaction(SIGCHLD,NULL,&childsig_handler) == 0 &&
We avoid calling libc functions directly because of memory safety concerns. You should be using the sys_ wrappers from linux_syscall_support.h, you can look around the rest of this file for examples.
You should be able to simply use sys_sigaction and sys_sigemptyset here.
https://breakpad.appspot.com/600002/diff/1/src/client/linux/handler/exception_handler.cc#newcode396
src/client/linux/handler/exception_handler.cc:396: sigaction(SIGCHLD,&childsig_handler,NULL);
Your indentation here is weird.
Message from issac.xwork@gmail.com
2013-06-16T14:20:03+00:00issac.xworkurn:md5:cabd1ab9f632b78acd3955c1e16b7f0f
https://breakpad.appspot.com/600002/diff/1/src/client/linux/handler/exception_handler.cc
File src/client/linux/handler/exception_handler.cc (right):
https://breakpad.appspot.com/600002/diff/1/src/client/linux/handler/exception_handler.cc#newcode389
src/client/linux/handler/exception_handler.cc:389: if(sigaction(SIGCHLD,NULL,&childsig_handler) == 0 &&
On 2013/06/06 13:53:36, Ted Mielczarek wrote:
> We avoid calling libc functions directly because of memory safety concerns. You
> should be using the sys_ wrappers from linux_syscall_support.h, you can look
> around the rest of this file for examples.
>
> You should be able to simply use sys_sigaction and sys_sigemptyset here.
ok, I noticed that before.
But I saw no use of sys_xxx like function for signal action, even in compromise context, eg: code in ExceptionHandler::SignalHandler()
Message from issac.xwork@gmail.com
2013-06-16T14:31:29+00:00issac.xworkurn:md5:4bdec73923ba2f41861d4973cbc2bcf7
as reviewer required.
Message from unknown
2013-06-16T14:36:28+00:00issac.xworkurn:md5:7f89885296bef28df96ccec3a98f6091
Message from issac.xwork@gmail.com
2013-06-16T14:36:28+00:00issac.xworkurn:md5:a814da321f21337708bd0b7deb715fec
as reviewer required.
Message from issac.xwork@gmail.com
2013-06-16T14:39:30+00:00issac.xworkurn:md5:d5ccb10c444b29164912251709bdf4ac
On 2013/06/06 13:53:35, Ted Mielczarek wrote:
> Hi Isaac,
>
> This patch looks conceptually good, it just needs a little tweaking to be able
> to land.
>
> https://breakpad.appspot.com/600002/diff/1/src/client/linux/handler/exception_handler.cc
> File src/client/linux/handler/exception_handler.cc (right):
>
> https://breakpad.appspot.com/600002/diff/1/src/client/linux/handler/exception_handler.cc#newcode385
> src/client/linux/handler/exception_handler.cc:385: * handler for SIGCHLD will
> make wait() hang forever.
> Generally we avoid first-person in comments, although we're not totally
> consistent about it. Also, capitalize the first word of sentences in comments.
>
> https://breakpad.appspot.com/600002/diff/1/src/client/linux/handler/exception_handler.cc#newcode389
> src/client/linux/handler/exception_handler.cc:389:
> if(sigaction(SIGCHLD,NULL,&childsig_handler) == 0 &&
> We avoid calling libc functions directly because of memory safety concerns. You
> should be using the sys_ wrappers from linux_syscall_support.h, you can look
> around the rest of this file for examples.
>
> You should be able to simply use sys_sigaction and sys_sigemptyset here.
>
> https://breakpad.appspot.com/600002/diff/1/src/client/linux/handler/exception_handler.cc#newcode396
> src/client/linux/handler/exception_handler.cc:396:
> sigaction(SIGCHLD,&childsig_handler,NULL);
> Your indentation here is weird.
Thank you for your review!
I have uploaded a new patch as your review required.
Please help review again.
Message from issac.xwork@gmail.com
2013-08-22T15:27:14+00:00issac.xworkurn:md5:211017db9eef90cd606baebe75f5c60e
Hi, Ted
Can you help review this once again?
Thanks.
On 2013/06/16 14:39:30, issac.xwork wrote:
> On 2013/06/06 13:53:35, Ted Mielczarek wrote:
> > Hi Isaac,
> >
> > This patch looks conceptually good, it just needs a little tweaking to be able
> > to land.
> >
> >
> https://breakpad.appspot.com/600002/diff/1/src/client/linux/handler/exception_handler.cc
> > File src/client/linux/handler/exception_handler.cc (right):
> >
> >
> https://breakpad.appspot.com/600002/diff/1/src/client/linux/handler/exception_handler.cc#newcode385
> > src/client/linux/handler/exception_handler.cc:385: * handler for SIGCHLD will
> > make wait() hang forever.
> > Generally we avoid first-person in comments, although we're not totally
> > consistent about it. Also, capitalize the first word of sentences in comments.
> >
> >
> https://breakpad.appspot.com/600002/diff/1/src/client/linux/handler/exception_handler.cc#newcode389
> > src/client/linux/handler/exception_handler.cc:389:
> > if(sigaction(SIGCHLD,NULL,&childsig_handler) == 0 &&
> > We avoid calling libc functions directly because of memory safety concerns.
> You
> > should be using the sys_ wrappers from linux_syscall_support.h, you can look
> > around the rest of this file for examples.
> >
> > You should be able to simply use sys_sigaction and sys_sigemptyset here.
> >
> >
> https://breakpad.appspot.com/600002/diff/1/src/client/linux/handler/exception_handler.cc#newcode396
> > src/client/linux/handler/exception_handler.cc:396:
> > sigaction(SIGCHLD,&childsig_handler,NULL);
> > Your indentation here is weird.
>
> Thank you for your review!
> I have uploaded a new patch as your review required.
> Please help review again.