From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=AWL,BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, NICE_REPLY_A,RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL, SPF_HELO_NONE,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id 303251F4B4 for ; Sun, 13 Sep 2020 14:07:27 +0000 (UTC) Received: from localhost ([::1]:58138 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1kHSf7-000416-WD for normalperson@yhbt.net; Sun, 13 Sep 2020 10:07:26 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:34612) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kHSf5-00040o-4h for bug-gnulib@gnu.org; Sun, 13 Sep 2020 10:07:23 -0400 Received: from mo4-p00-ob.smtp.rzone.de ([85.215.255.20]:35876) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kHSf2-0002wV-Ix for bug-gnulib@gnu.org; Sun, 13 Sep 2020 10:07:22 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; t=1600006037; s=strato-dkim-0002; d=clisp.org; h=References:In-Reply-To:Message-ID:Date:Subject:Cc:To:From: X-RZG-CLASS-ID:X-RZG-AUTH:From:Subject:Sender; bh=j/JxHAUkp5pcdZi0U9bBM2d58KyV3FDEiwZeS/CjHVI=; b=p/aD+5E4Z2gzmY3/vHaKv+xc53uySCmYMkF9nDLT7Z3TQoPHduI/Zg7dwnFlRkofzI 5VGs0teb7BIaotKpBOczgV5gG9to+FnivsCw8h2UFyTwt3ceUD2uuWpMNoEY4T26Wint CO2dnplZndAJ27xZ+3KI05svBZtQ6yp/N6IjZ/3buJESdcrm68qeJAsVfxpVClXGEWKW w+PBEagSmxblMo/Y/APj+NP4XG2CWI3nCI74zmAlPFFuFMkPdjBlanmnNwGTRWPcOylV B4sCd6RcR0ojtnOmhKaItGbMaCoXdCTviNQEZ04qb7brfOrEeflzZa4lofliWhnWroCu h4Rw== X-RZG-AUTH: ":Ln4Re0+Ic/6oZXR1YgKryK8brlshOcZlIWs+iCP5vnk6shH+AHjwLuWOHqfyyPs=" X-RZG-CLASS-ID: mo00 Received: from bruno.haible.de by smtp.strato.de (RZmta 46.10.7 DYNA|AUTH) with ESMTPSA id z05f0fw8DE7Geoi (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (curve X9_62_prime256v1 with 256 ECDH bits, eq. 3072 bits RSA)) (Client did not present a certificate); Sun, 13 Sep 2020 16:07:16 +0200 (CEST) From: Bruno Haible To: John Darrington Subject: Re: [PATCH] tmpdir.c (path_search_alloc): New function. Date: Sun, 13 Sep 2020 16:07:15 +0200 Message-ID: <1987219.eVT7o25B4W@omega> User-Agent: KMail/5.1.3 (Linux/4.4.0-189-generic; KDE/5.18.0; x86_64; ; ) In-Reply-To: <20200913121125.GA21210@jocasta.intra> References: <20200913093334.17844-1-john@darrington.wattle.id.au> <3111347.7ZDk9rG9Yt@omega> <20200913121125.GA21210@jocasta.intra> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="UTF-8" Received-SPF: none client-ip=85.215.255.20; envelope-from=bruno@clisp.org; helo=mo4-p00-ob.smtp.rzone.de X-detected-operating-system: by eggs.gnu.org: First seen = 2020/09/13 10:07:17 X-ACL-Warn: Detected OS = Linux 2.2.x-3.x [generic] [fuzzy] X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, NICE_REPLY_A=-0.001, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_PASS=-0.001, SPF_NONE=0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: bug-gnulib@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Gnulib discussion list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: bug-gnulib@gnu.org Errors-To: bug-gnulib-bounces+normalperson=yhbt.net@gnu.org Sender: "bug-gnulib" 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 lengh= t of *TMPL. */ > > +extern int path_search_alloc (char **tmpl, size_t *tmpl_len, cons= t char *dir, const char *pfx, > > + bool try_tmpdir); > =20 > The calling convention is odd: If the caller is only meant to use *T= MPL and > later free() it, why does he need *TMPL_LEN? It seems redundant to r= eturn it > from this function. And then, if *TMPL is the only output (besides t= he error > condition), why not make it the return value? That is: > =20 > extern char * path_search_alloc (const char *dir, const char *pfx,= bool try_tmpdir); > =20 > In case of error, this function would return NULL with errno set. >=20 > That would also work. But I don't think the suggested interface is parti= cularly odd. > It is very similar to the getline function from libc. The 'getline' function is not a good model to imitate. Why? Because general= ly 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 t= hat 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 calli= ng 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= =E2=80=94 including path_search_alloc =E2=80=94 are not like this. Actually your function supports (a) and (b). But the documentation is lacki= ng 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 s= et. 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 f= ind 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 =3D=3D 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 fil= e. > =20 > 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