Re: [Freeipa-devel] [PATCH 0141] Generalize attribute_name-rdata_type conversions
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 Thank you! ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Tomas ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0141] Generalize attribute_name-rdata_type conversions
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? -- Petr^2 Spacek ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0141] Generalize attribute_name-rdata_type conversions
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 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0141] Generalize attribute_name-rdata_type conversions
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 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0141] Generalize attribute_name-rdata_type conversions
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.)
Thank you!
--
Petr^2 Spacek
From 121ca310271b655f05d030b0a841007184de8fcd Mon Sep 17 00:00:00 2001
From: Petr Spacek pspa...@redhat.com
Date: Mon, 8 Apr 2013 19:03:43 +0200
Subject: [PATCH] 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.
Signed-off-by: Petr Spacek pspa...@redhat.com
---
src/ldap_convert.c | 70 +---
src/ldap_convert.h | 10 -
src/ldap_helper.c | 116 +++--
3 files changed, 95 insertions(+), 101 deletions(-)
diff --git a/src/ldap_convert.c b/src/ldap_convert.c
index b1d9a18b4afa83a6fc10b348b1657d73a667a027..f0901a3c333caa0e62205b8fe1eb04261e35eea8 100644
--- a/src/ldap_convert.c
+++ b/src/ldap_convert.c
@@ -24,7 +24,6 @@
#include isc/string.h
#include dns/name.h
-#include dns/rdatatype.h
#include dns/result.h
#include dns/types.h
@@ -41,28 +40,6 @@
#include util.h
#include zone_register.h
-/*
- * Consistency must be preserved in these tables.
- * ldap_dns_records[i] must always corespond to dns_records[i]
- */
-const char *ldap_dns_records[] = {
- ARecord, Record, A6Record,NSRecord,
- CNAMERecord, PTRRecord, SRVRecord, TXTRecord, MXRecord,
- MDRecord,HINFORecord, MINFORecord, AFSDBRecord, SIGRecord,
- KEYRecord, LOCRecord, NXTRecord, NAPTRRecord, KXRecord,
- CERTRecord, DNAMERecord, DSRecord,SSHFPRecord,
- RRSIGRecord, NSECRecord, NULL
-};
-
-const char *dns_records[] = {
- A, , A6,NS,
- CNAME, PTR, SRV, TXT, MX,
- MD,HINFO, MINFO, AFSDB, SIG,
- KEY, LOC, NXT, NAPTR, KX,
- CERT, DNAME, DS,SSHFP,
- RRSIG, NSEC, NULL
-};
-
static isc_result_t explode_dn(const char *dn, char ***explodedp, int notypes);
static isc_result_t explode_rdn(const char *rdn, char ***explodedp,
int notypes);
@@ -436,45 +413,52 @@ cleanup:
return result;
}
+/**
+ * Convert attribute name to dns_rdatatype.
+ *
+ * @param[in] ldap_attribute String with attribute name terminated by \0.
+ * @param[out] rdtype
+ */
isc_result_t
ldap_attribute_to_rdatatype(const char *ldap_attribute, dns_rdatatype_t *rdtype)
{
isc_result_t result;
- unsigned i;
+ unsigned len;
isc_consttextregion_t region;
- for (i = 0; ldap_dns_records[i] != NULL; i++) {
- if (!strcasecmp(ldap_attribute, ldap_dns_records[i]))
- break;
- }
- if (dns_records[i] == NULL)
- return ISC_R_NOTFOUND;
+ len = strlen(ldap_attribute);
+ if (len = LDAP_RDATATYPE_SUFFIX_LEN)
+ return ISC_R_UNEXPECTEDEND;
- region.base = dns_records[i];
- region.length = strlen(region.base);
+ /* Does attribute name end with RECORD_SUFFIX? */
+ if (strcasecmp(ldap_attribute + len - LDAP_RDATATYPE_SUFFIX_LEN,
+ LDAP_RDATATYPE_SUFFIX))
+ return ISC_R_UNEXPECTED;
+
+ region.base = ldap_attribute;
+ region.length = len - LDAP_RDATATYPE_SUFFIX_LEN;
result = dns_rdatatype_fromtext(rdtype, (isc_textregion_t *)region);
if (result != ISC_R_SUCCESS)
- log_error(dns_rdatatype_fromtext() failed);
+ log_error_r(dns_rdatatype_fromtext() failed for attribute '%s',
+ ldap_attribute);
return result;
}
isc_result_t
-rdatatype_to_ldap_attribute(dns_rdatatype_t rdtype, const char **target)
+rdatatype_to_ldap_attribute(dns_rdatatype_t rdtype, char *target,
+ unsigned int size)
{
- unsigned
Re: [Freeipa-devel] [PATCH 0141] Generalize attribute_name-rdata_type conversions.
On 04/08/2013 07:45 PM, Petr Spacek wrote: Hello, 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. Regards, Tomas Hozza ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
