While reviewing rad_fork, as part of an enhancement I'm working on, I found something questionable. After the call to fork, it tests if child_pid != 0 (to see if the parent is the process executing, I presume). If true, then, the child_pid is added to the thread pool and the thread pool mutex is unlocked. However, if the fork failed, -1 is returned (and stored in child_pid), which I believe will then be added to the thread pool (because -1 != 0 and there isn't an explicit test for fork success/failure). Shouldn't there be a test for fork success (child_pid > 0) before adding to the thread pool? Perhaps this isn't a big deal because fork probably doesn't fail that often. But, it isn't clear to me what would happen in reap_children when it calls waitpid with -1 as this tells waitpid to return status for any child. Am I missing something? If not, and you want me to submit a bug report with the minor code change, let me know..
Garber, Neal wrote:
While reviewing rad_fork, as part of an enhancement I’m working on, I found something questionable.
Most of that code is pretty bad, unfortunately.
After the call to fork, it tests if child_pid != 0 (to see if the parent is the process executing, I presume). If true, then, the child_pid is added to the thread pool and the thread pool mutex is unlocked. However, if the fork failed, -1 is returned (and stored in child_pid), which I believe will then be added to the thread pool (because -1 != 0 and there isn’t an explicit test for fork success/failure). Shouldn’t there be a test for fork success (child_pid > 0) before adding to the thread pool?
Yes.
Perhaps this isn’t a big deal because fork probably doesn’t fail that often. But, it isn’t clear to me what would happen in reap_children when it calls waitpid with -1 as this tells waitpid to return status for any child.
Am I missing something? If not, and you want me to submit a bug report with the minor code change, let me know..
No, just send a patch to the list. Alan DeKok. -- http://deployingradius.com - The web site of the book http://deployingradius.com/blog/ - The blog
No, just send a patch to the list.
I submitted bug 420 with a patch against 1.1.4. Strangely, I couldn't select 1.1.4 from the listbox when creating the bug. For future reference, is the preference to provide patches against the latest stable release (currently 1.1.4) or against CVS head?
On Feb 1, 2007, at 8:41 AM, Garber, Neal wrote:
No, just send a patch to the list.
I submitted bug 420 with a patch against 1.1.4. Strangely, I couldn't select 1.1.4 from the listbox when creating the bug.
1.1.4 is added to the version history on the bugtracker. -Chris -- Chris Parker Director, Engineering US LEC Corp. Internet Telephony & ISP Operations (888)212-0099 Fax (847)963-1302 Wholesale Internet Services http://www.megapop.net VoiceEclipse, The Fresh Alternative http://www.voiceeclipse.com NOTICE: Message is sent IN CONFIDENCE to addressees. It may contain information that is privileged, proprietary or confidential.
On Feb 1, 2007, at 8:41 AM, Garber, Neal wrote:
No, just send a patch to the list.
I submitted bug 420 with a patch against 1.1.4. Strangely, I couldn't select 1.1.4 from the listbox when creating the bug.
For future reference, is the preference to provide patches against the latest stable release (currently 1.1.4) or against CVS head?
CVS Head would be preferred, unless it's a critical security related patch/bug and would call for an immediate new release. -Chris -- Chris Parker Director, Engineering US LEC Corp. Internet Telephony & ISP Operations (888)212-0099 Fax (847)963-1302 Wholesale Internet Services http://www.megapop.net VoiceEclipse, The Fresh Alternative http://www.voiceeclipse.com NOTICE: Message is sent IN CONFIDENCE to addressees. It may contain information that is privileged, proprietary or confidential.
participants (4)
-
Alan DeKok -
Chris Parker -
Garber, Neal -
Joe Maimon