From owner-svn-src-all@freebsd.org Fri Jul 27 16:07:22 2018 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 7E48F104F7FD for ; Fri, 27 Jul 2018 16:07:22 +0000 (UTC) (envelope-from ian@freebsd.org) Received: from outbound1b.ore.mailhop.org (outbound1b.ore.mailhop.org [54.200.247.200]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 124CB7BC2C for ; Fri, 27 Jul 2018 16:07:21 +0000 (UTC) (envelope-from ian@freebsd.org) X-MHO-RoutePath: aGlwcGll X-MHO-User: 1edfa327-91b7-11e8-93fa-f3ebd9db2b94 X-Report-Abuse-To: https://support.duocircle.com/support/solutions/articles/5000540958-duocircle-standard-smtp-abuse-information X-Originating-IP: 67.177.211.60 X-Mail-Handler: DuoCircle Outbound SMTP Received: from ilsoft.org (unknown [67.177.211.60]) by outbound1.ore.mailhop.org (Halon) with ESMTPSA id 1edfa327-91b7-11e8-93fa-f3ebd9db2b94; Fri, 27 Jul 2018 16:07:12 +0000 (UTC) Received: from rev (rev [172.22.42.240]) by ilsoft.org (8.15.2/8.15.2) with ESMTP id w6RG7BuC023810; Fri, 27 Jul 2018 10:07:11 -0600 (MDT) (envelope-from ian@freebsd.org) Message-ID: <1532707631.61594.66.camel@freebsd.org> Subject: Re: svn commit: r336746 - in head/lib: libc/gen libutil From: Ian Lepore To: Konstantin Belousov Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Date: Fri, 27 Jul 2018 10:07:11 -0600 In-Reply-To: <20180727154359.GB2489@kib.kiev.ua> References: <201807261834.w6QIYc9i080738@repo.freebsd.org> <20180727150304.GA2489@kib.kiev.ua> <1532705741.61594.53.camel@freebsd.org> <20180727154359.GB2489@kib.kiev.ua> Content-Type: text/plain; charset="ISO-8859-1" X-Mailer: Evolution 3.18.5.1 FreeBSD GNOME Team Port Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 27 Jul 2018 16:07:22 -0000 On Fri, 2018-07-27 at 18:43 +0300, Konstantin Belousov wrote: > On Fri, Jul 27, 2018 at 09:35:41AM -0600, Ian Lepore wrote: > > > > On Fri, 2018-07-27 at 18:03 +0300, Konstantin Belousov wrote: > > > > > > On Thu, Jul 26, 2018 at 06:34:38PM +0000, Ian Lepore wrote: > > > > > > > > > > > > Author: ian > > > > Date: Thu Jul 26 18:34:38 2018 > > > > New Revision: 336746 > > > > URL: https://svnweb.freebsd.org/changeset/base/336746 > > > > > > > > Log: > > > >   Make pw_scan(3) more compatible with getpwent(3) et. al. when > > > > processing > > > >   data from /etc/passwd rather than /etc/master.passwd. > > > >    > > > >   The libc getpwent(3) and related functions automatically read > > > > master.passwd > > > >   when run by root, or passwd when run by a non-root > > > > user.  When run by non- > > > >   root, getpwent() copes with the missing data by setting the > > > > corresponding > > > >   fields in the passwd struct to known values (zeroes for > > > > numbers, or a > > > >   pointer to an empty string for literals).  When libutil's > > > > pw_scan(3) was > > > >   used to parse a line without the root-accessible data, it was > > > > leaving > > > >   garbage in the corresponding fields. > > > >    > > > >   These changes rename the static pw_init() function used by > > > > getpwent() and > > > >   friends to __pw_initpwd(), and move it into pw_scan.c so that > > > > common init > > > >   code can be shared between libc and libutil.  pw_scan(3) now > > > > calls > > > >   __pw_initpwd() before __pw_scan(), just like the getpwent() > > > > family does, so > > > >   that reading an arbitrary passwd file in either format and > > > > parsing it with > > > >   pw_scan(3) returns the same results as getpwent(3) would. > > > >    > > > >   This also adds a new pw_initpwd(3) function to libutil, so > > > > that code which > > > >   creates passwd structs from scratch in some manner that > > > > doesn't involve > > > >   pw_scan() can initialize the struct to the values expected by > > > > lots of > > > >   existing code, which doesn't expect to encounter NULL > > > > pointers or garbage > > > >   values in some fields. > > > > > > > If my reading is right, you just made libutil depend on the > > > internal > > > libc interfaces. Formal consequence is that libutil.so version > > > must > > > be bumped each time the used interface is changed (and it is > > > allowed > > > to change). I think that your change actually requires the bump > > > of > > > libutil.so.N version already. > > > > > > Also, libutil.so.N should be moved from the libutil pkgbase > > > package to > > > the clibs package, I am not sure about this. > > > > > > At the higher level, I very much dislike this change. > > > FBSDprivate_1.0 > > > namespace is for symbols providing the internal interfaces for > > > the > > > C runtime implementation in the FreeBSD. This is mostly a knot of > > > inter-dependencies between rtld, libc and libthr. libutil > > > arguably > > > should not participate. > > > > > > If you want for libc to provide a functionality outside the C > > > runtime, > > > please make the sustainable interface, which ABI can be > > > maintained, and > > > export the symbols in the normal namespace, with the usual > > > stability > > > guarantees. > > There was already a function, __pw_scan(), in file pw_scan.c, which > > was > > called from both libutil and libc implementations. I added a new > > function __pw_initpwd() into the pw_scan.c file. That function is > > called from all the same places that __pw_scan() is called from. So > > as > > near as I can tell, I haven't changed the structure of anything or > > created any new linkages between the libraries that didn't exist > > already. > > > > I will admit I don't understand the FBSDprivate_1.0 stuff at all, > > and > > there appears to be no documentation or guidance on how to work > > with > > it. Since __pw_scan was in the private list, and I was adding a new > > function that is like it in every way, I reasoned that the new > > function > > should be in the list too. It's actually not clear to me that > > either of > > the functions should be in that list, but like I said... no > > published > > info about it that I could find. > > > > I also noticed that chpass(1) and pwd_mkdb(8)_both directly compile > > in > > their own copy of the pw_scan.c source using .PATH in their > > makefiles. > > I wonder if doing that as the way of sharing the code between libc > > and > > libutil would be a better thing to do? (And presumably that would > > remove the need to have entries in the FBSDprivate_1.0 list?) > I suspect that the better way is to export the used functions > in the FBSD_1.5 namespace.  Might be use the opportunity to rename > them, in particular, remove the leading double underscore.  Might > be, reconsider the interfaces to make them more generic. > > Do the consumers depend on the specific layout of some structure ? I believe pw_scan() was originally a static function in libc used by getpwent() and related functions. When libutil grew some pw utilties, it looks like the static pw_scan was renamed to __pw_scan and added to the private list, so that it could be shared with libutil (and so I copied that process with pw_init(), but I had to rename it to pw_initpwd because libutil already has a pw_init()). I'm not sure how to make the functions more generic, pw_scan() parses a line of text in passwd(5) format, optionally warns about errors, and fills in a struct passwd with what it finds. pw_initpwd() inits a struct passwd to (well-)known default values. But libutil pw_scan() allocates the struct and requires the caller to free it, and the internal __pw_scan() fills in a struct passed in to it and returns an int, so there's no way to re-unify the functions under a single name. It would be strange to me to have one or two of the pw_xxxx() family of functions in libc and the rest of them in libutil. Libutil seems like a fine place for password/group file utilities that go beyond the posix functions. It's just an implementation detail that we'd prefer to share the source code for a small bit of common functionality around parsing lines of passwd file data. -- Ian