I'm resending a patch for ticket #2870 on behalf of the original
reporter who also kindly submitted a patch.
The patch looks good to me, as soon as we fix CI, I'll submit it as well
and I think we can push it..
On 11/23/2015 04:32 PM, Petr Spacek wrote:
> On 23.11.2015 13:53, Pavel Reichl wrote:
>> On 11/20/2015 05:35 PM, Jakub Hrozek wrote:
>>> On Fri, Nov 20, 2015 at 03:57:04PM +0100, Pavel Reichl wrote:
>>>> please see attached patch.
>>> Could you ask Petr Spacek to do a conceptual review? Somebody else (me
>>> of noone else is interested) can then do the code-review but I don't
>>> think our team has as good experience in handling nsupdate details..
>> Hello Petr, could you do the conceptual review as Jakub wishes?
>> I hope it should not be too hard as what we change in the patches is
>> generation of a textual message for nsupdate and it's tested I dare to say
> Sure, just send me a link :-) I do not see it the message above.
Sure, hope the link will work for you:
I'm working on pam_sss.so tests and I ran into a problem that I don't
know how to solve best.
tl;dr, I would like to set different environment variables for different
tests in order to set up cwrap libraries differently per-test.
I can't use setenv() in the test itself, because that's too late, I need
the variables to be set when __attribute__(constructor) is run, so normally
at program startup, when the libraries are loaded.
With cmake it's easy, use set(TEST_ENVIRONMENT). But with autotools, I
only found two ways:
- TESTS_ENVIRONMENT - this is fine, but it's per Makefile.am. So I
would have to split the tests more, into pam_wrapper tests that also
require uid_wrapper, tests that only require pam_wrapper, ...
- LOG_COMPILER - this allows to run a wrapper script before a test
that receives the test name as argv. So this is pretty much what I
want except this is a feature new to automake 1.12, which would
rule out both RHEL-6 and Ubuntu Trusty (which is used by Travis)
So I'm really leaning towards creating a src/tests/cwrap/pwrap/Makefile.am
and src/tests/cwrap/pwrap_root/Makefile.am. The downside of multiple
Makefile.am files is that there is some code duplication and the build
takes longer. But I still think there is enough interest (from us and from
our users) to support git master on old platforms. I can file a ticket to
remove this and use LOG_COMPILER when we drop support for RHEL-6 and old
If you disagree, please reply, otherwise I'm going to send a patch with
42% code coverage, up from 0% last week. Woo!
please see attached patch set which contains some refactoring which is
in my opinion quite safe. It doesn't touch the recursive call of
pam_reply() as I think that changing that would be more risky change.
the attached patches are my proposal to fix
I haven't tested them past make check yet, because I'm not sure I like
them myself :) but at the same time I can't see a better way to keep
track of the servers and let callers set state of servers.
The most ugly thing so far IMO is the fo_internal_owner member. I would
prefer to instead have a fo_server_wrap structure that would be used for
the server_list, but I didn't want to do a large change before we agree
the refcount is a good idea at all.
The other ugly side-effect is that we need to be sure that nobody calls
talloc_free on the fo_server structure. Instead, only the parent context
can be freed (that's also what the first patch is about).
I found this potential crash when trying to find another issue in the
failover code. To reproduce, just revert the changes to
src/providers/fail_over.c and run make check, you should see either a
crash or at least an error if you use valgrind.