[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] Re: Is mod_dav_svn safe for use in a threaded MPM?

From: Eric Gillespie <epg_at_pretzelnet.org>
Date: 2007-04-11 01:27:08 CEST

Thanks for the comments. I'll work on these tomorrow.

Malcolm Rowe <malcolm-svn-dev@farside.org.uk> writes:

> > (_read_txn): New helper to return the txn from an activity file.
> Any reason this uses a leading underscore? That's not our usual naming
> convention for static functions. (Have you been doing too much Python
> recently? :-)

Hehe, i'll clean these up.

> > - if (status !=3D APR_SUCCESS)
> > + /* ### let's just assume that any error means the
> > + ### activity/transaction doesn't exist */
> Does the '###' imply that we might want to consider doing something
> different in the future?

I would say no, but this comment was already there, so i left
it. Maybe there was originally some plan behind it...

> > + status =3D apr_stat(&finfo, pathname, APR_FINFO_SIZE, repos->pool);
> > +
> > + status =3D apr_file_open(&activity_file, pathname, APR_READ,
> > + APR_OS_DEFAULT, repos->pool);
> What's the lifetime of repos->pool? If it's too long, we could
> potentially stack up file handles and...

Don't i close the file handles? I'll double check. Since the
dbm stuff was working out of repos->pool also, this new stuff
shouldn't be any worse.

> > + status =3D apr_file_read_full(activity_file, txn_name, finfo.size, &by=
> tes_read);
> There's a race between the stat() and open() - if the activity was
> reassigned to a new transaction with a longer name between the
> two calls, we'd read a partial transaction name. Perhaps either

I didn't think of that.

> terminate the transaction name with something ('\n'?) and check for
> that, or simply try to read an extra byte (and keep the length/read
> verification), or maybe just use fstat() [which might even be more
> efficient anyway, and is certainly easier to understand].

Hm, how does fstat avoid the problem? Since i currently just
write over the file when storing an activity, it seems to me it
would have the same problem. If i change to renaming over the
file, this problem goes away, but i now have to check for ESTALE.

> > + if (bytes_read !=3D finfo.size)
> > {
> Worth checking for zero-sized files as well?

Yes (unless we switch to rename).

> > + status =3D apr_file_open(&activity_file, pathname, APR_WRITE | APR_CRE=
> ATE,
> > + APR_OS_DEFAULT, repos->pool);
> This should write to a temporary file and rename() so that readers don't
> see partially-written state. Of course, that also means that the code
> should deal with ESTALE as well.

I thought that since i pass the whole transaction name in one go
that would not be a problem, but now i think about interrupted
writes, so i guess that's not true after all. So yeah, i'll move
the ESTALE macros to svn_error.h and rework this stuff.

I don't know mod-dav-svn well enough to comment on your latest
post about the activity id's usability as a filename. Waiting
for someone else to chime in...

Eric Gillespie <*> epg@pretzelnet.org

To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Apr 11 01:28:29 2007

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.