Re: [Patch] libfetch - closing the cached FTP connection

Mark,

My 2 cents: Isn't it more appropriate to set FD_CLOEXEC on the fd?

	fcntl(fd, F_SETFD, FD_CLOEXEC); 

It doesn't sound like you ever want to have a cached connection be copied into the child. Mum and child calling daddy using the same phone line isn't going to make the conversation any easier for daddy.

Cheers,

Nick

On 25 Oct 2010, at 01:16, Mark Johnston wrote:

> Hello,
> 
> We've run into problems with pkg_add because of some caching behaviour
> in libfetch. Specifically, after an FTP transfer, a connection to the
> FTP server is held open by the cached_connection pointer in ftp.c. Thus,
> if one requests a file with fetchGetFTP() and later closes the
> connection with fclose(), a socket is still held open, and the
> descriptor is copied to any child processes.
> 
> What was apparently happening was that we were using pkg_add to install
> a package whose install script started a daemon, which consequently kept
> open a connection to our FTP server. This is "fixed" in our tree with a
> closefrom(2) in pkg_install at an appropriate point, but I thought that
> libfetch should provide some way of forcing a close on the cached
> connection so that the above hack isn't necessary.
> 
> My solution is provided in a patch below. It's not particularly elegant,
> but I can't see a better way to go about it. I was hoping to get some
> feedback and to see if anyone can come up with a better approach. I'll
> also update the libfetch man page if the patch below is acceptable.
> 
> Thanks,
> -Mark
> 
> diff --git a/lib/libfetch/fetch.h b/lib/libfetch/fetch.h
> index 11a3f77..d379e63 100644
> --- a/lib/libfetch/fetch.h
> +++ b/lib/libfetch/fetch.h
> _at__at_ -109,6 +109,7 _at__at_ FILE		*fetchGetFTP(struct url *, const char *);
> FILE		*fetchPutFTP(struct url *, const char *);
> int		 fetchStatFTP(struct url *, struct url_stat *, const char *);
> struct url_ent	*fetchListFTP(struct url *, const char *);
> +void		 fetchCloseCachedFTP();
> 
> /* Generic functions */
> FILE		*fetchXGetURL(const char *, struct url_stat *, const char *);
> diff --git a/lib/libfetch/ftp.c b/lib/libfetch/ftp.c
> index 0983a76..746ad54 100644
> --- a/lib/libfetch/ftp.c
> +++ b/lib/libfetch/ftp.c
> _at__at_ -1204,3 +1204,12 _at__at_ fetchListFTP(struct url *url __unused, const char *flags __unused)
> 	warnx("fetchListFTP(): not implemented");
> 	return (NULL);
> }
> +
> +/*
> + * Force close the cached connection.
> + */
> +void
> +fetchCloseCachedFTP()
> +{
> +	ftp_disconnect(cached_connection);
> +}
> diff --git a/usr.sbin/pkg_install/lib/url.c b/usr.sbin/pkg_install/lib/url.c
> index 914ce39..68f31bb 100644
> --- a/usr.sbin/pkg_install/lib/url.c
> +++ b/usr.sbin/pkg_install/lib/url.c
> _at__at_ -163,5 +163,6 _at__at_ fileGetURL(const char *base, const char *spec, int keep_package)
> 	printf("tar command returns %d status\n", WEXITSTATUS(pstat));
>     if (rp && (isatty(0) || Verbose))
> 	printf(" Done.\n");
> +    fetchCloseCachedFTP();
>     return rp;
> }
> _______________________________________________
> freebsd-current_at_freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-current
> To unsubscribe, send any mail to "freebsd-current-unsubscribe_at_freebsd.org"