[Freeipa-devel] [PATCH 0141] Generalize attribute_name<->rdata_type conversions

[Petr Spacek]

On 14.5.2013 17:42, Tomas Babej wrote:
> On 05/14/2013 02:01 PM, Petr Spacek wrote:
>> On 14.5.2013 11:45, Tomas Babej wrote:
>>> On 05/10/2013 04:57 PM, Petr Spacek wrote:
>>>> On 6.5.2013 17:40, Tomas Hozza wrote:
>>>>> On 04/08/2013 07:45 PM, Petr Spacek wrote:
>>>>>> Generalize attribute_name<->rdata_type conversions.
>>>>>>
>>>>>> Attribute names are generated on-the-fly: String "Record" is appended
>>>>>> to textual representation of DNS RDATA type.
>>>>>>
>>>>>> String "Record" is cut down from the attribute name during
>>>>>> attribute name to rdata type conversion.
>>>>>>
>>>>>>  From now, the plugin doesn't add artificial limitation to supported
>>>>>> record types.
>>>>>
>>>>> ACK.
>>>>>
>>>>> The patch looks good. (I didn't do functional test)
>>>>>
>>>>> Cosmetic issue:
>>>>> I think it would be good to dynamically allocate "mod_type" in LDAPMod
>>>>> in every case and include the "mod_type" memory freeing in
>>>>> free_ldapmod() function. Now one has to be be careful when it is
>>>>> statically or dynamically allocated. Before it was static in every case.
>>>>
>>>> It is good idea. This version of the patch contains ldap_mod_create()
>>>> function which allocates the whole structure including mod_type of fixed
>>>> size. All writes to mod_type checks the array length, so it should not cause
>>>> any harm.
>>>>
>>>> The function modify_soa_record() still uses statically allocated LDAPMod
>>>> structure with statically allocated strings for mod_type, but the LDAPMod
>>>> structure never leave this function. There are no calls to ldap_mod_create()
>>>> and ldap_mod_free(), so I think it is obvious.
>>>>
>>>> Tbabej, please try to dynamically update some A records with sync_ptr
>>>> enabled. (And of course the support for some new type, like TLSA.)
>>> For the existing record types, the patch works fine.
>>>
>>> For any new types, a schema change is still required, since record types are
>>> still hardcoded in LDAP schema:
>>>
>>>   LDAP error: Object class violation: attribute "tlsarecord" not allowed
>>
>> That is expected behaviour. The point is that schema change is much simpler
>> than recompiling the bind-dyndb-ldap (and can be done at run-time).
>>
>> BTW schema file contains instructions how to add support for any record type
>> in a way compatible with rest of the world:
>> http://drift.uninett.no/nett/ip-nett/dnsattributes.schema
>>
>> Was it ACK?
>>
> I was not nacking, just pointing out. Tested with changed schema, works as
> expected.
>
> Here, have my ACK.

Pushed to master: 35ac3e422376cc28040d03c7550c2c68a967a0cf

-- 
Petr^2 Spacek