[Freeipa-devel] [PATCH][SSSD] Implement GetUserAttributes in the InfoPipe

[Simo Sorce]

On Mon, 2009-03-02 at 06:56 -0500, Stephen Gallagher wrote:
> Replies inline.
> 
> Simo Sorce wrote:
> > Stephen,
> > I think there is some work to do on the patch, as it is it will not work
> > as, for example, the latest patches removed check_provider.
> > (It's always true except for LOCAL, so I removed it in favor of checking
> > explicitly for the domain name != LOCAL)
> 
> I like it the way I have it for now. When the infp_getattr_req is
> created, I do the strcasecmp("LOCAL") and assign that to
> infp_getattr_req, then I can just check the boolean value wherever I
> need to. Much faster than doing another string compare. (And since
> GetAttributes may call into the sysdb any number of times, depending on
> how many users they are requesting, this is a definite optimization).
> 
> Also, for what it's worth, you are doing the same in nsssrv.c.

That's what happen when you review late at night, I read check_provider
and thought about has_provider, of course the use you make is fine
here ...

> > See some minor comments inline, but the general approach looks ok.
> > 
> > On Sun, 2009-03-01 at 15:19 -0500, Stephen Gallagher wrote:
> >> This patch adds support for requesting user data in the sysdb via the
> >> InfoPipe. It currently has support for reading defined entries of
> >> integral, floating-point or string types.
> > 
> > Some comments:
> > 
> > - When you use btreemap_get_value() like in infp_get_domain_obj() you
> > should probably use talloc_get_type() instead of doing a cast yourself,
> > so that if something wrong comes out of it you get a null pointer and do
> > not act on random data.
> 
> Fixed. I'm not sure why I did that. I usually use talloc_get_type().
> 
> > 
> > - why in create_getattr_result_map() do you fetch uint64_t data but
> > variant->data is cast to int ?
> 
> That was a mistake. I was tinkering with smaller integral types to
> ensure that casting to other (and signed) types was working as expected.
> I thought I had switched everything back to uint64_t. Fixed.
> 
> > 
> > - In infp_get_all_attributes() why do you copy all these static strings?
> > As far as I can see they are not manipulated in any way, but just used
> > as is.
> 
> Only talloc pointers can be added to btreemaps because they steal the
> reference (to make sure the data they hold doesn't disappear during the
> tree's life). Passing a static string == a segfault (this was an
> annoying bug to track down).

I thought you were passing the "attributes" array. However if this is
the case, I would prefer btreemap to strdup the key instead of getting a
reference, I really want to avoid talloc_reference() for now as it have
subtle implications. I will commit a patch to btreemap to do that.

> > - I see that you added confdb/confbd.h to sysdb.h, but you also keep
> > including them both explicitly in infopipe, any reason for this ?
> > 
> >> Tasks remaining:
> >>     1) Implement call to the provider when cache is out of date
> >>     2) Support byte arrays for userpic and similar
> >>
> >> I modified init_src_context in sysdb_search.c to accept an array of
> >> attributes to pass into the LDB search.
> > 
> > I would rather not make attrs a parameter of init_src_context() but just
> > assign it when necessary like we do for expression, given most of the
> > time we would pass in NULL otherwise.
> 
> You're right, that makes more sense. Fixed.
> 
> > 
> >> I also made one additional related fix: the btreemap now sorts in the
> >> correct order. Previously I had accidentally transposed the two values
> >> for sorting, so the map would always have been in exact reverse order.
> > 
> > Thanks for this.
> > 
> > 
> > Simo.
> > 
> 
> Please see new attached patch.

ack, I iwll push it as is and then fix btreemap.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York