Date: Fri, 27 Jul 2018 10:07:11 -0600 From: Ian Lepore <ian@freebsd.org> To: Konstantin Belousov <kostikbel@gmail.com> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r336746 - in head/lib: libc/gen libutil Message-ID: <1532707631.61594.66.camel@freebsd.org> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1532707631.61594.66.camel>