Re: [PATCH] tmpdir.c (path_search_alloc): New function.

[John Darrington <john@darrington.wattle.id.au>]

On Sun, Sep 13, 2020 at 01:12:28PM +0200, Bruno Haible wrote:
     Hi John,
     
     As this is your first major contribution to Gnulib, can you please make sure
     you have signed a copyright assignment with the FSF regarding 'GNULIB'?

Yes,  I have.
     
     > * lib/tmpdir.c (path_search_alloc): Define new function similar to
     > path_search,  but which allocates the buffer for the result instead
     > of relying on the caller to preallocate it.
     
     This is a good idea, because it gets rid of an EINVAL error return.

It also means the caller does not have to guess how long the buffer should be.
     
     > +/* Like path_search, except that TMPL is allocated automatically.
     > +   TMPL may not be null.  *TMPL must be freed by the caller, when no longer needed.
     > +   After calling this function *TMPL_LEN will be set to the lenght of *TMPL.  */
     > +extern int path_search_alloc (char **tmpl, size_t *tmpl_len, const char *dir, const char *pfx,
     > +                       bool try_tmpdir);
     
     The calling convention is odd: If the caller is only meant to use *TMPL and
     later free() it, why does he need *TMPL_LEN? It seems redundant to return it
     from this function. And then, if *TMPL is the only output (besides the error
     condition), why not make it the return value? That is:
     
       extern char * path_search_alloc (const char *dir, const char *pfx, bool try_tmpdir);
     
     In case of error, this function would return NULL with errno set.

That would also work.  But I don't think the suggested interface is particularly odd.
It is very similar to the getline function from libc.

Regarding TMPL_LEN,  you are right, it *may* not be needed.   However I often find that
when writing code which munges strings, one needs to know the length of a string which
has already been calculated.   Of course one could simply use strlen to find it, but
strlen is O(n)  So I think, that if a function has to calculate a length of a string
anyway,  then it is worthwhile making that number available to the caller.
The caller is free to use it or ignore it as she wishes.   Often she'll be gratefull
for it.
     
     Typo: s/lenght/length/

Thanks for noticing.
     
     Please don't choose function names that start with underscore. These function
     names are in the scope of the vendor libc. And in fact, such function names
     would make merging with glibc harder, since glibc already has a function
     '__path_search' (defined in sysdeps/posix/tempname.c).

I wondered why that was there.

     Also, __path_search is a misnomer now: it does not search anything; it
     determines the temporary directory in which to place a temporary file.
     
So what name would you suggest?  "get_temp_directory" ?

J'