On Fri, Nov 5, 2010 at 1:38 AM, Nick Hibma <n...@van-laarhoven.org> wrote: > 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 >> @@ -109,6 +109,7 @@ 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 >> @@ -1204,3 +1204,12 @@ 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 >> @@ -163,5 +163,6 @@ 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; >> }
There are a lot of corner cases not caught with libpkg and pkg_install that should be due to the fact that ports and pkg_install are spaghetti code. That being said, small patches submitted to portmgr via send-pr are more than welcome as anything that would help improve binary packages is more than welcome. I do agree with Nick though... it's better that the issue be `fixed' in libpkg/pkg_install, not libfetch in this case. Thanks, -Garrett _______________________________________________ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"