Re: [PATCH] Make fsfs survive stale NFS file handles

On Wed, Apr 04, 2007 at 12:03:08PM -0700, Eric Gillespie wrote:
> --- subversion/libsvn_fs_fs/fs_fs.c (revision 24424)
> +++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
> @@ -677,6 +678,104 @@
> return SVN_NO_ERROR;
> }
>
> +/* SVN_ERR-like macros for dealing with ESTALE
> + *
> + * In NFS v3 and under, the server doesn't track opened files. If you

(Note that we don't support NFSv2 locking, which typically means that we
can only run against NFSv3 servers... or maybe NFSv2 servers that support
NFSv3 locking, if such a thing exists).

> + * You must initialize err to SVN_NO_ERROR, as these macros will never
> + * touch it if the platform has no ESTALE.

(You must also initialise it to SVN_NO_ERROR because otherwise the
svn_error_clear() statement at the start will try to free a random
pointer).

> +#define SVN_IGNORE_ESTALE(err, expr) \
> + { \
> + svn_error_clear(err); \
> + err = (expr); \
> + if (err) \
> + { \
> + if (APR_TO_OS_ERROR(err->apr_err) != ESTALE) \
> + return err; \
> + } \
> + }

I was going to say that this leaks err, when ESTALE, except that I see
it doesn't - in the precise cases you're using it, the error is either
returned when the loop is exited, or cleared at the top of the loop (by
virtue of the clear hidden in SVN_RETRY_ESTALE).

Reasoning about the behaviour of these macros is pretty hard, though,
especially as they contain 'continue' statements. Does the benefit
of code de-duplication really outweigh the cost of understanding them?
I'm not sure.

> +/* Long enough to hold: "<svn_revnum_t> <node id> <copy id>\0"
> + * 19 bytes for svn_revnum_t (long)

That's 10 bytes for a long, 19 bytes would be for a hypothetical 64-bit
revision number.

> + Allocations are performed from POOL. */

"performed in" is the normal terminology.

The rest of the patch looks fine.

Regards,
Malcolm