[SSSD] [PATCHES] ELAPI related patches

Dmitri Pal dpal at redhat.com
Mon Oct 5 17:45:40 UTC 2009


Stephen Gallagher wrote:
> 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?
>
Are you sure you commented to the right patch?
You comments seems to be a review of Jakub's patch and this is my patch



_______________________________________________
sssd-devel mailing list
sssd-devel at lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel

-- 
Thank you,
Dmitri Pal

Engineering Manager IPA project,
Red Hat Inc.


-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/




More information about the sssd-devel mailing list