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

[Stephen Gallagher]

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.

> 
> 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 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.

-- 
Stephen Gallagher
RHCE 804006346421761

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-Implement-GetUserAttributes-in-the-InfoPipe.patch
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20090302/51541d10/attachment.ksh>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 259 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20090302/51541d10/attachment.sig>