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, <jor...@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
