Date: Fri, 5 Nov 2010 01:56:27 -0700 From: Garrett Cooper <gcooper@FreeBSD.org> To: Nick Hibma <nick@van-laarhoven.org> Cc: freebsd-current@freebsd.org, Mark Johnston <markjdb@gmail.com> Subject: Re: [Patch] libfetch - closing the cached FTP connection Message-ID: <AANLkTimaKCKZw=iB6enaPQjwLJQNStp8E1_OXXyqz6b=@mail.gmail.com> In-Reply-To: <A72115A0-00E4-4FDB-974C-E06D31116794@van-laarhoven.org> References: <20101024231119.GA2123@mark-laptop-bsd.mark-home> <20101024231642.GB2123@mark-laptop-bsd.mark-home> <A72115A0-00E4-4FDB-974C-E06D31116794@van-laarhoven.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Nov 5, 2010 at 1:38 AM, Nick Hibma <nick@van-laarhoven.org> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTimaKCKZw=iB6enaPQjwLJQNStp8E1_OXXyqz6b=>