Malcolm Rowe <malcolm-svn-dev@farside.org.uk> writes:
> On Thu, Apr 26, 2007 at 05:19:29PM -0700, Eric Gillespie wrote:
> > > > + err =3D3D svn_io_file_close(activity_file, iterpool);
> > > > + if (err && err->apr_err !=3D3D APR_SUCCESS)
> > > > + {
> > > > +#ifdef ESTALE
> > > > + if (APR_TO_OS_ERROR(err->apr_err) !=3D3D ESTALE)
> > > > +#endif
> > > > + return NULL;
> > >=20
> > > Why have different behaviour for ESTALE/non-ESTALE? While I can't think
> > > of a valid non-ESTALE return, it seems like we should treat them the
> > > same - whether that means returning what we have or returning NULL, I'm
> > > not sure. (especially as ESTALE means that someone just overwrote the
> > > transaction name, so we know we have stale data).
> >=20
> > We know we have stale data at this time only on NFS < 3, with its
>
> (We don't support NFSv2 - it doesn't have sane locking)
I meant <= 3. NFSv4 is what tracks opens. Anyone using NFSv4
isn't going to see any ESTALEs.
> > +/* Return the transaction name from the file pathname. */
> "Return the transaction name of the activity stored in file PATHNAME in
> repos REPOS, or NULL if PATHNAME cannot be read for any reason.", or simila=
> r?
> > +static const char *
> > +read_txn(const dav_svn_repos *repos, const char *pathname)
Re-reading this, i changed read_txn to take a pool, not dav_svn_repos.
> Still unsure about this lifetime of repos->pool, but as you pointed out
> before, that's not a new problem.
I don't disagree, but yeah, it's not a new problem. I was
thinking that callers ought to have access to the request pool,
which seems more appropriate. After committing this, i'll look
at having callers pass in a more appropriate pool (these are
private interfaces, so that's fine, right?).
Wait, no, dav_svn_repos is allocated out of the request pool,
isn't it? I think this is fine after all; i think repos->pool
only lasts as long as this request.
> > + err =3D svn_io_file_open(&activity_file, pathname, APR_READ | APR=
> _BUFFERED,
>
> line length?
It ends at 80 columns. Bah, hacking says 79, WTF? Fixed.
> > + err =3D svn_io_read_length_line(activity_file, txn_name, &len, NUL=
> L);
>
> Using the global pool for allocations? I don't think so :-)
> (even if, as here, the pool is only used for error reporting)
Hah, that was not intentional. Fixed.
> Rest looks good to me.
Thanks; i took your suggestions and committed r24815.
--
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 Fri Apr 27 23:53:37 2007