Re: [PATCH 0/2] Implement fgetspent

[Rich Felker <dalias@aerifal.cx>]

On Wed, Nov 20, 2013 at 07:13:39PM -0800, Michael Forney wrote:
> Hi Rich,
> 
> It turns out that one more function, fgetspent, is needed for the common
> shadow+pam_unix setup used in many desktop systems. Otherwise, when
> changing your password, pam_unix will attempt to enumerate through the
> shadow entries with fgetspent, rewriting them as it goes (and replacing
> the entry to be updated). This leaves you with a shadow file containing
> only the updated entry.
> 
> In the first patch, I moved spent parsing to an internal function,
> __parsespent. I opted to use __parsespent instead of __getspent_a
> (similar to the passwd and group functions) for several reasons:
> 
>     - To minimize the changes necessary to getspnam_r
>     - To avoid the extra memcpy as in getpw_r
>     - It seemed like a more self-contained function (which didn't rely
>       on the source of the entry).
>     - It would make it easier to implement sgetspent if we ever wanted
>       that (though, so far, I haven't found anything that requires this)
> 
> However, if this is not desired, I can send a new patch which uses a
> function __getspent_a, similar to __get{pw,gr}ent_a.

Overall I like this, but I don't see any advantage to putting
__parsespent in a separate file. On the surface it appears to reduce
dependencies for static linking, but any real-world use of fgetspent
would already depend on most of the functions getspnam_r depends on.
On the other hand, each additional file contributes to build time,
static library size (from elf header overhead), and potentially
hitting the command line limits on the ar/ld command lines. If you
don't have any objections, how about leaving the parsing code in
getspnam_r.c and only adding a new file for fgetspent?

Rich