Validation on upgradeprovision patches needed

[Matthieu Patou]

  Andrew,

Do you have any pb with those patches ?

On 14/07/2010 00:29, Matthieu Patou wrote:
>  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 Team        http://samba.org