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);
> > + _CHECK_STATUS;
> > +
> > + 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.
> > + _CHECK_STATUS;
> > + 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