[SSSD] [PATCHES] ELAPI related patches

Stephen Gallagher sgallagh at redhat.com
Mon Oct 5 17:34:32 UTC 2009


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/05/2009 01:08 PM, Stephen Gallagher wrote:
> On 10/05/2009 12:55 PM, Dmitri Pal wrote:
>> Jakub Hrozek wrote:
>>   
>>> On 10/02/2009 06:49 PM, Dmitri Pal wrote:
>>>     
>>>> Updated versions are attached.
>>>> Comments are inline.
>>>>       
>>>
>>> ACK to all three
>>>
>>>     
>>>>>> 0001:
>>>>>> * Is it possible to change the callbacks in elapi_resolve.c to static?
>>>>>>           
>>>> Done
>>>>       
>>>
>>> Just one thing, it is better to squash the changes to the file that
>>> actually introduces the functions, that way you have them correct in the
>>> first patch, rather than non-static in first patch and fixed to static
>>> in second.
>>>
>>>     
>> I agree, but I think I had two sets of changes in the same file: one set
>> for the first patch (changing to static) and another for the second (do
>> not remember).
>> To avoid doing editing and squashing twice I did it once and added on
>> top of the second patch.
>> Is it a bad practice? Seems like a bit of overkill to me to do it twice
>> but I am a novice to the common practices of git so please guide me :-).
>>
>>   
> 
> Yes, it's a bad practice to have unrelated changes in a patch. Most
> notably because it makes it difficult to apply just one of them if the
> other isn't ready yet.
> 
> There are a couple ways to split patches. The easiest way is probably
> with 'git add -i' (interactive) which allows you to select individual
> portions of a diff for including into a commit.
> 
>> _______________________________________________
>> sssd-devel mailing list
>> sssd-devel at lists.fedorahosted.org
>> https://fedorahosted.org/mailman/listinfo/sssd-devel
>>
>>   
> 
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel

Nack.
sss_getpwnam_done() and friends should return EIO if the sysdb replies
with more than one result, I think. EINVAL implies that the fault was
with the caller, whereas EIO shows us that something went wrong internally.

I don't like the gratuitous use of discard_const_p in
sss_getpwnam_done(). These sync ops are theoretically useful in other
places in our code. I don't want people assuming that they can modify
the contents of the res->data->name field (for example). They should be
talloc_strdup()-ed into the res structure.

sss_useradd.c:
ERROR("A user or group with the same name or UID already exists\n");
should be
ERROR("A user or group with the same name or ID already exists\n");

tools_util.c:
Shouldn't we just make a call to something like a sysdb_getgrent_sync
and compare the resulting complete list with the received groups instead
of making N complete sync calls, where N could be equal to (or greater
than) the total list of groups in the DB?

- -- 
Stephen Gallagher
RHCE 804006346421761

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkrKLhkACgkQeiVVYja6o6PzGQCdEJH9sI9XB9fahw5gwZyPKv3I
23oAn2u9MVzvzjzEQPn3HLP67nAic83Y
=FhY7
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list