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 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 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/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 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
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
---
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
#include
-#include
#include
#include
@@ -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 *)®ion);
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,
+ unsigne
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
[Freeipa-devel] [PATCH 0141] Generalize attribute_name<->rdata_type conversions.
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.
--
Petr^2 Spacek
From 5d2a61fa9d98fc8eb7f7b68da4cba71a03ad0b33 Mon Sep 17 00:00:00 2001
From: Petr Spacek
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
---
src/ldap_convert.c | 69 --
src/ldap_convert.h | 10 ++--
src/ldap_helper.c | 30 ++--
3 files changed, 47 insertions(+), 62 deletions(-)
diff --git a/src/ldap_convert.c b/src/ldap_convert.c
index b1d9a18b4afa83a6fc10b348b1657d73a667a027..d4e5206e0a7f17500e57fe65f8d1f5d04c97b501 100644
--- a/src/ldap_convert.c
+++ b/src/ldap_convert.c
@@ -24,7 +24,6 @@
#include
#include
-#include
#include
#include
@@ -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,51 @@ 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 *)®ion);
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 i;
+ isc_result_t result;
char rdtype_str[DNS_RDATATYPE_FORMATSIZE];
dns_rdatatype_format(rdtype, rdtype_str, DNS_RDATATYPE_FORMATSIZE);
- for (i = 0; dns_records[i] != NULL; i++) {
- if (!strcmp(rdtype_str, dns_records[i]))
- break;
- }
- if (ldap_dns_records[i] == NULL)
- return ISC_R_NOTFOUND;
-
- *target = ldap_dns_records[i];
+ CHECK(isc_string_copy(target, size, rdtype_str));
+ CHECK(isc_string_append(target, size, LDAP_RDATATYPE_SUFFIX));
return ISC_R_SUCCESS;
+
+cleanup:
+ return result;
}
diff --git a/src/ldap_convert.h b/src/ldap_convert.h
index bcbe395dc3de684de62341f436310e34ee0d90ee..29f6f7fa41d69575e012b847fcef2b268331c2e6 100644
--- a/src/ldap_convert.h
+++ b/src/ldap_convert.h
@@ -22,10 +22,15 @@
#define _LD_LDAP_CONVERT_H_
#include
+#include
#include "str.h"
#include "zone_register.h"
+#define LDAP_RDATATYPE_SUFFIX "Record"
+#define LDAP_RDATATYPE_SUFFIX_LEN (sizeof(LDAP_RDATATYPE_SUFFIX) - 1)
+#define LDAP_RDATATYPE_FORMATSIZE DNS_RDATATYPE_FORMATSIZE + LDAP_RDATATYPE_SUFFIX_LEN + 1
+
/*
* Convert LDAP DN '
