On Fri, 2012-02-24 at 14:45 -0500, Stephen Gallagher wrote: > On Fri, 2012-02-24 at 15:19 +0100, Jan Zeleny wrote: > > Jakub Hrozek <jhro...@redhat.com> wrote: > > > On Wed, Feb 22, 2012 at 01:42:55PM +0100, Jan Zelený wrote: > > > > > On Tue, Feb 21, 2012 at 01:34:08PM +0100, Jan Zelený wrote: > > > > > > > On Thu, Feb 09, 2012 at 06:24:33PM +0100, Jan Zelený wrote: > > > > > > > > > On Tue, Feb 07, 2012 at 04:35:21PM +0100, Jan Zelený wrote: > > > > > > > > > > If all triplets of a netgroup are removed from LDAP server > > > > > > > > > > record, this change won't be projected to the sysdb and all > > > > > > > > > > triplets will remain there. The same situation will happen > > > > > > > > > > when removing all netgroup members. > > > > > > > > > > > > > > > > > > > > This patch fixes these bugs and provides the possibility to > > > > > > > > > > fix similar issues elsewhere. > > > > > > > > > > > > > > > > > > > > https://fedorahosted.org/sssd/ticket/1136 > > > > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > Jan > > > > > > > > > > > > > > > > > > I think that situations like this was the reason we use the > > > > > > > > > list_missing_attrs() function and then remove the "extra" > > > > > > > > > attributes during save. I see that list_missing_attrs() is > > > > > > > > > used > > > > > > > > > only when saving users and services, not netgroups. Have you > > > > > > > > > reproduced the bug with users or groups? (I see the patch also > > > > > > > > > touches sdap_save_user()) > > > > > > > > > > > > > > > > > > Using list_missing_attrs() and extending sysdb_add_netgroup() > > > > > > > > > (maybe with a sysdb_store_user() so the code is similar to > > > > > > > > > users) seems like the proper fix to me. > > > > > > > > > > > > > > > > I looked at the code and I believe the best solution is actually > > > > > > > > to switch the original approach you are referring to to this new > > > > > > > > one I'm proposing. > > > > > > > > > > > > > > > > The entity (user/group/netgroup) in sysdb will always have only > > > > > > > > those attributes set by sdap_attrs_add_*. Therefore it is IMO > > > > > > > > better to delete it directly when preparing the replace > > > > > > > > operation instead of searching for these attributes afterwards. > > > > > > > > This would also mean that the flag I'm setting is redundant and > > > > > > > > the operation can be performed every time. Is there any case > > > > > > > > which I'm missing and which would make my approach unusable? > > > > > > > > > > > > > > You'd have to also take care of options that are passed to > > > > > > > sysdb_save_* directly not set into the attrs array with > > > > > > > sdap_attrs_add - that's attributes like loginShell or gecos. > > > > > > > > > > > > > > So instead of calling list_missing_attrs() on one place just > > > > > > > before > > > > > > > the sysdb_save_* function, you'd have to check all the attributes > > > > > > > manually and that's quite error-prone. > > > > > > > > > > > > In the end I've applied the approach you suggested. I'm sending the > > > > > > patch together with one other patch which proposes a simplification > > > > > > of the list_missing_attrs() function. > > > > > > > > > > > > Thanks > > > > > > Jan > > > > > > > > > > nack, breaks unit tests: > > > > > CC src/providers/krb5/krb5_utils_tests-krb5_utils.o > > > > > > > > > > src/tests/sysdb-tests.c: In function ‘test_odd_characters’: > > > > > src/tests/sysdb-tests.c:2788:30: warning: passing argument 5 of > > > > > ‘sysdb_add_netgroup’ makes pointer from integer without a cast > > > > > [enabled > > > > > by default] ./src/db/sysdb.h:531:5: note: expected ‘char **’ but > > > > > argument is of type ‘int’ src/tests/sysdb-tests.c:2788:30: error: too > > > > > few arguments to function ‘sysdb_add_netgroup’ ./src/db/sysdb.h:531:5: > > > > > note: declared here > > > > > > > > Patches attached > > > > > > > > Jan > > > > > > 0001: One last thing, please don't use the legacy DEBUG levels in > > > sysdb_add_netgroup() > > > > > > 0002: While we're refactoring list_missing_attrs() already, I think > > > another > > > nice thing would be to either modify it to return "NULL" instead of "{ > > > NULL > > > }" if there are no missing attributes so the callers don't have to free > > > the "{ NULL }" array themselves OR alternatively modify consumers to > > > understant the "{ NULL }" array. Letting the list_missing_attrs() free the > > > array is error-prone as can be seen in sdap_save_service() where we forgot > > > to do it. > > > > Both suggestions were addressed. New patches attached. > > Ack to the delta of these latest patches for master and sssd-1-8. > Trusting Jakub's judgement on the rest.
Pushed to master and sssd-1-8
signature.asc
Description: This is a digitally signed message part
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
