From owner-freebsd-current@FreeBSD.ORG Fri Nov 5 08:56:28 2010 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id EFEAB106564A for ; Fri, 5 Nov 2010 08:56:28 +0000 (UTC) (envelope-from yanegomi@gmail.com) Received: from mail-wy0-f182.google.com (mail-wy0-f182.google.com [74.125.82.182]) by mx1.freebsd.org (Postfix) with ESMTP id 7EF7A8FC08 for ; Fri, 5 Nov 2010 08:56:28 +0000 (UTC) Received: by wyb34 with SMTP id 34so869335wyb.13 for ; Fri, 05 Nov 2010 01:56:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:sender:received :in-reply-to:references:date:x-google-sender-auth:message-id:subject :from:to:cc:content-type:content-transfer-encoding; bh=Qqg/9aYAqsu/FqT/IWk+dft0GRv9RHcA+K1uqpRlU4g=; b=QWOoaihhbqKiNEegIMfl4ZM9Nge9LGMYC1RMyuE5kFTpDZsjkPHaJwnAX9qdyNgpdI IqhMHhzv4KcxJrcvh68Sch5z6qCzxlZID5MJ9U6Ivh9Uww23q0K1qRLPD0BPTOn5I/pu ZcXRNhuFJnKl15l99RNgoo6dqz05sarSlUGNY= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; b=mHMKpp9ChU2ByDip+7LhHGalfnHiwhn4AzHkgs2jfjIubsV2mqpPXqr1OluhEOPZTU XvtSScJqU2b2MwMJVFBxidBp4LvjiZRv2k5WhQaYXoP4/mzeX/Xe4J/biiw5Fek/gm3L i31P4mDxYW+yEiT8IMxojSKnV8Cf0QTHLMCNU= MIME-Version: 1.0 Received: by 10.216.82.197 with SMTP id o47mr1827368wee.45.1288947387160; Fri, 05 Nov 2010 01:56:27 -0700 (PDT) Sender: yanegomi@gmail.com Received: by 10.216.198.27 with HTTP; Fri, 5 Nov 2010 01:56:27 -0700 (PDT) In-Reply-To: References: <20101024231119.GA2123@mark-laptop-bsd.mark-home> <20101024231642.GB2123@mark-laptop-bsd.mark-home> Date: Fri, 5 Nov 2010 01:56:27 -0700 X-Google-Sender-Auth: 8WhUw9TOsoCyV-H6LQGSa0qUS7U Message-ID: From: Garrett Cooper To: Nick Hibma Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: freebsd-current@freebsd.org, Mark Johnston Subject: Re: [Patch] libfetch - closing the cached FTP connection X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 05 Nov 2010 08:56:29 -0000 On Fri, Nov 5, 2010 at 1:38 AM, Nick Hibma wrote: > Mark, > > My 2 cents: Isn't it more appropriate to set FD_CLOEXEC on the fd? > > =A0 =A0 =A0 =A0fcntl(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 =A0 =A0 =A0 =A0 =A0 =A0 =A0*fetchGetFTP(struct = url *, const char *); >> FILE =A0 =A0 =A0 =A0 =A0*fetchPutFTP(struct url *, const char *); >> int =A0 =A0 =A0 =A0 =A0 =A0fetchStatFTP(struct url *, struct url_stat *,= const char *); >> struct url_ent =A0 =A0 =A0 =A0*fetchListFTP(struct url *, const char *); >> +void =A0 =A0 =A0 =A0 =A0fetchCloseCachedFTP(); >> >> /* Generic functions */ >> FILE =A0 =A0 =A0 =A0 =A0*fetchXGetURL(const char *, struct url_stat *, c= onst 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) >> =A0 =A0 =A0 warnx("fetchListFTP(): not implemented"); >> =A0 =A0 =A0 return (NULL); >> } >> + >> +/* >> + * Force close the cached connection. >> + */ >> +void >> +fetchCloseCachedFTP() >> +{ >> + =A0 =A0 ftp_disconnect(cached_connection); >> +} >> diff --git a/usr.sbin/pkg_install/lib/url.c b/usr.sbin/pkg_install/lib/u= rl.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 k= eep_package) >> =A0 =A0 =A0 printf("tar command returns %d status\n", WEXITSTATUS(pstat)= ); >> =A0 =A0 if (rp && (isatty(0) || Verbose)) >> =A0 =A0 =A0 printf(" Done.\n"); >> + =A0 =A0fetchCloseCachedFTP(); >> =A0 =A0 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