Re: Dpi_connect_socket retry

Hi,

On Thu, Dec 20, 2012 at 06:49:49PM +0400, [email protected] wrote:
> 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.

  As   from  the  patch  you  sent  (same  as  before),  and  the
explanation  you  give  (ignoring my explanation), I guess you're
making    conclusions    out    of    reading    the    code   in
Dpi_connect_socket() *only*.

  Have  you  checked what you state as fact? (i.e. that the FD is
leaked  and  not  closed  by  the  CCC). FWIW, I did check before
writing my previous answer.

  Of course I'm human and do make mistakes, so if this is the
case: can you please verify and show how to reproduce the bug?

-- 
  Cheers
  Jorge.-