apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Orton <jor...@redhat.com>
Subject Re: svn commit: r1862071 - in /apr/apr/trunk: file_io/os2/dir.c file_io/unix/dir.c file_io/win32/dir.c include/apr_file_info.h test/testdir.c
Date Tue, 02 Jul 2019 10:18:31 GMT
On Tue, Jul 02, 2019 at 09:59:20AM +0200, Branko ─îibej wrote:
> On 02.07.2019 08:49, Joe Orton wrote:
> > On Thu, Jun 27, 2019 at 05:01:56PM +0300, Ivan Zhakov wrote:
> >> On Tue, 25 Jun 2019 at 17:21, <jorton@apache.org> wrote:
> >>
> >>> Author: jorton
> >>> Date: Tue Jun 25 14:21:56 2019
> >>> New Revision: 1862071
> >>>
> >>> URL: http://svn.apache.org/viewvc?rev=1862071&view=rev
> >>> Log:
> >>> Add apr_dir_pread(), a variant of apr_dir_read() which allows callers
> >>> to read a directory with constant memory consumption:
> > ...
> >> I'm not sure it's best fix. Better solution would be allocate buffer for
> >> dirname + MAX_FILE_NAME in apr_dir_t and reuse it on every iteration. Win32
> >> already has such code.
>
> > I didn't look at win32 too closely, so on win32 currently doing:
> >
> >   apr_dir_open(&x, ...);
> >   apr_dir_read(&f1, ..., x);
> >   apr_dir_read(&f2, ..., x);
> >
> > invalidates f1?  That's pretty ugly, there is no indication in the API 
> > that apr_dir_read() has side-effects like that.
> 
> The 2.0 docstring says explicitly that "memory is allocated in the pool
> passed to apr_dir_open()". While this is really bad API design (hence
> apr_dir_pread() ...), it pretty much forbids the implementation from
> reusing an internal name buffer. Looks like the Windows implementation
> needs fixing?

Reading the win32 code a bit more, it is not constant-memory even with 
the buffer in apr_dir_t, because it can allocate from dir->pool (and 
even create cleanups!) in the more_finfo() call, so I think the current 
behaviour is not justifiable.

Ivan, do you have a counter-proposal to apr_dir_pread() to allow 
constant memory while reading a directory?  If we could have a hard 
constraint on apr_dir_read() *never* allocating any memory I guess that 
would work but appears to require substantial changes to the win32 side 
which I'm not qualified for.

IMO not pstrdup'ing the filenames on Win32 is an API violation.  You 
have to be very clear in documenting things like that if someone is 
passing in a struct *.

Regards, Joe

Mime
View raw message