You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by rp...@apache.org on 2009/10/04 14:09:58 UTC
svn commit: r821524 - in /apr/apr/trunk: CHANGES include/apr_network_io.h
libapr.dsp network_io/beos/socketcommon.c network_io/os2/socket_util.c
network_io/unix/socket_util.c test/sockchild.c test/testsock.c
Author: rpluem
Date: Sun Oct 4 12:09:57 2009
New Revision: 821524
URL: http://svn.apache.org/viewvc?rev=821524&view=rev
Log:
* Add apr_socket_is_connected to detect whether the remote side of a socket
is still open. The origin of apr_socket_is_connected is r473278 from
http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c
in httpd.
Added:
apr/apr/trunk/network_io/os2/socket_util.c (with props)
apr/apr/trunk/network_io/unix/socket_util.c (with props)
Modified:
apr/apr/trunk/CHANGES
apr/apr/trunk/include/apr_network_io.h
apr/apr/trunk/libapr.dsp
apr/apr/trunk/network_io/beos/socketcommon.c
apr/apr/trunk/test/sockchild.c
apr/apr/trunk/test/testsock.c
Modified: apr/apr/trunk/CHANGES
URL: http://svn.apache.org/viewvc/apr/apr/trunk/CHANGES?rev=821524&r1=821523&r2=821524&view=diff
==============================================================================
--- apr/apr/trunk/CHANGES [utf-8] (original)
+++ apr/apr/trunk/CHANGES [utf-8] Sun Oct 4 12:09:57 2009
@@ -1,6 +1,9 @@
-*- coding: utf-8 -*-
Changes for APR 2.0.0
+ *) Add apr_socket_is_connected to detect whether the remote side of
+ a socket is still open. [Ruediger Pluem]
+
*) Transfer the apr-util spec file contents to apr.spec. [Graham
Leggett]
Modified: apr/apr/trunk/include/apr_network_io.h
URL: http://svn.apache.org/viewvc/apr/apr/trunk/include/apr_network_io.h?rev=821524&r1=821523&r2=821524&view=diff
==============================================================================
--- apr/apr/trunk/include/apr_network_io.h (original)
+++ apr/apr/trunk/include/apr_network_io.h Sun Oct 4 12:09:57 2009
@@ -375,6 +375,12 @@
apr_sockaddr_t *sa);
/**
+ * Check whether the remote side of a socket is still open.
+ * @param socket The socket to check
+ */
+APR_DECLARE(int) apr_socket_is_connected(apr_socket_t *socket);
+
+/**
* Create apr_sockaddr_t from hostname, address family, and port.
* @param sa The new apr_sockaddr_t.
* @param hostname The hostname or numeric address string to resolve/parse, or
Modified: apr/apr/trunk/libapr.dsp
URL: http://svn.apache.org/viewvc/apr/apr/trunk/libapr.dsp?rev=821524&r1=821523&r2=821524&view=diff
==============================================================================
--- apr/apr/trunk/libapr.dsp (original)
+++ apr/apr/trunk/libapr.dsp Sun Oct 4 12:09:57 2009
@@ -436,6 +436,10 @@
# End Source File
# Begin Source File
+SOURCE=.\network_io\unix\socket_util.c
+# End Source File
+# Begin Source File
+
SOURCE=.\network_io\win32\sendrecv.c
# End Source File
# Begin Source File
Modified: apr/apr/trunk/network_io/beos/socketcommon.c
URL: http://svn.apache.org/viewvc/apr/apr/trunk/network_io/beos/socketcommon.c?rev=821524&r1=821523&r2=821524&view=diff
==============================================================================
--- apr/apr/trunk/network_io/beos/socketcommon.c (original)
+++ apr/apr/trunk/network_io/beos/socketcommon.c Sun Oct 4 12:09:57 2009
@@ -3,3 +3,4 @@
#include "../unix/sockets.c"
#include "../unix/sockaddr.c"
#include "../unix/sockopt.c"
+#include "../unix/socket_util.c"
Added: apr/apr/trunk/network_io/os2/socket_util.c
URL: http://svn.apache.org/viewvc/apr/apr/trunk/network_io/os2/socket_util.c?rev=821524&view=auto
==============================================================================
--- apr/apr/trunk/network_io/os2/socket_util.c (added)
+++ apr/apr/trunk/network_io/os2/socket_util.c Sun Oct 4 12:09:57 2009
@@ -0,0 +1 @@
+#include "../unix/socket_util.c"
Propchange: apr/apr/trunk/network_io/os2/socket_util.c
------------------------------------------------------------------------------
svn:eol-style = native
Added: apr/apr/trunk/network_io/unix/socket_util.c
URL: http://svn.apache.org/viewvc/apr/apr/trunk/network_io/unix/socket_util.c?rev=821524&view=auto
==============================================================================
--- apr/apr/trunk/network_io/unix/socket_util.c (added)
+++ apr/apr/trunk/network_io/unix/socket_util.c Sun Oct 4 12:09:57 2009
@@ -0,0 +1,57 @@
+/* Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "apr_network_io.h"
+#include "apr_poll.h"
+
+int apr_socket_is_connected(apr_socket_t *socket)
+{
+ apr_pollfd_t pfds[1];
+ apr_status_t status;
+ apr_int32_t nfds;
+
+ pfds[0].reqevents = APR_POLLIN;
+ pfds[0].desc_type = APR_POLL_SOCKET;
+ pfds[0].desc.s = socket;
+
+ do {
+ status = apr_poll(&pfds[0], 1, &nfds, 0);
+ } while (APR_STATUS_IS_EINTR(status));
+
+ if (status == APR_SUCCESS && nfds == 1 &&
+ pfds[0].rtnevents == APR_POLLIN) {
+ apr_sockaddr_t unused;
+ apr_size_t len = 1;
+ char buf[1];
+ /* The socket might be closed in which case
+ * the poll will return POLLIN.
+ * If there is no data available the socket
+ * is closed.
+ */
+ status = apr_socket_recvfrom(&unused, socket, MSG_PEEK,
+ &buf[0], &len);
+ if (status == APR_SUCCESS && len)
+ return 1;
+ else
+ return 0;
+ }
+ else if (APR_STATUS_IS_EAGAIN(status) || APR_STATUS_IS_TIMEUP(status)) {
+ return 1;
+ }
+ return 0;
+
+}
+
Propchange: apr/apr/trunk/network_io/unix/socket_util.c
------------------------------------------------------------------------------
svn:eol-style = native
Modified: apr/apr/trunk/test/sockchild.c
URL: http://svn.apache.org/viewvc/apr/apr/trunk/test/sockchild.c?rev=821524&r1=821523&r2=821524&view=diff
==============================================================================
--- apr/apr/trunk/test/sockchild.c (original)
+++ apr/apr/trunk/test/sockchild.c Sun Oct 4 12:09:57 2009
@@ -76,5 +76,9 @@
apr_socket_close(sock);
exit((int)length);
}
+ else if (!strcmp("close", argv[1])) {
+ apr_socket_close(sock);
+ exit(0);
+ }
exit(-1);
}
Modified: apr/apr/trunk/test/testsock.c
URL: http://svn.apache.org/viewvc/apr/apr/trunk/test/testsock.c?rev=821524&r1=821523&r2=821524&view=diff
==============================================================================
--- apr/apr/trunk/test/testsock.c (original)
+++ apr/apr/trunk/test/testsock.c Sun Oct 4 12:09:57 2009
@@ -204,6 +204,55 @@
APR_ASSERT_SUCCESS(tc, "Problem closing socket", rv);
}
+static void test_is_connected(abts_case *tc, void *data)
+{
+ apr_status_t rv;
+ apr_socket_t *sock;
+ apr_socket_t *sock2;
+ apr_proc_t proc;
+ apr_size_t length = STRLEN;
+ char datastr[STRLEN];
+
+ sock = setup_socket(tc);
+ if (!sock) return;
+
+ launch_child(tc, &proc, "write", p);
+
+ rv = apr_socket_accept(&sock2, sock, p);
+ APR_ASSERT_SUCCESS(tc, "Problem with receiving connection", rv);
+
+ /* Check that the remote socket is still open */
+ ABTS_INT_EQUAL(tc, 1, apr_socket_is_connected(sock2));
+
+ memset(datastr, 0, STRLEN);
+ apr_socket_recv(sock2, datastr, &length);
+
+ /* Make sure that the server received the data we sent */
+ ABTS_STR_EQUAL(tc, DATASTR, datastr);
+ ABTS_SIZE_EQUAL(tc, strlen(datastr), wait_child(tc, &proc));
+
+ /* The child is dead, so should be the remote socket */
+ ABTS_INT_EQUAL(tc, 0, apr_socket_is_connected(sock2));
+
+ rv = apr_socket_close(sock2);
+ APR_ASSERT_SUCCESS(tc, "Problem closing connected socket", rv);
+
+ launch_child(tc, &proc, "close", p);
+
+ rv = apr_socket_accept(&sock2, sock, p);
+ APR_ASSERT_SUCCESS(tc, "Problem with receiving connection", rv);
+
+ /* The child closed the socket instantly */
+ ABTS_INT_EQUAL(tc, 0, apr_socket_is_connected(sock2));
+ wait_child(tc, &proc);
+
+ rv = apr_socket_close(sock2);
+ APR_ASSERT_SUCCESS(tc, "Problem closing connected socket", rv);
+
+ rv = apr_socket_close(sock);
+ APR_ASSERT_SUCCESS(tc, "Problem closing socket", rv);
+}
+
static void test_timeout(abts_case *tc, void *data)
{
apr_status_t rv;
@@ -351,6 +400,7 @@
abts_run_test(suite, test_create_bind_listen, NULL);
abts_run_test(suite, test_send, NULL);
abts_run_test(suite, test_recv, NULL);
+ abts_run_test(suite, test_is_connected, NULL);
abts_run_test(suite, test_timeout, NULL);
abts_run_test(suite, test_print_addr, NULL);
abts_run_test(suite, test_get_addr, NULL);
Re: svn commit: r821524 - in /apr/apr/trunk: CHANGES
include/apr_network_io.h libapr.dsp network_io/beos/socketcommon.c
network_io/os2/socket_util.c network_io/unix/socket_util.c
test/sockchild.c test/testsock.c
Posted by Joe Orton <jo...@redhat.com>.
On Fri, Oct 09, 2009 at 10:16:45PM +0200, Ruediger Pluem wrote:
> Thanks. General looks fine to me. Just one comment in the code.
> Do you commit?
Thanks for catching the TIMEUP thing - fixed that and committed:
http://svn.apache.org/viewvc?rev=824500&view=rev
with otherwise only minor wording changes to the header.
Regards, Joe
Re: svn commit: r821524 - in /apr/apr/trunk: CHANGES include/apr_network_io.h
libapr.dsp network_io/beos/socketcommon.c network_io/os2/socket_util.c network_io/unix/socket_util.c
test/sockchild.c test/testsock.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 10/09/2009 10:22 AM, Joe Orton wrote:
> On Thu, Oct 08, 2009 at 09:10:12PM +0200, Ruediger Pluem wrote:
>> Sorry for being impatient here, but Joe any comment on the above?
>> I would like to fix this.
>
> Sorry for the delay.
>
> The closest parallel to this function in APR is apr_socket_atmark(). So
> I suggest this function should mirror that one as closely in possible.
>
> How about this instead?
Thanks. General looks fine to me. Just one comment in the code.
Do you commit?
> * Create apr_sockaddr_t from hostname, address family, and port.
> Index: network_io/unix/socket_util.c
> ===================================================================
> --- network_io/unix/socket_util.c (revision 823151)
> +++ network_io/unix/socket_util.c (working copy)
> @@ -17,41 +17,58 @@
> #include "apr_network_io.h"
> #include "apr_poll.h"
>
> -int apr_socket_is_connected(apr_socket_t *socket)
> +apr_status_t apr_socket_atreadeof(apr_socket_t *sock, int *atreadeof)
> {
> apr_pollfd_t pfds[1];
> - apr_status_t status;
> + apr_status_t rv;
> apr_int32_t nfds;
>
> + /* The purpose here is to return APR_SUCCESS only in cases in
> + * which it can be unambiguously determined whether or not the
> + * socket will return EOF on next read. In case of an unexpected
> + * error, return that. */
> +
> pfds[0].reqevents = APR_POLLIN;
> pfds[0].desc_type = APR_POLL_SOCKET;
> - pfds[0].desc.s = socket;
> + pfds[0].desc.s = sock;
>
> do {
> - status = apr_poll(&pfds[0], 1, &nfds, 0);
> - } while (APR_STATUS_IS_EINTR(status));
> + rv = apr_poll(&pfds[0], 1, &nfds, 0);
> + } while (APR_STATUS_IS_EINTR(rv));
>
> - if (status == APR_SUCCESS && nfds == 1 &&
> - pfds[0].rtnevents == APR_POLLIN) {
> + if (rv == APR_TIMEUP) {
Shouldn't we use APR_STATUS_IS_TIMEUP(rv) here?
Regards
Rüdiger
Re: svn commit: r821524 - in /apr/apr/trunk: CHANGES
include/apr_network_io.h libapr.dsp network_io/beos/socketcommon.c
network_io/os2/socket_util.c network_io/unix/socket_util.c
test/sockchild.c test/testsock.c
Posted by Joe Orton <jo...@redhat.com>.
On Thu, Oct 08, 2009 at 09:10:12PM +0200, Ruediger Pluem wrote:
> Sorry for being impatient here, but Joe any comment on the above?
> I would like to fix this.
Sorry for the delay.
The closest parallel to this function in APR is apr_socket_atmark(). So
I suggest this function should mirror that one as closely in possible.
How about this instead?
Index: include/apr_network_io.h
===================================================================
--- include/apr_network_io.h (revision 823151)
+++ include/apr_network_io.h (working copy)
@@ -375,17 +375,19 @@
apr_sockaddr_t *sa);
/**
- * Check whether the send part of the socket is still open on the
- * peer or that there is still data in the socket's read buffer.
- * If this is false the next read on the socket will return APR_EOF.
+ * Determine whether the receive part of the socket has been closed by
+ * the peer (such that the subsequent apr_socket_read call would
+ * return APR_EOF), if the socket's read buffer is empty. This
+ * function does not block waiting for I/O.
+ *
* @param socket The socket to check
- * @remark
- * <PRE>
- * This function does not block on the socket but returns immediately in
- * any case.
- * </PRE>
+ * @param atreadeof If APR_SUCCESS is returned, *atreadeof is set to
+ * non-zero if a subsequent read will return APR_EOF
+ * @return an error is returned if it was not possible to determine the
+ * status, in which case *atreadeof is not changed.
*/
-APR_DECLARE(int) apr_socket_is_connected(apr_socket_t *socket);
+APR_DECLARE(apr_status_t) apr_socket_atreadeof(apr_socket_t *sock,
+ int *atreadeof);
/**
* Create apr_sockaddr_t from hostname, address family, and port.
Index: network_io/unix/socket_util.c
===================================================================
--- network_io/unix/socket_util.c (revision 823151)
+++ network_io/unix/socket_util.c (working copy)
@@ -17,41 +17,58 @@
#include "apr_network_io.h"
#include "apr_poll.h"
-int apr_socket_is_connected(apr_socket_t *socket)
+apr_status_t apr_socket_atreadeof(apr_socket_t *sock, int *atreadeof)
{
apr_pollfd_t pfds[1];
- apr_status_t status;
+ apr_status_t rv;
apr_int32_t nfds;
+ /* The purpose here is to return APR_SUCCESS only in cases in
+ * which it can be unambiguously determined whether or not the
+ * socket will return EOF on next read. In case of an unexpected
+ * error, return that. */
+
pfds[0].reqevents = APR_POLLIN;
pfds[0].desc_type = APR_POLL_SOCKET;
- pfds[0].desc.s = socket;
+ pfds[0].desc.s = sock;
do {
- status = apr_poll(&pfds[0], 1, &nfds, 0);
- } while (APR_STATUS_IS_EINTR(status));
+ rv = apr_poll(&pfds[0], 1, &nfds, 0);
+ } while (APR_STATUS_IS_EINTR(rv));
- if (status == APR_SUCCESS && nfds == 1 &&
- pfds[0].rtnevents == APR_POLLIN) {
+ if (rv == APR_TIMEUP) {
+ /* Read buffer empty -> subsequent reads would block, so,
+ * definitely not at EOF. */
+ *atreadeof = 0;
+ return APR_SUCCESS;
+ }
+ else if (rv) {
+ /* Some other error -> unexpected error. */
+ return rv;
+ }
+ else if (nfds == 1 && pfds[0].rtnevents == APR_POLLIN) {
apr_sockaddr_t unused;
apr_size_t len = 1;
- char buf[1];
- /* The socket might be closed in which case
- * the poll will return POLLIN.
- * If there is no data available the socket
- * is closed.
- */
- status = apr_socket_recvfrom(&unused, socket, MSG_PEEK,
- &buf[0], &len);
- if (status == APR_SUCCESS && len)
- return 1;
- else
- return 0;
+ char buf;
+
+ /* The socket is readable - peek to see whether it returns EOF
+ * without consuming bytes from the socket buffer. */
+ rv = apr_socket_recvfrom(&unused, sock, MSG_PEEK, &buf, &len);
+ if (rv == APR_EOF) {
+ *atreadeof = 1;
+ return APR_SUCCESS;
+ }
+ else if (rv) {
+ /* Read error -> unexpected error. */
+ return rv;
+ }
+ else {
+ *atreadeof = 0;
+ return APR_SUCCESS;
+ }
}
- else if (APR_STATUS_IS_EAGAIN(status) || APR_STATUS_IS_TIMEUP(status)) {
- return 1;
- }
- return 0;
+ /* Should not fall through here. */
+ return APR_EGENERAL;
}
Index: test/testsock.c
===================================================================
--- test/testsock.c (revision 823151)
+++ test/testsock.c (working copy)
@@ -212,6 +212,7 @@
apr_proc_t proc;
apr_size_t length = STRLEN;
char datastr[STRLEN];
+ int atreadeof = -1;
sock = setup_socket(tc);
if (!sock) return;
@@ -222,7 +223,9 @@
APR_ASSERT_SUCCESS(tc, "Problem with receiving connection", rv);
/* Check that the remote socket is still open */
- ABTS_INT_EQUAL(tc, 1, apr_socket_is_connected(sock2));
+ rv = apr_socket_atreadeof(sock2, &atreadeof);
+ APR_ASSERT_SUCCESS(tc, "Determine whether at EOF, #1", rv);
+ ABTS_INT_EQUAL(tc, atreadeof, 0);
memset(datastr, 0, STRLEN);
apr_socket_recv(sock2, datastr, &length);
@@ -232,7 +235,9 @@
ABTS_SIZE_EQUAL(tc, strlen(datastr), wait_child(tc, &proc));
/* The child is dead, so should be the remote socket */
- ABTS_INT_EQUAL(tc, 0, apr_socket_is_connected(sock2));
+ rv = apr_socket_atreadeof(sock2, &atreadeof);
+ APR_ASSERT_SUCCESS(tc, "Determine whether at EOF, #2", rv);
+ ABTS_INT_EQUAL(tc, atreadeof, 1);
rv = apr_socket_close(sock2);
APR_ASSERT_SUCCESS(tc, "Problem closing connected socket", rv);
@@ -243,7 +248,9 @@
APR_ASSERT_SUCCESS(tc, "Problem with receiving connection", rv);
/* The child closed the socket instantly */
- ABTS_INT_EQUAL(tc, 0, apr_socket_is_connected(sock2));
+ rv = apr_socket_atreadeof(sock2, &atreadeof);
+ APR_ASSERT_SUCCESS(tc, "Determine whether at EOF, #3", rv);
+ ABTS_INT_EQUAL(tc, atreadeof, 1);
wait_child(tc, &proc);
rv = apr_socket_close(sock2);
Re: svn commit: r821524 - in /apr/apr/trunk: CHANGES include/apr_network_io.h
libapr.dsp network_io/beos/socketcommon.c network_io/os2/socket_util.c network_io/unix/socket_util.c
test/sockchild.c test/testsock.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 10/08/2009 09:39 PM, Jim Jagielski wrote:
>
> On Oct 8, 2009, at 3:10 PM, Ruediger Pluem wrote:
>
>>
>>
>> On 10/06/2009 08:51 PM, Ruediger Pluem wrote:
>>>
>>> On 10/06/2009 10:52 AM, Joe Orton wrote:
>>
>>>
>>>> or something similar? The API docs should reflect that the return
>>>> value
>>>> is 1-on-success/zero-on-failure (unusual for APR), and that the
>>>> function
>>>> does not block.
>>>
>>> I can invert the return value as well, but in this case shouldn't this
>>> be reflected in the name as well?
>>> So with inverted return value shouldn't it be
>>>
>>> apr_socket_at_read_not_eof
>>>
>>> or do you think I shouldn't return an int at all and return
>>> apr_status_t instead
>>> with APR_SUCCESS if the read side of our end of the socket is eof
>>> (and leaving the name as apr_socket_at_read_eof in this case)?
>>
>> Sorry for being impatient here, but Joe any comment on the above?
>> I would like to fix this.
>>
>> Regards
>>
>> Rüdiger
>>
>
> apr_socket_readable() ?
It is less about the name as such. It is more about whether to return an
int or apr_status_t and what the return value should be in the case that
the peer has closed the socket.
Regards
Rüdiger
Re: svn commit: r821524 - in /apr/apr/trunk: CHANGES include/apr_network_io.h libapr.dsp network_io/beos/socketcommon.c network_io/os2/socket_util.c network_io/unix/socket_util.c test/sockchild.c test/testsock.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Oct 8, 2009, at 3:10 PM, Ruediger Pluem wrote:
>
>
> On 10/06/2009 08:51 PM, Ruediger Pluem wrote:
>>
>> On 10/06/2009 10:52 AM, Joe Orton wrote:
>
>>
>>> or something similar? The API docs should reflect that the return
>>> value
>>> is 1-on-success/zero-on-failure (unusual for APR), and that the
>>> function
>>> does not block.
>>
>> I can invert the return value as well, but in this case shouldn't
>> this
>> be reflected in the name as well?
>> So with inverted return value shouldn't it be
>>
>> apr_socket_at_read_not_eof
>>
>> or do you think I shouldn't return an int at all and return
>> apr_status_t instead
>> with APR_SUCCESS if the read side of our end of the socket is eof
>> (and leaving the name as apr_socket_at_read_eof in this case)?
>
> Sorry for being impatient here, but Joe any comment on the above?
> I would like to fix this.
>
> Regards
>
> Rüdiger
>
apr_socket_readable() ?
Re: svn commit: r821524 - in /apr/apr/trunk: CHANGES include/apr_network_io.h
libapr.dsp network_io/beos/socketcommon.c network_io/os2/socket_util.c network_io/unix/socket_util.c
test/sockchild.c test/testsock.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 10/06/2009 08:51 PM, Ruediger Pluem wrote:
>
> On 10/06/2009 10:52 AM, Joe Orton wrote:
>
>> or something similar? The API docs should reflect that the return value
>> is 1-on-success/zero-on-failure (unusual for APR), and that the function
>> does not block.
>
> I can invert the return value as well, but in this case shouldn't this
> be reflected in the name as well?
> So with inverted return value shouldn't it be
>
> apr_socket_at_read_not_eof
>
> or do you think I shouldn't return an int at all and return apr_status_t instead
> with APR_SUCCESS if the read side of our end of the socket is eof
> (and leaving the name as apr_socket_at_read_eof in this case)?
Sorry for being impatient here, but Joe any comment on the above?
I would like to fix this.
Regards
Rüdiger
Re: svn commit: r821524 - in /apr/apr/trunk: CHANGES
include/apr_network_io.h libapr.dsp network_io/beos/socketcommon.c
network_io/os2/socket_util.c network_io/unix/socket_util.c test/sockchild.c
test/testsock.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Tue, Oct 6, 2009 at 3:10 PM, Jeff Trawick <tr...@gmail.com> wrote:
> On Tue, Oct 6, 2009 at 2:51 PM, Ruediger Pluem <rp...@apache.org> wrote:
>
>>
>>
>> On 10/06/2009 10:52 AM, Joe Orton wrote:
>> > On Sun, Oct 04, 2009 at 12:09:58PM -0000, rpluem@apache.org wrote:
>> >> Author: rpluem
>> >> Date: Sun Oct 4 12:09:57 2009
>> >> New Revision: 821524
>> >>
>> >> URL: http://svn.apache.org/viewvc?rev=821524&view=rev
>> >> Log:
>> >> * Add apr_socket_is_connected to detect whether the remote side of a
>> socket
>> >> is still open. The origin of apr_socket_is_connected is r473278 from
>> >>
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c
>> >> in httpd.
>> >
>> > The naming is not good here. A TCP socket is "connected" after the
>> > connection is successfully established via connect(), up until it is
>> > destroyed. From the name, I would expect this function to do would be
>> > to say whether a non-blocking connect() has completed.
>> >
>> > This function will reliably tell you that the receive part of the socket
>> > has been shut down by the peer, in the case that the socket's read
>> > buffer is empty. Notably the socket may still be "connected" in that
>> > state, albeit in half-duplex mode.
>> >
>> > I'd suggest a name like:
>> >
>> > apr_socket_atreadeof
>> > apr_socket_at_read_eof
>>
>> I am fine with changing the name. I let this thread sit some time
>> to give others the opportunity to chime in as I want to avoid changing
>> the name more than once. If no proposal comes up I am likely to go
>> in the apr_socket_at_read_eof direction.
>>
>> > or something similar? The API docs should reflect that the return value
>> > is 1-on-success/zero-on-failure (unusual for APR), and that the function
>> > does not block.
>>
>> I can invert the return value as well, but in this case shouldn't this
>> be reflected in the name as well?
>> So with inverted return value shouldn't it be
>>
>> apr_socket_at_read_not_eof
>>
>> or do you think I shouldn't return an int at all and return apr_status_t
>> instead
>> with APR_SUCCESS if the read side of our end of the socket is eof
>> (and leaving the name as apr_socket_at_read_eof in this case)?
>>
>
> silly thought from the crowd, in case there are other conditions we'd
> potentially want to report:
>
> if (apr_socket_status_get(sock, &status) != APR_SUCCESS || status &
> APR_SOCK_AT_EOF) {
>
but makes no sense to burn those syscalls in case this is what user was
looking for ;)
retract
Re: svn commit: r821524 - in /apr/apr/trunk: CHANGES
include/apr_network_io.h libapr.dsp network_io/beos/socketcommon.c
network_io/os2/socket_util.c network_io/unix/socket_util.c test/sockchild.c
test/testsock.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Tue, Oct 6, 2009 at 2:51 PM, Ruediger Pluem <rp...@apache.org> wrote:
>
>
> On 10/06/2009 10:52 AM, Joe Orton wrote:
> > On Sun, Oct 04, 2009 at 12:09:58PM -0000, rpluem@apache.org wrote:
> >> Author: rpluem
> >> Date: Sun Oct 4 12:09:57 2009
> >> New Revision: 821524
> >>
> >> URL: http://svn.apache.org/viewvc?rev=821524&view=rev
> >> Log:
> >> * Add apr_socket_is_connected to detect whether the remote side of a
> socket
> >> is still open. The origin of apr_socket_is_connected is r473278 from
> >>
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c
> >> in httpd.
> >
> > The naming is not good here. A TCP socket is "connected" after the
> > connection is successfully established via connect(), up until it is
> > destroyed. From the name, I would expect this function to do would be
> > to say whether a non-blocking connect() has completed.
> >
> > This function will reliably tell you that the receive part of the socket
> > has been shut down by the peer, in the case that the socket's read
> > buffer is empty. Notably the socket may still be "connected" in that
> > state, albeit in half-duplex mode.
> >
> > I'd suggest a name like:
> >
> > apr_socket_atreadeof
> > apr_socket_at_read_eof
>
> I am fine with changing the name. I let this thread sit some time
> to give others the opportunity to chime in as I want to avoid changing
> the name more than once. If no proposal comes up I am likely to go
> in the apr_socket_at_read_eof direction.
>
> > or something similar? The API docs should reflect that the return value
> > is 1-on-success/zero-on-failure (unusual for APR), and that the function
> > does not block.
>
> I can invert the return value as well, but in this case shouldn't this
> be reflected in the name as well?
> So with inverted return value shouldn't it be
>
> apr_socket_at_read_not_eof
>
> or do you think I shouldn't return an int at all and return apr_status_t
> instead
> with APR_SUCCESS if the read side of our end of the socket is eof
> (and leaving the name as apr_socket_at_read_eof in this case)?
>
silly thought from the crowd, in case there are other conditions we'd
potentially want to report:
if (apr_socket_status_get(sock, &status) != APR_SUCCESS || status &
APR_SOCK_AT_EOF) {
/* stop using */
}
Doesn't "EOF" imply "read"?
Re: svn commit: r821524 - in /apr/apr/trunk: CHANGES include/apr_network_io.h
libapr.dsp network_io/beos/socketcommon.c network_io/os2/socket_util.c network_io/unix/socket_util.c
test/sockchild.c test/testsock.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 10/06/2009 10:52 AM, Joe Orton wrote:
> On Sun, Oct 04, 2009 at 12:09:58PM -0000, rpluem@apache.org wrote:
>> Author: rpluem
>> Date: Sun Oct 4 12:09:57 2009
>> New Revision: 821524
>>
>> URL: http://svn.apache.org/viewvc?rev=821524&view=rev
>> Log:
>> * Add apr_socket_is_connected to detect whether the remote side of a socket
>> is still open. The origin of apr_socket_is_connected is r473278 from
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c
>> in httpd.
>
> The naming is not good here. A TCP socket is "connected" after the
> connection is successfully established via connect(), up until it is
> destroyed. From the name, I would expect this function to do would be
> to say whether a non-blocking connect() has completed.
>
> This function will reliably tell you that the receive part of the socket
> has been shut down by the peer, in the case that the socket's read
> buffer is empty. Notably the socket may still be "connected" in that
> state, albeit in half-duplex mode.
>
> I'd suggest a name like:
>
> apr_socket_atreadeof
> apr_socket_at_read_eof
I am fine with changing the name. I let this thread sit some time
to give others the opportunity to chime in as I want to avoid changing
the name more than once. If no proposal comes up I am likely to go
in the apr_socket_at_read_eof direction.
> or something similar? The API docs should reflect that the return value
> is 1-on-success/zero-on-failure (unusual for APR), and that the function
> does not block.
I can invert the return value as well, but in this case shouldn't this
be reflected in the name as well?
So with inverted return value shouldn't it be
apr_socket_at_read_not_eof
or do you think I shouldn't return an int at all and return apr_status_t instead
with APR_SUCCESS if the read side of our end of the socket is eof
(and leaving the name as apr_socket_at_read_eof in this case)?
Regards
Rüdiger
Re: svn commit: r821524 - in /apr/apr/trunk: CHANGES include/apr_network_io.h
libapr.dsp network_io/beos/socketcommon.c network_io/os2/socket_util.c network_io/unix/socket_util.c
test/sockchild.c test/testsock.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 10/06/2009 10:52 AM, Joe Orton wrote:
> The naming is not good here. A TCP socket is "connected" after the
> connection is successfully established via connect(), up until it is
> destroyed. From the name, I would expect this function to do would be
> to say whether a non-blocking connect() has completed.
>
> This function will reliably tell you that the receive part of the socket
> has been shut down by the peer, in the case that the socket's read
> buffer is empty. Notably the socket may still be "connected" in that
> state, albeit in half-duplex mode.
>
> I'd suggest a name like:
>
> apr_socket_atreadeof
> apr_socket_at_read_eof
>
> or something similar? The API docs should reflect that the return value
> is 1-on-success/zero-on-failure (unusual for APR), and that the function
> does not block.
Apart from the other discussion items (return value / name)
I improved the documentation in r822431
http://svn.apache.org/viewvc?rev=822431&view=rev
Regards
Rüdiger
Re: svn commit: r821524 - in /apr/apr/trunk: CHANGES
include/apr_network_io.h libapr.dsp network_io/beos/socketcommon.c
network_io/os2/socket_util.c network_io/unix/socket_util.c
test/sockchild.c test/testsock.c
Posted by Joe Orton <jo...@redhat.com>.
On Sun, Oct 04, 2009 at 12:09:58PM -0000, rpluem@apache.org wrote:
> Author: rpluem
> Date: Sun Oct 4 12:09:57 2009
> New Revision: 821524
>
> URL: http://svn.apache.org/viewvc?rev=821524&view=rev
> Log:
> * Add apr_socket_is_connected to detect whether the remote side of a socket
> is still open. The origin of apr_socket_is_connected is r473278 from
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c
> in httpd.
The naming is not good here. A TCP socket is "connected" after the
connection is successfully established via connect(), up until it is
destroyed. From the name, I would expect this function to do would be
to say whether a non-blocking connect() has completed.
This function will reliably tell you that the receive part of the socket
has been shut down by the peer, in the case that the socket's read
buffer is empty. Notably the socket may still be "connected" in that
state, albeit in half-duplex mode.
I'd suggest a name like:
apr_socket_atreadeof
apr_socket_at_read_eof
or something similar? The API docs should reflect that the return value
is 1-on-success/zero-on-failure (unusual for APR), and that the function
does not block.
Regards, Joe