From owner-svn-src-head@freebsd.org Wed Nov 18 22:32:22 2020 Return-Path: Delivered-To: svn-src-head@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 73FDE473F20; Wed, 18 Nov 2020 22:32:22 +0000 (UTC) (envelope-from se@freebsd.org) Received: from smtp.freebsd.org (smtp.freebsd.org [96.47.72.83]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4CbyHB2n7Hz4lmY; Wed, 18 Nov 2020 22:32:22 +0000 (UTC) (envelope-from se@freebsd.org) Received: from Stefans-MBP-WLAN.fritz.box (p200300cd5f3b88002402e588537088de.dip0.t-ipconnect.de [IPv6:2003:cd:5f3b:8800:2402:e588:5370:88de]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) (Authenticated sender: se/mail) by smtp.freebsd.org (Postfix) with ESMTPSA id B4E012CE0E; Wed, 18 Nov 2020 22:32:21 +0000 (UTC) (envelope-from se@freebsd.org) To: Mateusz Guzik Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org References: <202011181944.0AIJiUU3003699@repo.freebsd.org> From: Stefan Esser Subject: Re: svn commit: r367813 - head/lib/libutil Message-ID: Date: Wed, 18 Nov 2020 23:32:20 +0100 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.16; rv:78.0) Gecko/20100101 Thunderbird/78.5.0 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="M3Q0wLcC3m29rCmWvjFoU4rgXCMtZ0OW2" X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 Nov 2020 22:32:22 -0000 This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --M3Q0wLcC3m29rCmWvjFoU4rgXCMtZ0OW2 Content-Type: multipart/mixed; boundary="qhZvkLNJvoMuwLdfz38cO6PhYXevwOiV6"; protected-headers="v1" From: Stefan Esser To: Mateusz Guzik Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Message-ID: Subject: Re: svn commit: r367813 - head/lib/libutil References: <202011181944.0AIJiUU3003699@repo.freebsd.org> In-Reply-To: --qhZvkLNJvoMuwLdfz38cO6PhYXevwOiV6 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable Am 18.11.20 um 22:40 schrieb Mateusz Guzik: >> +{ >> + static const int localbase_oid[2] =3D {CTL_USER, USER_LOCALBASE}; >=20 > There is no use for this to be static. Why not? This makes it part of the constant initialized data of the library, which is more efficient under run-time and memory space aspects than initializing them on the stack. What do I miss? >> + char *tmppath; >> + size_t tmplen; >> + static const char *localbase =3D NULL; >> + >> + if (issetugid() =3D=3D 0) { >> + tmppath =3D getenv("LOCALBASE"); >> + if (tmppath !=3D NULL && tmppath[0] !=3D '\0') >> + return (tmppath); >> + } >> + if (sysctl(localbase_oid, 2, NULL, &tmplen, NULL, 0) =3D=3D 0 && >> + (tmppath =3D malloc(tmplen)) !=3D NULL && >> + sysctl(localbase_oid, 2, tmppath, &tmplen, NULL, 0) =3D=3D 0) { >=20 > Apart from the concurrency issue mentioned in the comment this is just > very wasteful. Instead you can have a small local buffer, say 128 > bytes and pass that to be populated. The sysctl handler than can > populate that and return an error if the size is too small. I don't > know if sysclt api allows it to return the set size as it is. Worst > case you can just retry with a bigger malloced buffer. You obviously did not follow the development of this code in the review. It used to have such a buffer, but this was rejected, since only very few of the programs that link against this library are going to call this function. Allocating a small buffer and replacing it with a larger one if it was too small adds needless complexity to this program and needs more code. It is not sensible (IMHO) to reduce the number of system calls by 1 for the small number of programs that use this feature, and which perform at least tens of other system calls at start-up. > Once you get the result you can malloc a buffer and > atomic_cmpset_rel_ptr localbase to point to it. If this fails, another > thread got the result, you free your buffer and return (localbase). Yes, I wrote that I could use atomics, feel free to modify the program accordingly. It will not make a difference in practice, since there is no good reason to call this function from within a multi-threaded context at all, and none of the candidates to be modified to use this function in base do. > Also the kernel counter part completely unnecessarily comes with a > static 1KB buffer to hold what typically is going to be nothing. This > should also be allocated as needed. Worst case you can add a trivial > lock around it. The kernel buffer does already exist and you did not complain about that buffer, when it has been committed. A size of PATHNAMEMAX is used to allow for any valid path name to be set from the loader. If there is consensus that the value should be limited to e.g. 64 or 128 bytes, this is trivially changed in kern_mib.c. I'd be more worried, if this buffer existed not as a single instance in the kernel but e.g. per process, but this is not the case. Feel free to make this a dynamically allocated variable, but make sure that you do not increase the code size by as much as you reduce the static storage required. I do not need the dynamic assignment to localbase at all, and I have suggested to introduce a kernel build option (e.g. WITH_LOCALBASE_VAR) to control compilation of kernel and library with this function or to return just the constant string _PATH_LOCALBASE or "/usr/local". But there were very strong requests for this dynamically set localbase and I have provided an implementation that is functional and easy to use. Regards, STefan --qhZvkLNJvoMuwLdfz38cO6PhYXevwOiV6-- --M3Q0wLcC3m29rCmWvjFoU4rgXCMtZ0OW2 Content-Type: application/pgp-signature; name="OpenPGP_signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="OpenPGP_signature" -----BEGIN PGP SIGNATURE----- wsB5BAABCAAjFiEEo3HqZZwL7MgrcVMTR+u171r99UQFAl+1oPQFAwAAAAAACgkQR+u171r99UQX PAgAo740FH+YypU6B+NKSBLCZJfBUvA6VBDeOkN1YkoUW94SHTdG9raBIE1rXSZq0NDhqfpGAqDs RmJCVQlYHyom4Y3Rqk630fThlxYQz5OJGmMr0grSgHscKjA3tPk5KxnKWSjSnk1MuwTFim+4f8Be psDBAmzYaMh4M6mIgyROK+0qEtX6dBiaamOv8l3kLhf8Uwcs0KOvZo1lB3EgNEtbAUWa/2rZuJpD /hwM2aqKHxdUo5EZ3Wa6ng7tDAJKpnrBd0Utm1nvgH4tgfn/Qi8jvt0C/Tlk9edTALEwPkRW9R6o POXtzby+modCyjXKx/H7/ktu0RCUaTv0chyAviypIA== =t6Ok -----END PGP SIGNATURE----- --M3Q0wLcC3m29rCmWvjFoU4rgXCMtZ0OW2--