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

Re: [PATCH] svn --version --verbose += /etc/os-release

From: Daniel Shahaf <d.s_at_daniel.shahaf.name>
Date: Tue, 27 Jan 2015 11:42:16 +0000

Philip Martin wrote on Tue, Jan 27, 2015 at 11:28:35 +0000:
> Daniel Shahaf <d.s_at_daniel.shahaf.name> writes:
>
> > +static const char *
> > +systemd_release(apr_pool_t *pool)
> > +{
> > + svn_error_t *err;
> > + svn_stream_t *stream;
> > +
> > + err = svn_stream_open_readonly(&stream, "/etc/os-release", pool, pool);
...
> > + if (!strncmp(line->data, "PRETTY_NAME=", 12))
> > + return remove_shell_quoting(line, 12, pool);
>
> The file below the stream is still open and will remain open until the
> pool is cleared and that also clears the result. scratch/result pools
> would allow the caller to close the stream and keep the result. It
> might be better to arrange for this function to close the stream,
> perhaps just assume the file is small and read it all into memory.
>

Is that a problem in practice?

Where should we introduce dual pools? The callstack is single-pool all
the way to the public entry point svn_version_extended(). We prefer to
avoid creating subpools in functions that don't do a lot of work, so if
we want to convert to dual-pools we'll need to revv that function.

Which I don't object to, but I think should be in a separate patch, not
part of this one.

The sibling function lsb_release() has the same problem.

Thanks for the review,

Daniel
Received on 2015-01-27 12:42:48 CET

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