getpass regressions on Solaris/Illumos - 3.6 and master.

[Ira Cooper]

In looking at this issue in more detail:

I've heard the argument: Putting all the header files in replace.h will
increase build time.  So I did the experiment.  I put all the system/*
include files in replace.h except for system/kerberos.h, because
system/kerberos.h broke my build.  So far the difference doesn't look very
big at all, at least on my machine!  Now, I'll admit my development machine
is a really bad place to benchmark builds, so I'd like others to chime in
with their results, if there is interest in this approach.  This patch is
include_it_all.patch.

(make -j64 of our autoconfed build; on my dev box.)

Without the extra headers:

real    0m47.438s
user    8m38.416s
sys     1m17.440s

With them:

real    0m49.108s
user    8m55.999s
sys     1m23.193s

There is a second viable approach that metze pointed at, but this is NOT
what he suggested directly.  Pull the getpass section of system/passwd.h
into replace.h  Including the definition of rep_getpass.  If you do it
without that definition, you'll break platforms without a good getpass and
getpassphrase.  :(  This patch is add_getpass.patch.

Diffs for both approaches are included.

The add_getpass.patch is what I consider the "absolute minimum" to close
this issue.  But I thought the first approach was interesting enough, and
the result interesting enough that I've included both.  I'd like to see
other developer's times on the "include_it_all.patch".  Please be careful
of caching effects.  Build both ways a few times.

Thanks,

-Ira

On Mon, Jan 30, 2012 at 7:39 AM, Jelmer Vernooij <jelmer at samba.org> wrote:

>
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 01/29/2012 10:48 AM, Andrew Bartlett wrote:
> > On Sun, 2012-01-29 at 00:24 -0500, Ira Cooper wrote:
> >> There's been at least 2 regressions on the Solaris platforms. One
> >> involving net join not working, and one involving smbclient not working,
> >> using typed passwords.
> >>
> >> I ended up tracking this issue down via bisection
> >> to: 510e61871c0c2b2659b80d5f785522184131b1d9 and then working out why
> that
> >> patch was the issue.
> >>
> >> If you look at the commit involved, it looks like a no-op. Alas, util.c
> >> included "system/passwd.h" and util_cmdline.c did not, so there was a
> >> regression, because getpass doesn't take passwords longer than 8
> >> characters, on Solaris.
> >>
> >> I'm looking for a better idea than just adding "system/passwd.h" to
> >> source3/include/includes.h, that also has the effect of protecting
> against
> >> the kind of simple mistake made in the commit above. I can't blame the
> >> committer for the error, it was far too easy to make, in my opinion.
> >>
> >> Patch enclosed, and verified to work, against v3-6-test, that just adds
> >> "system/passwd.h" to source3/include/includes.h.
> >>
> >> Thanks for the explanation of the brokenness of getpass on Solaris,
> Andrew
> >> Bartlett.
> >
> > I think the correct fix is change the getpass() calls to be explicit
> > calls directly to the Samba replacement functions (which would be
> > renamed and moved to samba-util).
> >
> > We should then only fall back if we find a real-world system where the
> > replacement (now samba-util) function fails to compile, that we wish to
> > support, and it has a non-broken getpass()/getpassphrase().
> >
> > (This is the opposite to our normal practice with replacement functions,
> > and I queried this myself in years past, but it was put to me
> > persuasively that otherwise the replacement will never be tested).
> >
> > This will greatly reduce the chance that this might happen again.
> The defines for using the alternative functions should be in replace.h,
> so we can't miss them if we happen to forget one of the system/ includes.
>
> The reason I added libreplace as a separate thing from the code in lib/
> in the first place was exactly to be able to avoid this. Libreplace
> should be as transparent as possible, so we can just do the right thing
> on sane platforms, and it will just work on other platforms. We
> shouldn't have to worry about calling wrapper functions in case there
> are platforms on which a function is broken.
>
> Originally it was also possible to compile with just an empty replace.h
> and without having libreplace at all on modern Linux systems.
> Unfortunately that's no longer possible because of the system/ headers.
>
> Cheers,
>
> Jelmer
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.11 (GNU/Linux)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
>
> iQIcBAEBAgAGBQJPJo+dAAoJEACAbyvXKaRXRBsP/RdKocB0LjyirzFiesAZiVp5
> mYZ8VK0Hw5umW3kcI86Sh0/GxedY84bDCCITXX6Rn2jcfVEND9HiRxroRrR/HayM
> D5tYBZLJIdKqMM7wX12tJa0v6uKMr9aiCv8g+tMBQV0hZH2EzWRVU9dFMDRRkFD/
> VFOhfSCIDohZO/dvCNdwBWz6aRzpllyB9PEfVbuv6JBLy/VQWnXHrOXqt9+gWQcc
> Ls2LX5pNvmKVYoCTOLO3YRUGN69TeakVYRL1CX/gdKo8l9SKgca9/L0Cu7ul0lcw
> 3iQyO0WnKDD2zZnmMjXQEWfq6mAVgcgtbKUz+0XEorh+hqeam4+Z8P/QC2Jj+owE
> TAQNkwpRT3N/m7Wocmi9I+cMWc9fDbSzM8QO1p5rff6niVMF6yYXWpuPCx/OWpK/
> fDFghD0F0fH2Tuk49YMKIv7auCXaQeLdWMYwvQ1hMgbc8+XSXmMoOe07c2ysKdOf
> CrXCzPOOIySKMfH9HJsF3TVlihABBudynFAgDAAW7CK9AuNZYOPuLW3/v3Kb1Bik
> EuERKrlBQEgp7LIMXNnRD4SUTv6vs+znrmLJlxpJWy8F2OtZRxTr9fDfDyS7NRQH
> E/JdEnZO0dKhyEFcS2I9oAnRX4Dbz4IdnWLBN1MORK+HWyPYAA1R93HoOqN6las9
> YSL3QR2BWVswS+KJbSu/
> =xbac
> -----END PGP SIGNATURE-----
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: add_getpass.patch
Type: application/octet-stream
Size: 1006 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20120130/3198608a/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: include_it_all.patch
Type: application/octet-stream
Size: 688 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20120130/3198608a/attachment-0001.obj>