You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Basant Kukreja <Ba...@Sun.COM> on 2008/02/12 20:32:27 UTC
apr_pollset_poll return value (APR_EINTR) on solaris
Hi,
I am Basant Kukreja. I was working on apache httpd bug 42580.
http://issues.apache.org/bugzilla/show_bug.cgi?id=42580
I figured out that the cause of the problem might be in APR.
apr_pollset_poll function returns APR_TIMEUP even when errno is EINTR. The
caller functions e.g listener_thread (in worker.c) expects this API to return
APR_EINTR if apr_pollset_poll fails.
Other implementation of apr_pollset_poll (as in epoll.c) handles it correctly.
Please provide your comments. Suggested patch is attached. With this patch the
issue 42580 seems to be fixed to me.
Regards,
Basant.
-- orghttpd-2.2.6/srclib/apr/poll/unix/port.c Fri Apr 13 13:54:13 2007
+++ httpd-2.2.6/srclib/apr/poll/unix/port.c Mon Feb 11 14:11:56 2008
@@ -295,7 +295,10 @@
if (ret == -1) {
(*num) = 0;
- if (errno == ETIME || errno == EINTR) {
+ if (errno == EINTR) {
+ rv = APR_EINTR;
+ }
+ else if (errno == ETIME) {
rv = APR_TIMEUP;
}
else {
Re: apr_pollset_poll return value (APR_EINTR) on solaris
Posted by Basant Kukreja <Ba...@Sun.COM>.
I beleive that apr_get_netos_error() is a better choice. APR_EGENERAL is
certainly a poor choice. Other poll methods follow the same approach e.g
epoll.c.
Here is snip from epoll.c :
-------------------------------------------
ret = epoll_wait(pollset->epoll_fd, pollset->pollset, pollset->nalloc,
timeout);
(*num) = ret;
if (ret < 0) {
rv = apr_get_netos_error();
}
else if (ret == 0) {
rv = APR_TIMEUP;
}
-------------------------------------------
Regards,
Basant.
On Tue, Feb 12, 2008 at 10:18:45PM +0200, Lucian Adrian Grijincu wrote:
> This is the code in question.
>
> if (ret == -1) {
> (*num) = 0;
> if (errno == ETIME || errno == EINTR) {
> rv = APR_TIMEUP;
> }
> else {
> rv = APR_EGENERAL;
> }
> }
>
> I don't really like the APR_EGENERAL in the else either.
> Shouldn't this be something like:
> if (ret == -1)
> {
> (*num) = 0;
> rv = apr_get_netos_error();
> }
>
> and let apr_get_netos_error handle OS to APR errors consistent with
> other architectures (select.c, poll.c)
>
> On Feb 12, 2008 9:32 PM, Basant Kukreja <Ba...@sun.com> wrote:
> > Hi,
> > I am Basant Kukreja. I was working on apache httpd bug 42580.
> > http://issues.apache.org/bugzilla/show_bug.cgi?id=42580
> >
> > I figured out that the cause of the problem might be in APR.
> >
> > apr_pollset_poll function returns APR_TIMEUP even when errno is EINTR. The
> > caller functions e.g listener_thread (in worker.c) expects this API to return
> > APR_EINTR if apr_pollset_poll fails.
> >
> > Other implementation of apr_pollset_poll (as in epoll.c) handles it correctly.
> >
> > Please provide your comments. Suggested patch is attached. With this patch the
> > issue 42580 seems to be fixed to me.
> >
> > Regards,
> > Basant.
> >
> >
> > -- orghttpd-2.2.6/srclib/apr/poll/unix/port.c Fri Apr 13 13:54:13 2007
> > +++ httpd-2.2.6/srclib/apr/poll/unix/port.c Mon Feb 11 14:11:56 2008
> > @@ -295,7 +295,10 @@
> >
> > if (ret == -1) {
> > (*num) = 0;
> > - if (errno == ETIME || errno == EINTR) {
> > + if (errno == EINTR) {
> > + rv = APR_EINTR;
> > + }
> > + else if (errno == ETIME) {
> > rv = APR_TIMEUP;
> > }
> > else {
> >
> >
>
>
>
> --
> Lucian
Re: apr_pollset_poll return value (APR_EINTR) on solaris
Posted by Henry Jen <he...@ztune.net>.
For port.c support on Solaris, there is also this outstanding issue:
http://issues.apache.org/bugzilla/show_bug.cgi?id=43000
The patch had been reviewed and waiting for someone to commit.
Cheers,
Henry
On Feb 12, 2008 1:26 PM, Basant Kukreja <Ba...@sun.com> wrote:
> After incorporating the sugestion by Lucian, the following patch works for me.
>
> -----------------------------------------------------------------------------
> --- orghttpd-2.2.6/srclib/apr/poll/unix/port.c Fri Apr 13 13:54:13 2007
> +++ httpd-2.2.6/srclib/apr/poll/unix/port.c Tue Feb 12 13:12:38 2008
> @@ -295,12 +295,7 @@
>
> if (ret == -1) {
> (*num) = 0;
> - if (errno == ETIME || errno == EINTR) {
> - rv = APR_TIMEUP;
> - }
> - else {
> - rv = APR_EGENERAL;
> - }
> + rv = apr_get_netos_error();
> }
> else if (nget == 0) {
> rv = APR_TIMEUP;
> -----------------------------------------------------------------------------
>
> Regards,
> Basant.
>
>
>
> On Tue, Feb 12, 2008 at 04:10:32PM -0500, Jim Jagielski wrote:
> >
> > On Feb 12, 2008, at 3:18 PM, Lucian Adrian Grijincu wrote:
> >
> >> This is the code in question.
> >>
> >> if (ret == -1) {
> >> (*num) = 0;
> >> if (errno == ETIME || errno == EINTR) {
> >> rv = APR_TIMEUP;
> >> }
> >> else {
> >> rv = APR_EGENERAL;
> >> }
> >> }
> >>
> >> I don't really like the APR_EGENERAL in the else either.
> >> Shouldn't this be something like:
> >> if (ret == -1)
> >> {
> >> (*num) = 0;
> >> rv = apr_get_netos_error();
> >> }
> >>
> >> and let apr_get_netos_error handle OS to APR errors consistent with
> >> other architectures (select.c, poll.c)
> >>
> >
> > +1
> >
> > Basant, can you confirm this works as expected?
> >
>
Re: apr_pollset_poll return value (APR_EINTR) on solaris
Posted by Basant Kukreja <Ba...@Sun.COM>.
After incorporating the sugestion by Lucian, the following patch works for me.
-----------------------------------------------------------------------------
--- orghttpd-2.2.6/srclib/apr/poll/unix/port.c Fri Apr 13 13:54:13 2007
+++ httpd-2.2.6/srclib/apr/poll/unix/port.c Tue Feb 12 13:12:38 2008
@@ -295,12 +295,7 @@
if (ret == -1) {
(*num) = 0;
- if (errno == ETIME || errno == EINTR) {
- rv = APR_TIMEUP;
- }
- else {
- rv = APR_EGENERAL;
- }
+ rv = apr_get_netos_error();
}
else if (nget == 0) {
rv = APR_TIMEUP;
-----------------------------------------------------------------------------
Regards,
Basant.
On Tue, Feb 12, 2008 at 04:10:32PM -0500, Jim Jagielski wrote:
>
> On Feb 12, 2008, at 3:18 PM, Lucian Adrian Grijincu wrote:
>
>> This is the code in question.
>>
>> if (ret == -1) {
>> (*num) = 0;
>> if (errno == ETIME || errno == EINTR) {
>> rv = APR_TIMEUP;
>> }
>> else {
>> rv = APR_EGENERAL;
>> }
>> }
>>
>> I don't really like the APR_EGENERAL in the else either.
>> Shouldn't this be something like:
>> if (ret == -1)
>> {
>> (*num) = 0;
>> rv = apr_get_netos_error();
>> }
>>
>> and let apr_get_netos_error handle OS to APR errors consistent with
>> other architectures (select.c, poll.c)
>>
>
> +1
>
> Basant, can you confirm this works as expected?
>
Re: apr_pollset_poll return value (APR_EINTR) on solaris
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Feb 12, 2008, at 3:18 PM, Lucian Adrian Grijincu wrote:
> This is the code in question.
>
> if (ret == -1) {
> (*num) = 0;
> if (errno == ETIME || errno == EINTR) {
> rv = APR_TIMEUP;
> }
> else {
> rv = APR_EGENERAL;
> }
> }
>
> I don't really like the APR_EGENERAL in the else either.
> Shouldn't this be something like:
> if (ret == -1)
> {
> (*num) = 0;
> rv = apr_get_netos_error();
> }
>
> and let apr_get_netos_error handle OS to APR errors consistent with
> other architectures (select.c, poll.c)
>
+1
Basant, can you confirm this works as expected?
Re: apr_pollset_poll return value (APR_EINTR) on solaris
Posted by Lucian Adrian Grijincu <lu...@gmail.com>.
This is the code in question.
if (ret == -1) {
(*num) = 0;
if (errno == ETIME || errno == EINTR) {
rv = APR_TIMEUP;
}
else {
rv = APR_EGENERAL;
}
}
I don't really like the APR_EGENERAL in the else either.
Shouldn't this be something like:
if (ret == -1)
{
(*num) = 0;
rv = apr_get_netos_error();
}
and let apr_get_netos_error handle OS to APR errors consistent with
other architectures (select.c, poll.c)
On Feb 12, 2008 9:32 PM, Basant Kukreja <Ba...@sun.com> wrote:
> Hi,
> I am Basant Kukreja. I was working on apache httpd bug 42580.
> http://issues.apache.org/bugzilla/show_bug.cgi?id=42580
>
> I figured out that the cause of the problem might be in APR.
>
> apr_pollset_poll function returns APR_TIMEUP even when errno is EINTR. The
> caller functions e.g listener_thread (in worker.c) expects this API to return
> APR_EINTR if apr_pollset_poll fails.
>
> Other implementation of apr_pollset_poll (as in epoll.c) handles it correctly.
>
> Please provide your comments. Suggested patch is attached. With this patch the
> issue 42580 seems to be fixed to me.
>
> Regards,
> Basant.
>
>
> -- orghttpd-2.2.6/srclib/apr/poll/unix/port.c Fri Apr 13 13:54:13 2007
> +++ httpd-2.2.6/srclib/apr/poll/unix/port.c Mon Feb 11 14:11:56 2008
> @@ -295,7 +295,10 @@
>
> if (ret == -1) {
> (*num) = 0;
> - if (errno == ETIME || errno == EINTR) {
> + if (errno == EINTR) {
> + rv = APR_EINTR;
> + }
> + else if (errno == ETIME) {
> rv = APR_TIMEUP;
> }
> else {
>
>
--
Lucian
Re: apr_pollset_poll return value (APR_EINTR) on solaris
Posted by Ruediger Pluem <rp...@apache.org>.
On 02/12/2008 08:32 PM, Basant Kukreja wrote:
> Hi,
> I am Basant Kukreja. I was working on apache httpd bug 42580.
> http://issues.apache.org/bugzilla/show_bug.cgi?id=42580
>
> I figured out that the cause of the problem might be in APR.
>
> apr_pollset_poll function returns APR_TIMEUP even when errno is EINTR. The
> caller functions e.g listener_thread (in worker.c) expects this API to return
> APR_EINTR if apr_pollset_poll fails.
>
> Other implementation of apr_pollset_poll (as in epoll.c) handles it correctly.
>
> Please provide your comments. Suggested patch is attached. With this patch the
> issue 42580 seems to be fixed to me.
>
> Regards,
> Basant.
Does the following patch work for you as well?
Index: poll/unix/port.c
===================================================================
--- poll/unix/port.c (Revision 627074)
+++ poll/unix/port.c (Arbeitskopie)
@@ -295,12 +295,7 @@
if (ret == -1) {
(*num) = 0;
- if (errno == ETIME || errno == EINTR) {
- rv = APR_TIMEUP;
- }
- else {
- rv = APR_EGENERAL;
- }
+ rv = apr_get_netos_error();
}
else if (nget == 0) {
rv = APR_TIMEUP;
Regards
Rüdiger