prefixMap refactoring

[Andrew Bartlett]

On Wed, 2009-10-14 at 18:29 +0300, Kamen Mazdrashki wrote:
> Hi list,
> 
> 
> On Mon, Oct 12, 2009 at 00:23, Kamen Mazdrashki <kamen.mazdrashki at postpath.com> wrote:
> > Hi Metze,
> >
> > On Fri, Oct 9, 2009 at 21:20, Stefan (metze) Metzmacher <metze at samba.org> wrote:
> >>> On Thu, Oct 8, 2009 at 19:05, Stefan (metze) Metzmacher <metze at samba.org> wrote:
> >>>> Kamen Mazdrashki schrieb:
> >>>
> >>> Here it is -> patch #0006
> >>>
> >>> Please, bear in mind that this is an intermediate patch.
> >>> I've tested it with RPS-DSSYNC test and it seems to work.
> >>
> >>> -     schema_info = data_blob_hex_string(pfm.ctr.dsdb.mappings, >schemaInfo);
> >>> -     W_ERROR_HAVE_NO_MEMORY(schema_info);
> >>>+      schema_info_blob = data_blob_dup_talloc(pfm.ctr.dsdb.mappings,
> >>>schemaInfo);
> >>
> >> W_ERROR_HAVE_NO_MEMORY(schema_info_blob.data);
> >> is missing
> >>
> > Agree. Fixed.
> >
> >>>@@ -158,18 +171,24 @@ WERROR dsdb_get_oid_mappings_drsuapi(const struct
> >>>dsdb_schema *schema,
> >>>       W_ERROR_HAVE_NO_MEMORY(ctr->mappings);
> >>>
> >>>       for (i=0; i < schema->num_prefixes; i++) {
> >>>+              if (!ber_write_partial_OID_String(ctr->mappings, >&oid_blob,
> >> schema->prefixes[i].oid)) {
> >>>+                      DEBUG(0, ("write_partial_OID failed for %s",
> >>>schema->prefixes[i].oid));
> >>>+                      return WERR_INTERNAL_ERROR;
> >>>+              }
> >>>+
> >>>               ctr->mappings[i].id_prefix      = >schema->prefixes[i].id>>16;
> >>>-              ctr->mappings[i].oid.oid        = >talloc_strndup(ctr->mappings,
> >>>-                                                               >schema->prefixes[i].oid,
> >>>-                                                               >schema->prefixes[i].oid_len - 1);
> >>>-              W_ERROR_HAVE_NO_MEMORY(ctr->mappings[i].oid.oid);
> >>>+              ctr->mappings[i].oid.length     = oid_blob.length;
> >>>+              ctr->mappings[i].oid.binary_oid = oid_blob.data;
> >>>+              W_ERROR_HAVE_NO_MEMORY(ctr->mappings[i].oid.binary_oid);
> >>>       }
> >>
> >> Here we can remote the W_ERROR_HAVE_NO_MEMORY() as we check the result
> >> of ber_write_partial_OID_String().
> >>
> > Agree. Fixed.
> >
> >>
> >>>       if (include_schema_info) {
> >>>+              oid_blob = strhex_to_data_blob(ctr->mappings, >schema->schema_info);
> >>
> >> We should have W_ERROR_HAVE_NO_MEMORY(oid_blob.data) here...
> >>
> >>>               ctr->mappings[i].id_prefix      = 0;
> >>>-              ctr->mappings[i].oid.oid        = >talloc_strdup(ctr->mappings,
> >>>-                                                              >schema->schema_info);
> >>>-              W_ERROR_HAVE_NO_MEMORY(ctr->mappings[i].oid.oid);
> >>>+              ctr->mappings[i].oid.length     = oid_blob.length;
> >>>+              ctr->mappings[i].oid.binary_oid = oid_blob.data;
> >>>+              W_ERROR_HAVE_NO_MEMORY(ctr->mappings[i].oid.binary_oid);
> >>>       }
> >>
> >> ...and can remove W_ERROR_HAVE_NO_MEMORY() here.
> >>
> >> We should handle error directly when they happen.
> >>
> > Agree. Fixed.
> >
> >>>-                      if (strcasecmp(schema->schema_info, >ctr->mappings[i].oid.oid) != 0) {
> >>>+                      oid_blob = strhex_to_data_blob(NULL, >schema->schema_info);
> >>
> >> strhex_to_data_blob() can fail!
> >>
> >>>+                      if (memcmp(oid_blob.data, >ctr->mappings[i].oid.binary_oid, 21) != 0) {
> >>>+                              data_blob_free(&oid_blob);
> >>>                               return WERR_DS_DRA_SCHEMA_MISMATCH;
> >>>                       }
> >>
> >>
> > Agree. Fixed.
> >
> >>>-              if (strncmp(prefixes[i].oid, in, prefixes[i].oid_len) != >0) {
> >>>-                      continue;
> >>>-              }
> >>>+      /* make oid prefix, i.e. oid w/o last subidentifier */
> >>>+      pstr = strrchr(in, '.');
> >>>+      if (!pstr)      return WERR_INVALID_PARAM;
> >>>+      if (pstr < in)  return WERR_INVALID_PARAM;
> >>>+      if ((pstr - in) < 4) return WERR_INVALID_PARAM;
> >>>
> >>>-              val_str = in + prefixes[i].oid_len;
> >>>-              end_str = NULL;
> >>>-              errno = 0;
> >>>+      oid_prefix = talloc_strndup(0, in, pstr - in);
> >>>
> >>>-              if (val_str[0] == '\0') {
> >>>-                      return WERR_INVALID_PARAM;
> >>>+      for (i=0; i < num_prefixes; i++) {
> >>>+              if (strcmp(prefixes[i].oid, oid_prefix) == 0) {
> >>>+                      break;
> >>>               }
> >>>+      }
> >>>
> >>>-              /* two '.' chars are invalid */
> >>>-              if (val_str[0] == '.') {
> >>>-                      return WERR_INVALID_PARAM;
> >>>-              }
> >>>+      talloc_free(oid_prefix);
> >>
> >> I think we should avoid using talloc_strdup() here, and better use
> >> strncmp() as before. This lookup needs to be fast...
> >
> > Please bear in mind that this is a work in progress and this is reference implementation,
> > which is to be removed shortly. This implementation does exactly what  should be done:
> > searches for an exact prefix. That's all.
> > Final implementation will be much faster, even from the original one ;)
> >
> > I am sending you the whole bunch of patches again for review.
> >
> 
> If there is someone that disagree with those patches, please comment.
> If everyone is OK with them, I will ask Anatoliy to merge them in 
> master.

I would have merged them myself, but I've not had the time to test them
(and I've been stuck in 'dynamically created partitions' for over a week
now).  As long as they actually work, they seem like a reasonable first
step. 

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org
Samba Developer, Cisco Inc.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20091015/ddc63069/attachment.pgp>