prefixMap refactoring

[Anatoliy Atanasov]

All pushed, thanks.

Anatoliy
----- Original Message -----
> From: samba-technical-bounces at lists.samba.org <samba-technical-bounces at lists.samba.org>
> To: Kamen Mazdrashki <kamen.mazdrashki at postpath.com>, metze at samba.org <metze at samba.org>
> Cc: tridge at samba.org <tridge at samba.org>, samba-technical at lists.samba.org <samba-technical at lists.samba.org>, abartlet at samba.org <abartlet at samba.org>
> Sent: Wednesday, October 14, 2009 6:29:54 PM GMT+0200 Europe;Athens
> Subject: RE: prefixMap refactoring

> > 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.
> 
> Thanks,
> Kamen Mazdrashki
> kamen.mazdrashki at postpath.com
> http://repo.or.cz/w/Samba/kamenim.git
> -------------------------------------
> CISCO SYSTEMS BULGARIA EOOD
> http://www.cisco.com/global/BG/