The mail is getting too big, I'm going to trim the acked patches. On Wed, May 02, 2012 at 02:26:38PM -0400, Stephen Gallagher wrote: > On Wed, 2012-05-02 at 16:21 +0200, Jan Zelený wrote: > > > On Fri, Apr 27, 2012 at 01:47:51PM -0400, Stephen Gallagher wrote: > > > > These patches are built atop Sumit's recent patch "Allow different SID > > > > representations in libidmap". I added the manpage as a single patch near > > > > the end because it was just too much trouble to do it piecemeal > > > > throughout the set. This patch series went through numerous iterations, > > > > so some of the patches may be slightly out of order. Please review as a > > > > whole. The patches themselves are separated primarily the way they are > > > > to make reviewing easier. Some have notes below to guide the reviewer to > > > > changes that may have been revised later but were nontrivial to rewrite > > > > history for. > > > > > > > > Patch 0001: Add objectSID config option > > > > This patch adds an option to specify the objectSID attribute on the LDAP > > > > server, for use when performing ID-mapping > > > > > > Ack > > > > Nack, > > please add ldap_group_objectsid to man page and API definition > > >
ldap_group_objectsid seems to be still missing from src/config/SSSDConfig.py > Fixed. Thanks for catching that. I'd added the user version but not the > group one. > > > > > Patch 0002: Add option to enable id-mapping > > > > > > Ack > > > > Nack, please add ldap_id_mapping to man page > > > > As noted elsewhere, this is covered in patch 0020. > Yes, Ack > > > > Patch 0003: Add sysdb routines for storing ID maps in the cache > > > > > > Nack, you forgot to free tmp_ctx in sysdb_idmap_store_mapping() > > > > Line 254 in the patch: if (old_slice == -1) - you forgot to set the ret > > value > > > > > > Good catches. Fixed both. > > > I'm not sure about name comparison on line 282 in the patch. Since it is > > related to windows, I suppose it should be case insensitive > > > > I think this is okay here in general. The only time this functionality > should ever be used is if we're populating it with known data, which > will therefore be the canonical capitalization. If it's different from > what's there, we'll update it. > Ack > > > > Patch 0004: Add helper routines to the LDAP provider for ID mapping > > > > This contains the meat of the ID-mapping algorithm. See the manpage in > > > > Patch 0020 for full details. > > > > > > Mostly ack, but in sdap_idmap_add_domain I would prefer to check for > > > rangesize > 0 before dividing with its value. > > > > To avoid problems, I think all three values (idmap_lower, idmap_upper and > > rangesize) should be checked for sanity. > > > > Added checks for rangesize <= 0, for upper being lower than the lower > bound and for the difference being less than a rangesize. > Ack > > The rest of the review wil follow, I just didn't want to add one more round > > to > > the review process. > > Thanks > > > 0005: No change, ack remains 0006: No change, ack remains 0007: No change, ack remains 0008: No change, ack remains 0009: No change, ack remains 0010: No change, ack remains 0011: No change, ack remains > > > > Patch 0012: Look up mapped users by UID > > > > > > UID and GID values are 32bit unsigned values in general, right? Wouldn't > > > it > > > be better to use strtouint32() and not strtoint32(), then to avoid errors > > > when a very large ID is requested? The strto(u)int32 functions also set > > > errno to 0 themselves, no need to do it manually. > > > > > The errno thing isn't harmful, so I'm not going to bother with it. It > actually turns out that id_t (as well as uid_t and gid_t) are actually > *signed* values, not unsigned. This has actually caused us issues > elsewhere before. See https://fedorahosted.org/sssd/ticket/1216 for > details. Wasn't that bug just a matter of /proc not representing the data correctly? On my system, both uid_t and gid_t eventually resolve to unsigned int (through a __U32_TYPE constant). > > > > > Patch 0013: Look up mapped groups by name > > > > > > Ack for the changes that actually made it into sdap_save_group(). > > > > > > I noticed that even though the basic filters for group lookups have been > > > changed to not include SDAP_AT_GROUP_GID, the enumeration filters still > > > do include it. Does id-mapped enumeration work? (I haven't tested the > > > patches yet, I'll do it while waiting for the next revision) > > > > I acked patch #21 in the other thread. Ack to patch #13 now > Eep. Thanks, I had completely forgotten about enumeration. I'm going to > add that support as new patches atop these. Let's get these in first, > please. > > > > > Patch 0014: Look up mapped groups by GID > > > > > > Same comment about strtoint32 vs. strtouint32 as I had for patch #12 > > > applies here, too. Otherwise looks good. > > > 0015: No change, ack remains > > > > Patch 0016: Common routine to convert an LDAP blob of the objectSID into > > > > a UNIX ID. > > > > > > Ack, but please fix the compound bracket indentation in > > > sdap_add_incomplete_groups - it's line 155 when all patches are applied. > > > That was fixed, ack 0017: No change, ack remains 0018: No change, ack remains 0019: No change, ack remains > > > > Patch 0020: (Hopefully) comprehensive manpage on the ID-mapping feature. > > > > It's separated into its own include file so that it will be possible to > > > > import it into the manpage for the AD provider when that is available. > > > > > > + components that represent the Active Directory domain identity and > > > + the relative identifier (ID) of the user or group object. > > > > > > Shouldn't the sentence say "RID" in the brackets? > > > > > > Otherwise Ack > > Yes, thanks. Fixed. Ack _______________________________________________ sssd-devel mailing list [email protected] https://fedorahosted.org/mailman/listinfo/sssd-devel
