On Tue, 2012-08-21 at 12:35 +0200, Jakub Hrozek wrote:
On Tue, Aug 21, 2012 at 12:28:22PM +0200, Jakub Hrozek wrote:
> On Fri, Aug 17, 2012 at 08:41:59AM -0400, Stephen Gallagher wrote:
> > On Thu, 2012-08-16 at 18:16 +0200, Jakub Hrozek wrote:
> > > On Wed, Aug 15, 2012 at 01:45:35PM -0400, Stephen Gallagher wrote:
> > > > On Wed, 2012-08-15 at 11:12 +0200, Jakub Hrozek wrote:
> > > > > On Tue, Aug 14, 2012 at 04:47:04PM -0400, Stephen Gallagher
wrote:
> > > > > > On Tue, 2012-08-14 at 18:12 +0200, Jakub Hrozek wrote:
> > > > > > > On Mon, Aug 13, 2012 at 07:27:42PM -0400, Stephen
Gallagher wrote:
> > > > > > > > On Mon, 2012-08-13 at 15:03 -0400, Stephen
Gallagher wrote:
> > > > > > > > > This patch backports commit
6fb75e297bf7fc83e3db1f5ae8560624656ef319 to
> > > > > > > > > the 1.5.x branch. It makes one major change:
1.5.x does not have support
> > > > > > > > > for deref/ASQ searches, so the deref lookup
for hostgroups has been
> > > > > > > > > replaced by parallel LDAP "base"
searches.
> > > > > > > > >
> > > > > > > > > I'm attaching two patches. One is the
complete patch to backport, the
> > > > > > > > > other is just an interdiff of the changes
from Jan's original patch to
> > > > > > > > > support the parallel lookups.
> > > > > > > > >
> > > > > > > > > I'm still in the process of testing this
patch, but I wanted to get it
> > > > > > > > > out for review in the meantime.
> > > > > > > >
> > > > > > > > Ok, during testing I found one bug where I forgot
to initialize a
> > > > > > > > loop-control/array-index variable. Oops.
> > > > > > > >
> > > > > > > > My basic smoketesting is now complete. Both modes
(srchost enabled and
> > > > > > > > disabled) are working as expected for a small set
of hosts. I don't have
> > > > > > > > the infrastructure to test large numbers, but I
suspect the only
> > > > > > > > difference will be performance, not function.
> > > > > > > >
> > > > > > > > New patches attached. As before, one is the
complete patch ("Add
> > > > > > > > ipa_hbac_support_srchost option to IPA
provider") and the other is just
> > > > > > > > the changes from Jan Zeleny's original patch
(to simplify the review
> > > > > > > > process).
> > > > > > >
> > > > > > > I don't understand this part:
> > > > > > >
> > > > > > > > + if (queue_len == 0) {
> > > > > > > > + /* This host is not in any hostgroups
*/
> > > > > > > > + ret = ENOMEM;
> > > > > > > > + goto error;
> > > > > > > > + }
> > > > > > >
> > > > > > > This seems to be preventing me from logging into a
host that is not part
> > > > > > > of any host group:
> > > > > > >
> > > > > > > [ipa_hbac_hostgroup_info_done] (3): Error [12][Cannot
allocate memory]
> > > > > > > [sdap_id_op_done] (9): releasing operation connection
> > > > > > >
> > > > > >
> > > > > > Thanks, there were two bugs there. The first was that this
should have
> > > > > > been ENOENT, not ENOMEM. The second was that I forgot to
special-case
> > > > > > ENOENT in ipa_hbac_hostgroup_info_done(). Both are now
fixed.
> > > > > >
> > > > > > New patches attached.
> > > > > >
> > > > > > > The rest looks good. Thank you for the interdiff,
indeed it made the
> > > > > > > review easier.
> > > > > >
> > > > > >
> > > > >
> > > > > There's one more problem - if the host is not a member of
any host
> > > > > group, it has no memberof attribute. However, this call would
add an
> > > > > empty attribute in that case:
> > > > >
> > > > > > + ret = sysdb_attrs_get_el(state->hosts[0],
> > > > > > + SYSDB_ORIG_MEMBEROF,
> > > > > > + &parent_el);
> > > > > > + if (ret != EOK) goto error;
> > > > > > +
> > > > >
> > > > > and later saving that host would fail:
> > > > >
> > > > > [sysdb_store_custom] (1): Failed to store custmo entry:
Constraint violation(19)[attribute originalMemberOf on
name=HOSTNAME,cn=hbac_hosts,cn=custom,cn=idm.lab.bos.redhat.com,cn=sysdb specified, but
with 0 values (illegal)]
> > > > > [sysdb_store_custom] (6): Error: 14 (Bad address)
> > > > > cancel ldb transaction (nesting: 2)
> > > > > [ipa_hbac_save_list] (1): sysdb_store_custom failed.
> > > > > [ipa_hbac_sysdb_save] (1): Could not save hbac_hosts. [14][Bad
address]
> > > > >
> > > > > Does anyone remember why does sysdb_attrs_get_el internally
create the
> > > > > attribute if it doesn't exist? To ease error checking? I
think we should
> > > > > either change it or create another getter that wouldn't
create the
> > > > > attribute automatically.
> > > >
> > > > I don't remember the reasoning either, but I don't want to go
through
> > > > the hassle of trying to change that during this backport. I added a
new
> > > > patch (that will work on 1.5.x and master) that just exposes the
> > > > internal get_el() routine that takes an option to create or not
create
> > > > the value. I then changed the above so it detects the ENOENT case
> > > > appropriately.
> > >
> > > Sure, this backport is not the right place for a big change to the sysdb
> > > API.
> > >
> > > This patchset works for me.
> > >
> > > Ack.
> >
> > One minor correction (for master branch only). I realized this morning
> > that on the master branch, there was one more place that needed to be
> > converted from sysdb_attrs_get_el_int() to sysdb_attrs_get_el_ext().
> > Please apply the attached patch to master instead. The other patch is
> > correct for sssd-1-5.
>
> There was one more case that needed converting in
> sysdb_attrs_get_uint16_t. I've fixed that and pushed the corrected
> patch.
>
> The interdiff is attached.
I also think we should push the patch to 1-8 when it was pushed to 1-5
and master, it would be weird if functionality was not present in one of
the three supported branches.
A 1-8 backport is attached, please review.
Ack. BTW, sorry about the earlier patch. I realized this morning that
I'd forgotten to actually squash in the missing piece.