Re: Dpi_connect_socket retry

On Sun, Dec 16, 2012 at 07:43:19PM -0300, Jorge Arellano Cid wrote:
> On Sun, Dec 09, 2012 at 02:58:43PM +0400, [email protected] wrote:
> > Also, comment says "the server closes sock_fd on error". However,
> > sock_fd should still be closed by client to free allocated file
> > descriptor. OS don't know if Dillo is going to read from it (in which
> > case error such as ECONNRESET should happen) so file descriptor can't
> > be reused.
> > 
> > File descriptor should be closed in case of error in a_Dpip_build_cmd
> > too.
> 
>   The  file  descriptor  is properly closed by its CCC afterwards
> (Visible by defining VERBOSE in chain.c).
>
>   The  CCC  manages  the  error propagation/handling, in both the
> requesting and answer branches.

File descriptor is properly closed if there were no errors. But if
there is an error in a_Dpip_build_cmd or Dpi_blocking_write, sock_fd
should be closed before returning from Dpi_connect_socket.

Dpi_connect_socket calls a_Dpip_build_cmd to build command and
Dpi_blocking_write to write this command to sock_fd. a_Dpip_build_cmd
does not accept sock_fd as its argument so it can't close it.
Dpi_blocking_write only writes to fd, but does not close it on error.

So if one of these two function fails, Dpi_connect_socket should close
sock_fd before returning -1. In the current state it just returns -1
and leave sock_fd open.

By the time Dpi_connect_socket returns, sock_fd is lost and will not
be closed by any code outside Dpi_connect_socket.

Attached patch adds code to close sock_fd in case of error.

_______________________________________________
Dillo-dev mailing list
[email protected]
http://lists.auriga.wearlab.de/cgi-bin/mailman/listinfo/dillo-dev

# HG changeset patch
# Parent 3eb0d7fc0c825ccba1ad0b75d81d1dceb227ef72

diff -r 3eb0d7fc0c82 src/IO/dpi.c
--- a/src/IO/dpi.c
+++ b/src/IO/dpi.c
@@ -637,8 +637,6 @@
       perror("[dpi::socket]");
    } else if (connect(sock_fd, (void*)&sin, sizeof(sin)) == -1) {
       MSG("[dpi::connect] errno:%d %s\n", errno, dStrerror(errno));
-
-   /* send authentication Key (the server closes sock_fd on error) */
    } else if (!(cmd = a_Dpip_build_cmd("cmd=%s msg=%s", "auth", SharedKey))) {
       MSG_ERR("[Dpi_connect_socket] Can't make auth message.\n");
    } else if (Dpi_blocking_write(sock_fd, cmd, strlen(cmd)) == -1) {
@@ -646,6 +644,9 @@
    } else {
       ret = sock_fd;
    }
+
+   if (ret == -1)
+       Dpi_close_fd(sock_fd);
    dFree(cmd);
 
    return ret;
diff -r 3eb0d7fc0c82 test/cookies.c
--- a/test/cookies.c
+++ b/test/cookies.c
@@ -391,8 +391,6 @@
       perror("[dpi::socket]");
    } else if (connect(sock_fd, (void*)&sin, sizeof(sin)) == -1) {
       MSG("[dpi::connect] errno:%d %s\n", errno, dStrerror(errno));
-
-   /* send authentication Key (the server closes sock_fd on error) */
    } else if (!(cmd = a_Dpip_build_cmd("cmd=%s msg=%s", "auth", SharedKey))) {
       MSG_ERR("[Dpi_connect_socket] Can't make auth message.\n");
    } else if (Dpi_blocking_write(sock_fd, cmd, strlen(cmd)) == -1) {
@@ -400,6 +398,9 @@
    } else {
       ret = sock_fd;
    }
+
+   if (ret == -1)
+      Dpi_close_fd(sock_fd);
    dFree(cmd);
 
    return ret;