You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ben Collins-Sussman <su...@collab.net> on 2001/09/18 21:08:33 UTC
tmpfile callback problems.
So I'm trying to modify ra_dav to use the new callback for creating
and destroying tmpfiles (in SVN/tmp/)
The callbacks vtable looks like:
typedef struct svn_ra_callbacks_t
{
/* Open a unique temporary file for writing in the working copy.
This file will be automatically deleted when FP is closed.
The full path to the file is returned in *FILENAME. */
svn_error_t *(*open_tmp_file) (apr_file_t **fp,
svn_stringbuf_t **filename,
void *callback_baton);
/* Retrieve an AUTHENTICATOR/AUTH_BATON pair from the client,
which represents the protocol METHOD. */
svn_error_t *(*get_authenticator) (void **authenticator,
void **auth_baton,
apr_uint64_t method,
void *callback_baton,
apr_pool_t *pool);
} svn_ra_callbacks_t;
Notice that instead of a delete_tmp_file() callback, open_tmp_file
simply adds the APR_DELONCLOSE flag. So calling apr_file_close(fp) is
all ra_dav needs to do. (open_tmp_file also calls the thread-safe
svn_io_open_unique_file() under the hood, so ra_dav doesn't have to
anymore.)
So now I discover ickiness... apparently in both the commit and update
cases, ra_dav needs to hand a vanilla "FILE *" version of the tmpfile
to neon; an apr_file_t won't do. So ra_dav closes the apr_file_t,
then fopen()/fclose()'s the file for neon.
Being ignorant, I say, "this is bad, because of delete-on-close". So
I move the apr_file_close() -after- all the fopen/fclose business (for
example, I do this in commit.c:commit_stream_close).
Still though -- I get a segfault when committing, specifically in
commit_stream_close, because fopen() tries to reopen the tmpfile, and
it's already gone somehow.
gstein or joe, can you give me a clue?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: tmpfile callback problems.
Posted by Philip Martin <ph...@ntlworld.com>.
Oops, forgot the list.
Ben Collins-Sussman <su...@collab.net> writes:
> Notice that instead of a delete_tmp_file() callback, open_tmp_file
> simply adds the APR_DELONCLOSE flag. So calling apr_file_close(fp) is
> all ra_dav needs to do. (open_tmp_file also calls the thread-safe
> svn_io_open_unique_file() under the hood, so ra_dav doesn't have to
> anymore.)
If apr_file_open() is passed APR_DELONCLOSE it calls unlink() on the
newly opened file. This deletes the name from the filesystem. The OS
will then delete the file when all the file descriptors are closed.
>
> So now I discover ickiness... apparently in both the commit and update
> cases, ra_dav needs to hand a vanilla "FILE *" version of the tmpfile
> to neon; an apr_file_t won't do. So ra_dav closes the apr_file_t,
> then fopen()/fclose()'s the file for neon.
>
> Being ignorant, I say, "this is bad, because of delete-on-close". So
> I move the apr_file_close() -after- all the fopen/fclose business (for
> example, I do this in commit.c:commit_stream_close).
fopen() will fail because the name has gone.
>
> Still though -- I get a segfault when committing, specifically in
> commit_stream_close, because fopen() tries to reopen the tmpfile, and
> it's already gone somehow.
Yup!
Even if you don't use APR_DELONCLOSE reopening with the filename is a
*really* bad idea, you are introducing a race. The file might get
renamed between the two opens, and another file with the original name
may get created.
Philip
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Neon and APR WAS: RE: tmpfile callback problems.
Posted by Joe Orton <jo...@manyfish.co.uk>.
On Wed, Sep 19, 2001 at 01:53:31AM +0200, Sander Striker wrote:
> Well, I was thinking something more radical: use of pools, registering
> cleanups etc. The temp file problem being solved is just a side-effect
> of consistent usage of apr. If this is too much, I'll go and do something
> else. This just seemed a good project to get into the inner workings of
> neon aswell :)
Mmmm, yeah, that sounds *way* too radical for me :)
joe
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Neon and APR
Posted by Greg Stein <gs...@lyra.org>.
On Wed, Sep 19, 2001 at 12:45:27AM +0100, Joe Orton wrote:
> On Wed, Sep 19, 2001 at 01:12:10AM +0200, Sander Striker wrote:
> > > Time for that use-APR-in-Neon patch :-)
> >
> > Well, I'll take that task*, together with getting client auth ssl working
> > in subversion (currently I tripped over another segv in mod_ssl :( ).
> >
> > *) If people want me to. Joe, Greg, thoughts?
>
> Personally I'm not that interested in seeing neon use APR, though I
> might apply a patch if it's squeaky clean
Well, of course it would be :-)
> and I can be convinced there are no licensing issues
There are no compatibility problems between the ASF license and the (L)GPL.
> (it will of course have to be an optional layer;
> neon will always work without APR). It would mean more complexity, and
> in this case, only to work around a deeper API issue, the need for this
> temp file in the first place.
I've been mentioning it for about a year now. It would solve quite a few
portability things; not just the temp file issue.
As with Sander, I'd see a broad use of the various APR types such as pools,
hash tables, files, and sockets.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: Neon and APR WAS: RE: tmpfile callback problems.
Posted by Sander Striker <st...@apache.org>.
> From: Joe Orton [mailto:joe@manyfish.co.uk]
> Sent: 19 September 2001 01:45
> On Wed, Sep 19, 2001 at 01:12:10AM +0200, Sander Striker wrote:
> > > Time for that use-APR-in-Neon patch :-)
> >
> > Well, I'll take that task*, together with getting client auth
> ssl working
> > in subversion (currently I tripped over another segv in mod_ssl :( ).
> >
> > *) If people want me to. Joe, Greg, thoughts?
>
> Personally I'm not that interested in seeing neon use APR, though I
> might apply a patch if it's squeaky clean and I can be convinced there
> are no licensing issues (it will of course have to be an optional layer;
> neon will always work without APR). It would mean more complexity, and
> in this case, only to work around a deeper API issue, the need for this
> temp file in the first place.
>
> joe
Well, I was thinking something more radical: use of pools, registering
cleanups etc. The temp file problem being solved is just a side-effect
of consistent usage of apr. If this is too much, I'll go and do something
else. This just seemed a good project to get into the inner workings of
neon aswell :)
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Neon and APR WAS: RE: tmpfile callback problems.
Posted by Joe Orton <jo...@manyfish.co.uk>.
On Wed, Sep 19, 2001 at 01:12:10AM +0200, Sander Striker wrote:
> > Time for that use-APR-in-Neon patch :-)
>
> Well, I'll take that task*, together with getting client auth ssl working
> in subversion (currently I tripped over another segv in mod_ssl :( ).
>
> *) If people want me to. Joe, Greg, thoughts?
Personally I'm not that interested in seeing neon use APR, though I
might apply a patch if it's squeaky clean and I can be convinced there
are no licensing issues (it will of course have to be an optional layer;
neon will always work without APR). It would mean more complexity, and
in this case, only to work around a deeper API issue, the need for this
temp file in the first place.
joe
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Neon and APR WAS: RE: tmpfile callback problems.
Posted by Sander Striker <st...@apache.org>.
> From: Greg Stein [mailto:gstein@lyra.org]
> Sent: 19 September 2001 00:40
[...]
>> An apr_file_t can't be
>> (portably) morphed into anything useful to neon AFAIK.
>
> Actually, you can... look at the (misnamed) apr_portable.h header file.
> There is a way to get the file descriptor out of the apr_file_t.
>
> However, I suspect that will return the HANDLE on Windows, which will be
> wrong for Neon. Need to look a bit more.
>
> Time for that use-APR-in-Neon patch :-)
Well, I'll take that task*, together with getting client auth ssl working
in subversion (currently I tripped over another segv in mod_ssl :( ).
*) If people want me to. Joe, Greg, thoughts?
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: tmpfile callback problems.
Posted by Greg Stein <gs...@lyra.org>.
Ben and I talked about this over AIM (gstein67, if you wonder). We resolved
the obvious issues, but it appears something a little more ugly is still
happening in there. Ben is going to debug some more tomorrow.
On Tue, Sep 18, 2001 at 10:25:16PM +0100, Joe Orton wrote:
> On Tue, Sep 18, 2001 at 04:08:33PM -0500, Ben Collins-Sussman wrote:
> ...
> > So now I discover ickiness... apparently in both the commit and update
> > cases, ra_dav needs to hand a vanilla "FILE *" version of the tmpfile
> > to neon; an apr_file_t won't do. So ra_dav closes the apr_file_t,
> > then fopen()/fclose()'s the file for neon.
>
> neon actually takes an integer fd in 0.15, so the FILE * is
> historical... but this doesn't help you.
We figured that one out after a bit :-)
> An apr_file_t can't be
> (portably) morphed into anything useful to neon AFAIK.
Actually, you can... look at the (misnamed) apr_portable.h header file.
There is a way to get the file descriptor out of the apr_file_t.
However, I suspect that will return the HANDLE on Windows, which will be
wrong for Neon. Need to look a bit more.
Time for that use-APR-in-Neon patch :-)
>...
> > Still though -- I get a segfault when committing, specifically in
> > commit_stream_close, because fopen() tries to reopen the tmpfile, and
> > it's already gone somehow.
>
> Yeah, I think that's how it works... because it's been unlink()ed by APR
> to implement DELONCLOSE, it no longer has a name in the filesystem. So
> you can't open it again. :(
Yup.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: tmpfile callback problems.
Posted by Joe Orton <jo...@manyfish.co.uk>.
On Tue, Sep 18, 2001 at 04:08:33PM -0500, Ben Collins-Sussman wrote:
...
> So now I discover ickiness... apparently in both the commit and update
> cases, ra_dav needs to hand a vanilla "FILE *" version of the tmpfile
> to neon; an apr_file_t won't do. So ra_dav closes the apr_file_t,
> then fopen()/fclose()'s the file for neon.
neon actually takes an integer fd in 0.15, so the FILE * is
historical... but this doesn't help you. An apr_file_t can't be
(portably) morphed into anything useful to neon AFAIK.
> Being ignorant, I say, "this is bad, because of delete-on-close". So
> I move the apr_file_close() -after- all the fopen/fclose business (for
> example, I do this in commit.c:commit_stream_close).
>
> Still though -- I get a segfault when committing, specifically in
> commit_stream_close, because fopen() tries to reopen the tmpfile, and
> it's already gone somehow.
Yeah, I think that's how it works... because it's been unlink()ed by APR
to implement DELONCLOSE, it no longer has a name in the filesystem. So
you can't open it again. :(
joe
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org