Jakub Hrozek <jhro...@redhat.com> wrote: > On 07/22/2011 11:53 AM, Jan Zeleny wrote: > > Jakub Hrozek <jhro...@redhat.com> wrote: > >> On 07/22/2011 09:18 AM, Jan Zeleny wrote: > >>> Jakub Hrozek<jhro...@redhat.com> wrote: > >>>> On 07/21/2011 01:57 PM, Jan Zelený wrote: > >>>>>> https://fedorahosted.org/sssd/ticket/916 > >>>>> > >>>>> Nack, > >>>>> please look at following lines, there are parts of code which might > >>>>> need update along with your changes: > >>>>> > >>>>> providers/ldap/sdap_async_accounts.cz : 728 > >>>>> db/sysdb_ops.c : 1576 > >>>> > >>>> Right - the problem would be if a group changed its gid from nonzero > >>>> to zero, the sysdb update would keep the original version. It's a > >>>> corner case but we'd better cover it. The problem is there even with > >>>> non-posix groups so I'm sending a separate fix. > >>>> > >>>> Two patches attached now. > >>> > >>> Nack, > >>> I think you tried to fix the issue we discussed yesterday at the wrong > >>> place. The place your patch 0001 is taking care of only needs comsmetic > >>> change (the condition is needlesly complicated. Something like if > >>> (posix_group&& OUT_OF_ID_RANGE(...)) will do, since now gid == 0 always > >>> implies posix_group == true. > >> > >> Patch 0001 does not deal with gid=0 implying non-posix. That's what > >> patch #2 does. > > > > I know that and I'm not arguing about that. My thought is valid, but I > > made a mistake in explanation. The condition if (posix_group || gid != > > 0) should be if (posix_group) because posix_group == true always implies > > gid != 0. > > Sure that's exactly what the the second patch does: > > if (posix_group) { > /* Check if the ID in in (min_id,max_id) range */ > else { > /* Explicitly add GID 0 for non-posix groups */ > }
Sorry, I did't examine both patches at once. I stand corrected. > > Also, the "else" part of your patch is redundant, see how the function > > sdap_save_group handles the gid (especially what sysdb_store_group does). > > The else handles the corner case where a group would be saved with GID>0 > and needs to be updated to GID=0. Due to sysdb_store_group only > rewriting GID if it is > 0, I explicitly add the GID to the group > attributes. This scenario now works, I tested it. > > If it is unreadable, maybe a wrapper around sysdb_store_group() that > always rewrites GID would be better? I agree that the idea is not clear at first sight (even I didn't see it now). The corner case is not handled and we leave it for upper layers - this approach is correct, I'm just a bit worried that in the future this case can be forgotten and a code which won't be handling it will make its way to SSSD. The wrapper you mentioned seems to be a safe way to avoid this, but only in case we will start using it and not just make it available. Another possibility that comes to my mind is to emphasize this in a comment. > > >>> The issue you are trying to fix should be fixed in sysdb_ops.c in the > >>> place we were looking at yesterday. > >> > >> No, that would be wrong abstraction. The sysdb function does not need to > >> know about posix/non-posix. > > > > I'm sorry, I didn't express myself clearly. I was referring to the error > > where, in some corner cases, the group can be marked as non-posix, but > > the GID number will be > 0 because it was not overwritten (see the > > condition on line 1576). I didn't analyze the code deeply, but I believe > > this can happen sometimes. But as I said yesterday, this is completely > > different issue and therefore can (and should) be solved separately. > > See above. The patches themselves are ok now. ACK Jan _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
