[PATCH] Don't call semanage transaction unless the context has changed

[Jakub Hrozek]

Hi,

the attached patches fix ticket
https://fedorahosted.org/sssd/ticket/2624

It turns out that calling libsemanage transactions is a fairly intensive
operation that involves copying multiple files under the /etc/selinux
hierarchy to a temporary subtree and then back. With the patches, the
context from IPA is checked against the local database first and only
applied if it differs.

[Michal Židek]

On 04/10/2015 03:24 PM, Jakub Hrozek wrote:
> Hi,
>
> the attached patches fix ticket
> https://fedorahosted.org/sssd/ticket/2624
>
> It turns out that calling libsemanage transactions is a fairly intensive
> operation that involves copying multiple files under the /etc/selinux
> hierarchy to a temporary subtree and then back. With the patches, the
> context from IPA is checked against the local database first and only
> applied if it differs.

Hi,

the patches look good, but you used talloc_strdup in
libsss_semanage and dlopen test failed.

src/tests/dlopen-tests.c:149:F:dlopen:test_dlopen_base:0: Error opening 
libsss_semanage.so: [dlopen() failed: 
/home/user/gitrepo/sssd/.libs/libsss_semanage.so: undefined symbol: 
talloc_strdup]

I think you can feel free to link the library with talloc,
there is no particular reason for not doing so, other than
it was not needed until now. It is an "sss" internal library
so talloc will have to be around anyway.

Michal

[Jakub Hrozek]

On Tue, Apr 14, 2015 at 03:18:24PM +0200, Michal Židek wrote:
> On 04/10/2015 03:24 PM, Jakub Hrozek wrote:
> > Hi,
> >
> > the attached patches fix ticket
> > https://fedorahosted.org/sssd/ticket/2624
> >
> > It turns out that calling libsemanage transactions is a fairly intensive
> > operation that involves copying multiple files under the /etc/selinux
> > hierarchy to a temporary subtree and then back. With the patches, the
> > context from IPA is checked against the local database first and only
> > applied if it differs.
>
> Hi,
>
> the patches look good, but you used talloc_strdup in
> libsss_semanage and dlopen test failed.
>
> src/tests/dlopen-tests.c:149:F:dlopen:test_dlopen_base:0: Error opening
> libsss_semanage.so: [dlopen() failed:
> /home/user/gitrepo/sssd/.libs/libsss_semanage.so: undefined symbol:
> talloc_strdup]
>
> I think you can feel free to link the library with talloc,
> there is no particular reason for not doing so, other than
> it was not needed until now. It is an "sss" internal library
> so talloc will have to be around anyway.
>
> Michal

Yeah, I don't suppose talloc dependency would be a problem. See attached
patches.

[Michal Židek]

On 04/14/2015 03:47 PM, Jakub Hrozek wrote:
> On Tue, Apr 14, 2015 at 03:18:24PM +0200, Michal Židek wrote:
> > On 04/10/2015 03:24 PM, Jakub Hrozek wrote:
> > > Hi,
> > >
> > > the attached patches fix ticket
> > > https://fedorahosted.org/sssd/ticket/2624
> > >
> > > It turns out that calling libsemanage transactions is a fairly intensive
> > > operation that involves copying multiple files under the /etc/selinux
> > > hierarchy to a temporary subtree and then back. With the patches, the
> > > context from IPA is checked against the local database first and only
> > > applied if it differs.
> >
> > Hi,
> >
> > the patches look good, but you used talloc_strdup in
> > libsss_semanage and dlopen test failed.
> >
> > src/tests/dlopen-tests.c:149:F:dlopen:test_dlopen_base:0: Error opening
> > libsss_semanage.so: [dlopen() failed:
> > /home/user/gitrepo/sssd/.libs/libsss_semanage.so: undefined symbol:
> > talloc_strdup]
> >
> > I think you can feel free to link the library with talloc,
> > there is no particular reason for not doing so, other than
> > it was not needed until now. It is an "sss" internal library
> > so talloc will have to be around anyway.
> >
> > Michal
>
> Yeah, I don't suppose talloc dependency would be a problem. See attached
> patches.

Ack.

CI link:
http://sssd-ci.duckdns.org/logs/job/13/04/summary.html

Michal

[Jakub Hrozek]

On Tue, Apr 14, 2015 at 05:58:22PM +0200, Michal Židek wrote:
> On 04/14/2015 03:47 PM, Jakub Hrozek wrote:
> > On Tue, Apr 14, 2015 at 03:18:24PM +0200, Michal Židek wrote:
> > > On 04/10/2015 03:24 PM, Jakub Hrozek wrote:
> > > > Hi,
> > > >
> > > > the attached patches fix ticket
> > > > https://fedorahosted.org/sssd/ticket/2624
> > > >
> > > > It turns out that calling libsemanage transactions is a fairly intensive
> > > > operation that involves copying multiple files under the /etc/selinux
> > > > hierarchy to a temporary subtree and then back. With the patches, the
> > > > context from IPA is checked against the local database first and only
> > > > applied if it differs.
> > >
> > > Hi,
> > >
> > > the patches look good, but you used talloc_strdup in
> > > libsss_semanage and dlopen test failed.
> > >
> > > src/tests/dlopen-tests.c:149:F:dlopen:test_dlopen_base:0: Error opening
> > > libsss_semanage.so: [dlopen() failed:
> > > /home/user/gitrepo/sssd/.libs/libsss_semanage.so: undefined symbol:
> > > talloc_strdup]
> > >
> > > I think you can feel free to link the library with talloc,
> > > there is no particular reason for not doing so, other than
> > > it was not needed until now. It is an "sss" internal library
> > > so talloc will have to be around anyway.
> > >
> > > Michal
> >
> > Yeah, I don't suppose talloc dependency would be a problem. See attached
> > patches.
>
> Ack.
>
> CI link:
> http://sssd-ci.duckdns.org/logs/job/13/04/summary.html
>
> Michal

THank you for the review. Pushed upstream:

master:
    1e0fa55fb377db788e065de917ba8e149eb56161
    748b38a7991d78cbf4726f2a14ace5e926629a54
    aa00d67b2a8e07c9080e7798defdc6c774c93465 
sssd-1-12:
    4d31f2c294db6090047e4d5348322b32ea0aaac1
    9c695e3a82fe5903b36b2d514b3284efeadc908c
    816d3cc041e276b138057aacb81d1a2bfb25add6