Re: [PATCH] Updating exclusively opened files locks working copy

Eric Hanchrow <offby1@blarg.net> writes:

> I know precisely nothing about APR programming, so forgive me if this
> is stupid, but: doesn't this leak a file handle?
>
> + /* Make sure the file is not exclusively opened by someone else. */
> + if (!adding && entry && entry->schedule != svn_wc_schedule_delete)
> + {
> + apr_file_t* file;
> + SVN_ERR(svn_io_file_open(&file, fb->path, APR_READ, APR_OS_DEFAULT,
> + subpool));
> + }

The file will be closed when the subpool is destroyed and that happens
immediately after this code. Once the file has been closed it might
then get locked and so the problem has not been eliminated but simply
reduced. It's a bit like doing stat() before open() to check whether
the file exists, it's not a good solution since open() still has to
handle errors.

In this case it would be better to improve the error handling that is
going wrong, although the original report is short on details so it's
hard to know what to suggest and log handling was mentioned which can
make things tricky. If it turns out that the patch is the best way to
do things, then it needs a much better comment explaining what is
going on. One further disadvantage of the patch is that the code for
the "solution" is a long way from the code that causes the "problem"
and there is no regression test.

-- 
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org