Validation on upgradeprovision patches needed

[Matthieu Patou]

  On 13/07/2010 03:04, Andrew Bartlett wrote:
> On Tue, 2010-07-13 at 02:58 +0400, Matthieu Patou wrote:
>> On 13/07/2010 02:41, Andrew Bartlett wrote:
>>> On Tue, 2010-07-13 at 01:16 +0400, Matthieu Patou wrote:
>>>> Hello Andrews,
>>>>
>>>> As metze and Jelmer are on vacation,
>>>> can one of you had final look on
>>>> http://git.samba.org/?p=mat/samba.git;a=shortlog;h=refs/heads/upgradeprovision-review
>>>> and push it if it's ok.
>>>>
>>>> I normally addressed the remarks of Matthias and those made by metze on IRC.
>>> These look good, but I hope you won't mind if make some comments:
>>>
>>> In s4 dsdb: Use the changereplmetadata control
>>> http://git.samba.org/?p=mat/samba.git;a=commitdiff;h=351ecebb1b294012bc311ff9b38e24a2b6cb77e0
>>>
>>> You do a check for
>>> +               objectclass_el = ldb_msg_find_element(res->msgs[0], "objectClass");
>>> +               if (is_urgent&&   replmd_check_urgent_objectclass(objectclass_el,
>>> +                                                               situation)) {
>>> +                       *is_urgent = true;
>>> +               }
>>>
>>> In both arms of the if (rmd_is_provided) statement.
>>>
>>> But in any case, if we are offline doing an upgradeprovision, then it
>>> isn't urgent to replicate anything.
>> Well even though this control is used only by upgradeprovision right now
>> it's better to have this just in case ?
>> This came form the fact that I kept in this branch of the test almost
>> the same logic.
> That is quite reasonable.  Perhaps then it would be better to push it
> out of the if() statement?
Well that's not so easy, because before the if we do not have the res 
variable which is the result of the ldb_search.
So pushing it after the if looks like at a first glance like a good 
choice but in the else part (so if we do not have the replace_metadata 
control)
there is this:

                 for (i=0; i<msg->num_elements; i++) {
                         struct ldb_message_element *old_el;
                         old_el = ldb_msg_find_element(res->msgs[0], 
msg->elements[i].name);
                         ret = replmd_update_rpmd_element(ldb, msg, 
&msg->elements[i], old_el, &omd, schema, seq_num,
                                                          
our_invocation_id, now);
                         if (ret != LDB_SUCCESS) {
                                 return ret;
                         }

                         if (is_urgent && !*is_urgent && (situation == 
REPL_URGENT_ON_UPDATE)) {
                                 *is_urgent = 
replmd_check_urgent_attribute(&msg->elements[i]);
                         }

                 }

So putting

                if (is_urgent&&   replmd_check_urgent_objectclass(objectclass_el,
                                                                situation)) {
                        *is_urgent = true;
                }

After the if would change the logic, so I guess it's better to leave it 
like that.
>>> In s4 upgradeprovision: do not copy RID Set it's automaticaly created by
>>> the RID manager
>>> http://git.samba.org/?p=mat/samba.git;a=commitdiff;h=48773341a19e94ac116bc2aa5327d595c48c4212
>>> +    try:
>>> +        if str(reference[0].get("cn"))  == "RID Set":
>>> +            skip = True
>>>
>>> Is this the best way to identify the RID Set?
>>>
>> obviously objectClass: rIDSet is much better
> Thanks,
So I propose something like this:

          if str(reference[0].get("cn"))  == "RID Set":
-            skip = True
+            for klass in reference[0].get("objectClass"):
+                if str(klass).lower == "ridset":
+                    skip = True


I pushed my upgradeprovision-review branch.

It pass make tests on my computer.

Please push it if you think it's ok.

Matthieu.


-- 
Matthieu Patou
Samba Teamhttp://samba.org