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

[Bruno Haible <bruno@clisp.org>]

Hi John,

>      > +/* 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.

The 'getline' function is not a good model to imitate. Why? Because generally there
should be two supported ways to call such a function
  (a) with a NULL argument - to let the function allocate as much memory as it needs,
  (b) with a stack-allocated buffer as argument - to let the function use that buffer
      if its size is sufficient.
The 'getline' function, as documented [1], does not support the second case.
Its calling convention is thus apparently tailored to the use-case of calling it in
a loop, avoiding memory allocations if a line is shorter than the previous line.
But this is a *very* special use-case, and most functions that people write — including
path_search_alloc — are not like this.

Actually your function supports (a) and (b). But the documentation is lacking and
incorrect:
  - "TMPL may not be null." OK but what can the caller put in TMPL?
  - "*TMPL must be freed by the caller, when no longer needed." Not true.
    In case (b), *TMPL is unchanged, and thus must not be free()d.

How about using this wording, from the GNU libunistring documentation [2]:

  [Functions returning a string result] take a (resultbuf, lengthp) argument
  pair. If resultbuf is not NULL and the result fits into *lengthp units, it
  is put in resultbuf, and resultbuf is returned. Otherwise, a freshly
  allocated string is returned. In both cases, *lengthp is set to the length
  of the returned string. In case of error, NULL is returned and errno is set.

And the prototype that goes with it:

extern char * path_search_alloc (char *resultbuf, size_t *lengthp,
                                 const char *dir, const char *pfx, bool try_tmpdir);

This is simpler than
   extern int path_search_alloc (char **tmpl, size_t *tmpl_len,
                                 const char *dir, const char *pfx, bool try_tmpdir);
in two aspects:
  - It has one less indirection.
  - In the use-cases (a) and (b) the caller has one less local variable.

> 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)

OK, but then make sure that the caller understands what the returned value is.
If you call it 'tmpl_len' everyone would assume that tmpl_len == strlen (tmpl).
But in fast you return strlen (tmpl) + 1. Therefore it should be called
'tmpl_size', not 'tmpl_len'.

>      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" ?

How about renaming
  path_search -> gen_tempfile_template_prealloc
  path_search_alloc -> gen_tempfile_template_alloc
? The first one takes preallocated storage, whereas your function allocates
storage.

Bruno

[1] https://linux.die.net/man/3/getline
[2] https://www.gnu.org/software/libunistring/manual/html_node/Conventions.html