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

[Tomas Babej]

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.

Tomas