On Sun, Sep 13, 2015 at 01:43:03PM +0200, Pavel Reichl wrote:
Hello,
this patch works around the bug in nsupdate
(
https://bugzilla.redhat.com/show_bug.cgi?id=1262430)
thanks!
From 4ce49023285b9435abcd3b86d88b40151b76b24c Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)redhat.com>
Date: Sat, 12 Sep 2015 09:09:35 -0400
Subject: [PATCH] DDNS: execute nsupdate for single update of PTR rec
nsupdate fails definitely if any of update request fails when GSSAPI is used.
As tmp solution nsupdate is executed for each update.
Resolves:
https://fedorahosted.org/sssd/ticket/2783
---
src/providers/dp_dyndns.c | 60 ++++++++++++++++++++++-----------
src/providers/dp_dyndns.h | 11 +++++--
src/providers/ldap/sdap_dyndns.c | 71 +++++++++++++++++++++++++++++++++++++++-
3 files changed, 120 insertions(+), 22 deletions(-)
diff --git a/src/providers/dp_dyndns.c b/src/providers/dp_dyndns.c
index 0577743cb2daca9c0e86b5beb6bf059ee7b5783f..845ab7adf6ec3341d19a0d3b7486dd686b3a903f
100644
--- a/src/providers/dp_dyndns.c
+++ b/src/providers/dp_dyndns.c
@@ -52,6 +52,25 @@ struct sss_iface_addr {
struct sockaddr_storage *addr;
};
+struct sockaddr_storage*
+sss_iface_addr_get_item(struct sss_iface_addr *address)
+{
+ if (address == NULL) {
+ return NULL;
+ }
+
+ return address->addr;
+}
+
+struct sss_iface_addr *sss_iface_addr_get_next(struct sss_iface_addr *address)
+{
+ if (address) {
+ return address->next;
+ }
+
+ return NULL;
+}
+
void sss_iface_addr_concatenate(struct sss_iface_addr **list,
struct sss_iface_addr *list2)
{
@@ -294,34 +313,34 @@ nsupdate_msg_add_fwd(char *update_msg, struct sss_iface_addr
*addresses,
}
The intent is good, but I think the implementation is too complex. The
caller keeps both add iter and del and then passes both to
nsupdate_msg_add_ptr which has quite duplicated code.
The add and delete blocks in nsupdate_msg_add_ptr now differ only in
uding add or delete in the message, right? Could we simplify that? For
readability's sake, we could even create wrappers like
nsupdate_msg_add_ptr() and +nsupdate_msg_del_ptr() that would call an
internal function with only either add or del.
static char *
-nsupdate_msg_add_ptr(char *update_msg, struct sss_iface_addr *addresses,
+nsupdate_msg_add_ptr(char *update_msg,
+ struct sockaddr_storage *add_address,
const char *hostname, int ttl, uint8_t remove_af,
- struct sss_iface_addr *old_addresses)
+ struct sockaddr_storage *del_address)
{
- struct sss_iface_addr *new_record, *old_record;
char *strptr;
uint8_t *addr;
- DLIST_FOR_EACH(old_record, old_addresses) {
- switch(old_record->addr->ss_family) {
+ if (del_address != NULL) {
+ switch(del_address->ss_family) {
case AF_INET:
if (!(remove_af & DYNDNS_REMOVE_A)) {
- continue;
+ return NULL;
}
- addr = (uint8_t *) &((struct sockaddr_in *)
old_record->addr)->sin_addr;
+ addr = (uint8_t *) &((struct sockaddr_in *) del_address)->sin_addr;
break;
case AF_INET6:
if (!(remove_af & DYNDNS_REMOVE_AAAA)) {
- continue;
+ return NULL;
}
- addr = (uint8_t *) &((struct sockaddr_in6 *)
old_record->addr)->sin6_addr;
+ addr = (uint8_t *) &((struct sockaddr_in6 *)
del_address)->sin6_addr;
break;
default:
DEBUG(SSSDBG_CRIT_FAILURE, "Unknown address family\n");
return NULL;
}
- strptr = resolv_get_string_ptr_address(update_msg,
old_record->addr->ss_family,
+ strptr = resolv_get_string_ptr_address(update_msg, del_address->ss_family,
addr);
if (strptr == NULL) {
return NULL;
@@ -336,23 +355,25 @@ nsupdate_msg_add_ptr(char *update_msg, struct sss_iface_addr
*addresses,
if (update_msg == NULL) {
return NULL;
}
+
+ return update_msg;
}
/* example: update add 11.78.16.10.in-addr.arpa. 85000 in PTR
testvm.example.com */
- DLIST_FOR_EACH(new_record, addresses) {
- switch(new_record->addr->ss_family) {
+ if (add_address != NULL) {
+ switch(add_address->ss_family) {
case AF_INET:
- addr = (uint8_t *) &((struct sockaddr_in *)
new_record->addr)->sin_addr;
+ addr = (uint8_t *) &((struct sockaddr_in *) add_address)->sin_addr;
break;
case AF_INET6:
- addr = (uint8_t *) &((struct sockaddr_in6 *)
new_record->addr)->sin6_addr;
+ addr = (uint8_t *) &((struct sockaddr_in6 *)
add_address)->sin6_addr;
break;
default:
DEBUG(SSSDBG_CRIT_FAILURE, "Unknown address family\n");
return NULL;
}
- strptr = resolv_get_string_ptr_address(update_msg,
new_record->addr->ss_family,
+ strptr = resolv_get_string_ptr_address(update_msg, add_address->ss_family,
addr);
if (strptr == NULL) {
return NULL;
@@ -469,11 +490,12 @@ done:
}
errno_t
+
Extra line.
be_nsupdate_create_ptr_msg(TALLOC_CTX *mem_ctx, const char *realm,
const char *servername, const char *hostname,
const unsigned int ttl, uint8_t remove_af,
- struct sss_iface_addr *addresses,
- struct sss_iface_addr *old_addresses,
+ struct sockaddr_storage *add_address,
+ struct sockaddr_storage *del_address,
char **_update_msg)
{
errno_t ret;
@@ -490,8 +512,8 @@ be_nsupdate_create_ptr_msg(TALLOC_CTX *mem_ctx, const char *realm,
goto done;
}
- update_msg = nsupdate_msg_add_ptr(update_msg, addresses, hostname,
- ttl, remove_af, old_addresses);
+ update_msg = nsupdate_msg_add_ptr(update_msg, add_address, hostname,
+ ttl, remove_af, del_address);
if (update_msg == NULL) {
ret = ENOMEM;
goto done;
diff --git a/src/providers/dp_dyndns.h b/src/providers/dp_dyndns.h
index 9f72331b6fd68e17e9eb91505a13fc839d3f54e1..1bbe806354c2d8b57fe323703d5d993ac0f6dc0f
100644
--- a/src/providers/dp_dyndns.h
+++ b/src/providers/dp_dyndns.h
@@ -98,8 +98,8 @@ errno_t
be_nsupdate_create_ptr_msg(TALLOC_CTX *mem_ctx, const char *realm,
const char *servername, const char *hostname,
const unsigned int ttl, uint8_t remove_af,
- struct sss_iface_addr *addresses,
- struct sss_iface_addr *old_addresses,
+ struct sockaddr_storage *add_address,
+ struct sockaddr_storage *del_address,
char **_update_msg);
/* Returns:
@@ -133,4 +133,11 @@ errno_t
sss_get_dualstack_addresses(TALLOC_CTX *mem_ctx,
struct sockaddr *ss,
struct sss_iface_addr **_iface_addrs);
+
+struct sss_iface_addr *
+sss_iface_addr_get_next(struct sss_iface_addr *address);
+
+struct sockaddr_storage*
+sss_iface_addr_get_item(struct sss_iface_addr *address);
When we agree on the rest, please add tests for these two functions. I
know they are trivial, but we should keep adding tests.
+
#endif /* DP_DYNDNS_H_ */
diff --git a/src/providers/ldap/sdap_dyndns.c b/src/providers/ldap/sdap_dyndns.c
index 2a179fd1b5e88bdf2442657ff6fa1dcc55417467..670a9e82e001dcb22c536013cfaa345684b86bc1
100644
--- a/src/providers/ldap/sdap_dyndns.c
+++ b/src/providers/ldap/sdap_dyndns.c
@@ -60,6 +60,9 @@ struct sdap_dyndns_update_state {
enum be_nsupdate_auth auth_type;
bool fallback_mode;
char *update_msg;
+ struct sss_iface_addr *ptr_add_addr_iter;
+ struct sss_iface_addr *ptr_del_addr_iter;
+ bool del_phase;
Do we need both iterators /and/ a flag? Can't we get away with either a
single iterator and a flag or two iterators?
};
static void sdap_dyndns_update_addrs_done(struct tevent_req *subreq);
@@ -70,6 +73,8 @@ static errno_t sdap_dyndns_update_step(struct tevent_req *req);
static errno_t sdap_dyndns_update_ptr_step(struct tevent_req *req);
static void sdap_dyndns_update_done(struct tevent_req *subreq);
static void sdap_dyndns_update_ptr_done(struct tevent_req *subreq);
+static bool sdap_dyndns_next_ptr_record(struct sdap_dyndns_update_state *state,
+ struct tevent_req *req);
struct tevent_req *
sdap_dyndns_update_send(TALLOC_CTX *mem_ctx,
@@ -106,6 +111,9 @@ sdap_dyndns_update_send(TALLOC_CTX *mem_ctx,
state->ev = ev;
state->opts = opts;
state->auth_type = auth_type;
+ state->ptr_add_addr_iter = NULL;
+ state->ptr_del_addr_iter = NULL;
+ state->del_phase = true;
/* fallback servername is overriden by user option */
conf_servername = dp_opt_get_string(opts, DP_OPT_DYNDNS_SERVER);
@@ -381,6 +389,15 @@ sdap_dyndns_update_done(struct tevent_req *subreq)
}
talloc_free(state->update_msg);
+
+ /* init iterator for addresses to be deleted */
+ state->ptr_del_addr_iter = state->dns_addrlist;
+ if (state->ptr_del_addr_iter == NULL) {
+ /* init iterator for addresses to be added */
+ state->del_phase = false;
+ state->ptr_add_addr_iter = state->addresses;
+ }
+
ret = sdap_dyndns_update_ptr_step(req);
if (ret != EOK) {
tevent_req_error(req, ret);
@@ -396,6 +413,7 @@ sdap_dyndns_update_ptr_step(struct tevent_req *req)
struct sdap_dyndns_update_state *state;
const char *servername;
struct tevent_req *subreq;
+ struct sockaddr_storage *add_item, *del_item;
Similarly here, we can only be adding /or/ deleting, so we could use
just one variable, right?
state = tevent_req_data(req, struct sdap_dyndns_update_state);
@@ -405,10 +423,28 @@ sdap_dyndns_update_ptr_step(struct tevent_req *req)
servername = state->servername;
}
+ del_item = NULL;
+ add_item = NULL;
+
+ if (state->del_phase) {
+ /* add 'del' addresses */
+ del_item = sss_iface_addr_get_item(state->ptr_del_addr_iter);
+ if (del_item == NULL) {
+ return EIO;
+ }
+
+ } else {
+ /* add 'add' addresses */
+ add_item = sss_iface_addr_get_item(state->ptr_add_addr_iter);
+ if (add_item == NULL) {
+ return EIO;
+ }
+ }
+
ret = be_nsupdate_create_ptr_msg(state, state->realm,
servername, state->hostname,
state->ttl, state->remove_af,
- state->addresses, state->dns_addrlist,
+ add_item, del_item,
&state->update_msg);
if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE, "Can't get addresses for DNS update\n");
@@ -454,10 +490,14 @@ sdap_dyndns_update_ptr_done(struct tevent_req *subreq)
}
}
+ if (sdap_dyndns_next_ptr_record(state, req)) return;
+
tevent_req_error(req, ret);
return;
}
+ if (sdap_dyndns_next_ptr_record(state, req)) return;
+
tevent_req_done(req);
}
@@ -801,3 +841,32 @@ sdap_dyndns_timer_conn_recv(struct tevent_req *req)
TEVENT_REQ_RETURN_ON_ERROR(req);
return EOK;
}
+
+static bool sdap_dyndns_next_ptr_record(struct sdap_dyndns_update_state *state,
+ struct tevent_req *req)
If you add a new function to an existing record, please add it below the
first function that calls it. The tevent requests should start with
_send, continue through _recv and be readable from top to bottom without
jumping around.
I would also prefer if we returned errno here instead of bool, with
EAGAIN if we continue with next step.
+{
+ if (state->del_phase) {
+ /* iterate to next address to delete */
+ state->ptr_del_addr_iter =
sss_iface_addr_get_next(state->ptr_del_addr_iter);
+ if (state->ptr_del_addr_iter == NULL) {
+ /* init iterator for addresses to be added */
+ state->del_phase = false;
+ state->ptr_add_addr_iter = state->addresses;
+ }
+ } else {
+ /* iterate to next address to add */
+ state->ptr_add_addr_iter =
sss_iface_addr_get_next(state->ptr_add_addr_iter);
+ }
+
+ if (state->ptr_add_addr_iter != NULL || state->ptr_del_addr_iter != NULL) {
+ errno_t ret;
+
+ state->fallback_mode = false;
+ ret = sdap_dyndns_update_ptr_step(req);
+ if (ret == EOK) {
+ return true;
+ }
+ }
+
+ return false;
+}
--
2.4.3
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel