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