[PATCH] DDNS: execute nsupdate for single update of PTR rec

[Pavel Reichl]

Hello,

this patch works around the bug in nsupdate (https://bugzilla.redhat.com/show_bug.cgi?id=1262430)

thanks!

[Jakub Hrozek]

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 
> 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
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Pavel Reichl]

On 09/14/2015 05:29 PM, Jakub Hrozek wrote:
> On Sun, Sep 13, 2015 at 01:43:03PM +0200, Pavel Reichl wrote:

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.

Hello Jakub, thanks for comments. I hope I addressed all of them except the request for tests - I'll do it with next iteration of patch. Thanks!

[Jakub Hrozek]

On Mon, Sep 14, 2015 at 07:53:06PM +0200, Pavel Reichl wrote:
> On 09/14/2015 05:29 PM, Jakub Hrozek wrote:
> > On Sun, Sep 13, 2015 at 01:43:03PM +0200, Pavel Reichl wrote:
>                         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.
>
> Hello Jakub, thanks for comments. I hope I addressed all of them except the request for tests - I'll do it with next iteration of patch. Thanks!
> From e48fc2844ab41cec3a2aa98b01468c673fb33c74 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> 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        | 155 +++++++++++++++++++++++----------------
>  src/providers/dp_dyndns.h        |  11 ++-
>  src/providers/ldap/sdap_dyndns.c |  77 ++++++++++++++++++-
>  3 files changed, 175 insertions(+), 68 deletions(-)
>
> diff --git a/src/providers/dp_dyndns.c b/src/providers/dp_dyndns.c
> index 0577743cb2daca9c0e86b5beb6bf059ee7b5783f..e78f7586b22f42572749d9407adad6b4c231bf4d 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)

We should rename this one, see below..
> +{
> 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)
>  {
> @@ -293,80 +312,92 @@ nsupdate_msg_add_fwd(char *update_msg, struct sss_iface_addr *addresses,
>      return talloc_asprintf_append(update_msg, "send\n");
>  }
>
> -static char *
> -nsupdate_msg_add_ptr(char *update_msg, struct sss_iface_addr *addresses,
>                 const char *hostname, int ttl, uint8_t remove_af,
>                 struct sss_iface_addr *old_addresses)
>
> +static uint8_t *nsupdate_convert_address(struct sockaddr_storage *add_address)
> +{
> uint8_t *addr;
> switch(add_address->ss_family) {
> case AF_INET:
>    addr = (uint8_t *) &((struct sockaddr_in *) add_address)->sin_addr;
>    break;
> case AF_INET6:
>    addr = (uint8_t *) &((struct sockaddr_in6 *) add_address)->sin6_addr;
>    break;
> default:
>    DEBUG(SSSDBG_CRIT_FAILURE, "Unknown address family\n");
>    addr = NULL;
>    break;
> }
> return addr;
>
> +}

Not directly related to this patch, but this going back and forth between
sockaddr_storage and uint8_t pointer made me wonder if we should change
resolv_get_string_ptr_address to accept sockaddr storage instead of
these hacks?
> +static bool nsupdate_should_remove_addr(int address_family, uint8_t remove_af)
> +{
> bool ret = false;
> switch(address_family) {
> case AF_INET:
>    if (remove_af & DYNDNS_REMOVE_A) {
>        ret = true;
>    }
>    break;
> case AF_INET6:
>    if (remove_af & DYNDNS_REMOVE_AAAA) {
>        ret = true;
>    }
>    break;
> default:
>    DEBUG(SSSDBG_CRIT_FAILURE, "Unknown address family\n");
>    ret = false;
> }
> return ret;
>
> +}
>
> +static char *nsupdate_msg_add_ptr(char *update_msg,
>                               struct sockaddr_storage *address,
>                               const char *hostname,
>                               int ttl,
>                               uint8_t remove_af,
>                               bool delete)
>
> {
>
> 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) {
>    case AF_INET:
>        if (!(remove_af & DYNDNS_REMOVE_A)) {
>            continue;
>        }
>        addr = (uint8_t *) &((struct sockaddr_in *) old_record->addr)->sin_addr;
>        break;
>    case AF_INET6:
>        if (!(remove_af & DYNDNS_REMOVE_AAAA)) {
>            continue;
>        }
>        addr = (uint8_t *) &((struct sockaddr_in6 *) old_record->addr)->sin6_addr;
>        break;
>    default:
>        DEBUG(SSSDBG_CRIT_FAILURE, "Unknown address family\n");
>        return NULL;
>    }
> if (delete
>        && !nsupdate_should_remove_addr(address->ss_family, remove_af)) {

This seems a bit strange to me, because if we hit this condition, then
the whole request is aborted. Shouldn't we just skip this address
instead?
>    return NULL;
> }
>    strptr = resolv_get_string_ptr_address(update_msg, old_record->addr->ss_family,
>                                           addr);
>    if (strptr == NULL) {
>        return NULL;
>    }
>
> addr = nsupdate_convert_address(address);
>
> if (addr == NULL) {
>    return NULL;
>
> }
>
> strptr = resolv_get_string_ptr_address(update_msg, address->ss_family,
>                                       addr);
>
> if (strptr == NULL) {
>    return NULL;
>
> }
>
> if (delete) {
>     /* example: update delete 38.78.16.10.in-addr.arpa. in PTR */
>     update_msg = talloc_asprintf_append(update_msg,
>                                         "update delete %s in PTR\n"
>                                         "send\n",
>                                         strptr);
>    talloc_free(strptr);
>    if (update_msg == NULL) {
>        return NULL;
>    }
> }
> /* 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) {
>    case AF_INET:
>        addr = (uint8_t *) &((struct sockaddr_in *) new_record->addr)->sin_addr;
>        break;
>    case AF_INET6:
>        addr = (uint8_t *) &((struct sockaddr_in6 *) new_record->addr)->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,
>                                           addr);
>    if (strptr == NULL) {
>        return NULL;
>    }
> } else {
>     /* example: update delete 38.78.16.10.in-addr.arpa. in PTR */
>     update_msg = talloc_asprintf_append(update_msg,
>                                         "update add %s %d in PTR %s.\n"
>                                         "send\n",
>                                         strptr, ttl, hostname);
>    talloc_free(strptr);
>    if (update_msg == NULL) {
>        return NULL;
>    }
>
> }
>
> talloc_free(strptr);
>
> if (update_msg == NULL) {
>    return NULL;
>
> }
>
> return update_msg;
>
> @@ -472,8 +503,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 *address,
>                       bool del_phase,
>                       char **_update_msg)
>
> {
>      errno_t ret;
> @@ -490,8 +521,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, address, hostname, ttl,
>                                  remove_af, del_phase);
> if (update_msg == NULL) {
>        ret = ENOMEM;
>        goto done;
>
> diff --git a/src/providers/dp_dyndns.h b/src/providers/dp_dyndns.h
> index 9f72331b6fd68e17e9eb91505a13fc839d3f54e1..75c595c22976b498fc08e88ee1015cbce0646822 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 *address,
>                       bool del_phase,

I don't think the option should contain 'phase' in its name, the fact
that we have some phases is a detail from the sdap layer.
>                         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);
>
> #endif /* DP_DYNDNS_H_ */
> diff --git a/src/providers/ldap/sdap_dyndns.c b/src/providers/ldap/sdap_dyndns.c
> index 2a179fd1b5e88bdf2442657ff6fa1dcc55417467..b1246026c7f44d5cbe7766522b5cb6e73c4bd2cb 100644
> --- a/src/providers/ldap/sdap_dyndns.c
> +++ b/src/providers/ldap/sdap_dyndns.c
> @@ -60,6 +60,8 @@ struct sdap_dyndns_update_state {
>      enum be_nsupdate_auth auth_type;
>      bool fallback_mode;
>      char *update_msg;
> struct sss_iface_addr *ptr_addr_iter;
> bool del_phase;
>
> };
>
> static void sdap_dyndns_update_addrs_done(struct tevent_req *subreq);
> @@ -70,6 +72,9 @@ 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 errno_t
> +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,8 @@ sdap_dyndns_update_send(TALLOC_CTX *mem_ctx,
>      state->ev = ev;
>      state->opts = opts;
>      state->auth_type = auth_type;
>
> state->ptr_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 +388,15 @@ sdap_dyndns_update_done(struct tevent_req *subreq)
>      }
>  talloc_free(state->update_msg);
> /* init iterator for addresses to be deleted */
> state->ptr_addr_iter = state->dns_addrlist;
> if (state->ptr_addr_iter == NULL) {
>    /* init iterator for addresses to be added */
>    state->del_phase = false;
>    state->ptr_addr_iter = state->addresses;
> }
> ret = sdap_dyndns_update_ptr_step(req);
>    if (ret != EOK) {
>        tevent_req_error(req, ret);
>
> @@ -396,6 +412,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 *item;

It's not an 'item', it's an address (The function is misnamed, too)
>  state = tevent_req_data(req, struct sdap_dyndns_update_state);
>
> @@ -405,10 +422,21 @@ sdap_dyndns_update_ptr_step(struct tevent_req *req)
>          servername = state->servername;
>      }
> ret = be_nsupdate_create_ptr_msg(state, state->realm,
>                                 servername, state->hostname,
>                                 state->ttl, state->remove_af,
>                                 state->addresses, state->dns_addrlist,
> if (state->del_phase) {
>    /* add 'del' addresses */
>    item = sss_iface_addr_get_item(state->ptr_addr_iter);
> } else {
>    /* add 'add' addresses */
>    item = sss_iface_addr_get_item(state->ptr_addr_iter);
> }
> if (item == NULL) {
>    return EIO;
> }
> ret = be_nsupdate_create_ptr_msg(state, state->realm, servername,
>                                 state->hostname, state->ttl,
>                                 state->remove_af, item, state->del_phase,
>                                 &state->update_msg);
> if (ret != EOK) {
>        DEBUG(SSSDBG_OP_FAILURE, "Can't get addresses for DNS update\n");
>
> @@ -454,10 +482,20 @@ sdap_dyndns_update_ptr_done(struct tevent_req *subreq)
>              }
>          }
>    ret = sdap_dyndns_next_ptr_record(state, req);
>    if (ret == EAGAIN) {
>        return;
>    }
>    tevent_req_error(req, ret);
>    return;
>
> }
>
> ret = sdap_dyndns_next_ptr_record(state, req);
>
> if (ret == EAGAIN) {
>    return;
>
> }
>
> tevent_req_done(req);
>
> }
>
> @@ -468,6 +506,37 @@ sdap_dyndns_update_recv(struct tevent_req *req)
>      return EOK;
>  }
>
> +static errno_t
> +sdap_dyndns_next_ptr_record(struct sdap_dyndns_update_state *state,
>                        struct tevent_req *req)
>
> +{

This function is still out of the request..no tevent request function
should be outside _send() and _recv() pair.

[Pavel Reichl]

Hello, please see updated version of the patch.

On 09/16/2015 11:58 AM, Jakub Hrozek wrote:
> On Mon, Sep 14, 2015 at 07:53:06PM +0200, Pavel Reichl wrote:
> > +static uint8_t *nsupdate_convert_address(struct sockaddr_storage *add_address)
> > +{
> > uint8_t *addr;
> > switch(add_address->ss_family) {
> > case AF_INET:
> >    addr = (uint8_t *) &((struct sockaddr_in *) add_address)->sin_addr;
> >    break;
> > case AF_INET6:
> >    addr = (uint8_t *) &((struct sockaddr_in6 *) add_address)->sin6_addr;
> >    break;
> > default:
> >    DEBUG(SSSDBG_CRIT_FAILURE, "Unknown address family\n");
> >    addr = NULL;
> >    break;
> > }
> > return addr;
> >
> > +}
>
> Not directly related to this patch, but this going back and forth between
> sockaddr_storage and uint8_t pointer made me wonder if we should change
> resolv_get_string_ptr_address to accept sockaddr storage instead of
> these hacks?

I filled the https://fedorahosted.org/sssd/ticket/2793
> > +static char *nsupdate_msg_add_ptr(char *update_msg,
> >                               struct sockaddr_storage *address,
> >                               const char *hostname,
> >                               int ttl,
> >                               uint8_t remove_af,
> >                               bool delete)
> > {
> >
> > 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) {
> >    case AF_INET:
> >        if (!(remove_af & DYNDNS_REMOVE_A)) {
> >            continue;
> >        }
> >        addr = (uint8_t *) &((struct sockaddr_in *) old_record->addr)->sin_addr;
> >        break;
> >    case AF_INET6:
> >        if (!(remove_af & DYNDNS_REMOVE_AAAA)) {
> >            continue;
> >        }
> >        addr = (uint8_t *) &((struct sockaddr_in6 *) old_record->addr)->sin6_addr;
> >        break;
> >    default:
> >        DEBUG(SSSDBG_CRIT_FAILURE, "Unknown address family\n");
> >        return NULL;
> >    }
> > if (delete
> >        && !nsupdate_should_remove_addr(address->ss_family, remove_af)) {
>
> This seems a bit strange to me, because if we hit this condition, then
> the whole request is aborted. Shouldn't we just skip this address
> instead?

That was definitely bug, thanks for noticing it. However I didn't see any other way how to fix this unless to move the logic to sdap layer.
> > 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 *address,
> >                       bool del_phase,
>
> I don't think the option should contain 'phase' in its name, the fact
> that we have some phases is a detail from the sdap layer.
> >                          char **_update_msg);

I renamed it to delete, but I'm not sure that's exactly what you had in mind.

[Jakub Hrozek]

On Wed, Sep 16, 2015 at 08:23:16PM +0200, Pavel Reichl wrote:
> Hello, please see updated version of the patch.
>
> On 09/16/2015 11:58 AM, Jakub Hrozek wrote:
> > On Mon, Sep 14, 2015 at 07:53:06PM +0200, Pavel Reichl wrote:
> > > +static uint8_t *nsupdate_convert_address(struct sockaddr_storage *add_address)
> > > +{
> > > uint8_t *addr;
> > > switch(add_address->ss_family) {
> > > case AF_INET:
> > >    addr = (uint8_t *) &((struct sockaddr_in *) add_address)->sin_addr;
> > >    break;
> > > case AF_INET6:
> > >    addr = (uint8_t *) &((struct sockaddr_in6 *) add_address)->sin6_addr;
> > >    break;
> > > default:
> > >    DEBUG(SSSDBG_CRIT_FAILURE, "Unknown address family\n");
> > >    addr = NULL;
> > >    break;
> > > }
> > > return addr;
> > >
> > > +}
> >
> > Not directly related to this patch, but this going back and forth between
> > sockaddr_storage and uint8_t pointer made me wonder if we should change
> > resolv_get_string_ptr_address to accept sockaddr storage instead of
> > these hacks?
>
> I filled the https://fedorahosted.org/sssd/ticket/2793
> > > +static char *nsupdate_msg_add_ptr(char *update_msg,
> > >                               struct sockaddr_storage *address,
> > >                               const char *hostname,
> > >                               int ttl,
> > >                               uint8_t remove_af,
> > >                               bool delete)
> > >
> > > {
> > >
> > > 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) {
> > >    case AF_INET:
> > >        if (!(remove_af & DYNDNS_REMOVE_A)) {
> > >            continue;
> > >        }
> > >        addr = (uint8_t *) &((struct sockaddr_in *) old_record->addr)->sin_addr;
> > >        break;
> > >    case AF_INET6:
> > >        if (!(remove_af & DYNDNS_REMOVE_AAAA)) {
> > >            continue;
> > >        }
> > >        addr = (uint8_t *) &((struct sockaddr_in6 *) old_record->addr)->sin6_addr;
> > >        break;
> > >    default:
> > >        DEBUG(SSSDBG_CRIT_FAILURE, "Unknown address family\n");
> > >        return NULL;
> > >    }
> > > if (delete
> > >        && !nsupdate_should_remove_addr(address->ss_family, remove_af)) {
> >
> > This seems a bit strange to me, because if we hit this condition, then
> > the whole request is aborted. Shouldn't we just skip this address
> > instead?
>
> That was definitely bug, thanks for noticing it. However I didn't see any other way how to fix this unless to move the logic to sdap layer.

Yes, I just didn't want the error to show up..
> > > 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 *address,
> > >                       bool del_phase,
> >
> > I don't think the option should contain 'phase' in its name, the fact
> > that we have some phases is a detail from the sdap layer.
> > >                         char **_update_msg);
>
> I renamed it to delete, but I'm not sure that's exactly what you had in mind.

That's fine.

I would just like to squash these style changes:

diff --git a/src/providers/ldap/sdap_dyndns.c b/src/providers/ldap/sdap_dyndns.c
    index c80a468..673f884 100644
    --- a/src/providers/ldap/sdap_dyndns.c
    +++ b/src/providers/ldap/sdap_dyndns.c
    @@ -395,7 +395,6 @@ sdap_dyndns_update_done(struct tevent_req *subreq)
         /* init iterator for addresses to be deleted */
         state->ptr_addr_iter = sdap_get_address_to_delete(state->dns_addrlist,
                                                           state->remove_af);
    -
         if (state->ptr_addr_iter == NULL) {
             /* init iterator for addresses to be added */
             state->del_phase = false;
    @@ -438,9 +437,9 @@ static struct sss_iface_addr*
     sdap_get_address_to_delete(struct sss_iface_addr *address_it,
                                uint8_t remove_af)
     {
    -    while (address_it != NULL) {
    -        struct sockaddr_storage* address;
    +    struct sockaddr_storage* address;

+    while (address_it != NULL) {
             address = sss_iface_addr_get_address(address_it);

/* skip addresses that are not to be deleted */
    @@ -450,6 +449,7 @@ sdap_get_address_to_delete(struct sss_iface_addr *address_it,

address_it = sss_iface_addr_get_next(address_it);
         }
    +
         return address_it;
     }

Then the code would look OK to me.

[Pavel Reichl]

On 09/18/2015 02:22 PM, Jakub Hrozek wrote:
> I would just like to squash these style changes:
>  diff --git a/src/providers/ldap/sdap_dyndns.c b/src/providers/ldap/sdap_dyndns.c
>  index c80a468..673f884 100644
>  --- a/src/providers/ldap/sdap_dyndns.c
>  +++ b/src/providers/ldap/sdap_dyndns.c
>  @@ -395,7 +395,6 @@ sdap_dyndns_update_done(struct tevent_req *subreq)
>       /* init iterator for addresses to be deleted */
>       state->ptr_addr_iter = sdap_get_address_to_delete(state->dns_addrlist,
>                                                         state->remove_af);
>  -
>       if (state->ptr_addr_iter == NULL) {
>           /* init iterator for addresses to be added */
>           state->del_phase = false;
>  @@ -438,9 +437,9 @@ static struct sss_iface_addr*
>   sdap_get_address_to_delete(struct sss_iface_addr *address_it,
>                              uint8_t remove_af)
>   {
>  -    while (address_it != NULL) {
>  -        struct sockaddr_storage* address;

I don't mind in this particular case, but generally I think that it's a good practice to minimize variable scope as much as possible.
>  +    struct sockaddr_storage* address;
>
>  +    while (address_it != NULL) {
>           address = sss_iface_addr_get_address(address_it);
>
>           /* skip addresses that are not to be deleted */
>  @@ -450,6 +449,7 @@ sdap_get_address_to_delete(struct sss_iface_addr *address_it,
>
>           address_it = sss_iface_addr_get_next(address_it);
>       }
>  +
>       return address_it;
>   }
>
> Then the code would look OK to me.
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Style changes done. Please see attached patch.

[Jakub Hrozek]

On Fri, Sep 18, 2015 at 05:51:47PM +0200, Pavel Reichl wrote:
> On 09/18/2015 02:22 PM, Jakub Hrozek wrote:
> > I would just like to squash these style changes:
> > diff --git a/src/providers/ldap/sdap_dyndns.c b/src/providers/ldap/sdap_dyndns.c
> > index c80a468..673f884 100644
> > --- a/src/providers/ldap/sdap_dyndns.c
> > +++ b/src/providers/ldap/sdap_dyndns.c
> > @@ -395,7 +395,6 @@ sdap_dyndns_update_done(struct tevent_req *subreq)
> >      /* init iterator for addresses to be deleted */
> >      state->ptr_addr_iter = sdap_get_address_to_delete(state->dns_addrlist,
> >                                                        state->remove_af);
> > -
> >      if (state->ptr_addr_iter == NULL) {
> >          /* init iterator for addresses to be added */
> >          state->del_phase = false;
> > @@ -438,9 +437,9 @@ static struct sss_iface_addr*
> >  sdap_get_address_to_delete(struct sss_iface_addr *address_it,
> >                             uint8_t remove_af)
> >  {
> > -    while (address_it != NULL) {
> > -        struct sockaddr_storage* address;
>
> I don't mind in this particular case, but generally I think that it's a good practice to minimize variable scope as much as possible.
> > +    struct sockaddr_storage* address;
> >
> > +    while (address_it != NULL) {
> >          address = sss_iface_addr_get_address(address_it);
> >
> >          /* skip addresses that are not to be deleted */
> > @@ -450,6 +449,7 @@ sdap_get_address_to_delete(struct sss_iface_addr *address_it,
> >
> >          address_it = sss_iface_addr_get_next(address_it);
> >      }
> > +
> >      return address_it;
> >  }
> >
> > Then the code would look OK to me.
> > _______________________________________________
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> Style changes done. Please see attached patch.

ACK code-wise and based on simple testing. Did you have a chance to run
downstream tests? If yes, I'll push the patch..

[Pavel Reichl]

On 09/21/2015 05:46 PM, Jakub Hrozek wrote:
> On Fri, Sep 18, 2015 at 05:51:47PM +0200, Pavel Reichl wrote:
> > On 09/18/2015 02:22 PM, Jakub Hrozek wrote:
> > > I would just like to squash these style changes:
> > >  diff --git a/src/providers/ldap/sdap_dyndns.c b/src/providers/ldap/sdap_dyndns.c
> > >  index c80a468..673f884 100644
> > >  --- a/src/providers/ldap/sdap_dyndns.c
> > >  +++ b/src/providers/ldap/sdap_dyndns.c
> > >  @@ -395,7 +395,6 @@ sdap_dyndns_update_done(struct tevent_req *subreq)
> > >       /* init iterator for addresses to be deleted */
> > >       state->ptr_addr_iter = sdap_get_address_to_delete(state->dns_addrlist,
> > >                                                         state->remove_af);
> > >  -
> > >       if (state->ptr_addr_iter == NULL) {
> > >           /* init iterator for addresses to be added */
> > >           state->del_phase = false;
> > >  @@ -438,9 +437,9 @@ static struct sss_iface_addr*
> > >   sdap_get_address_to_delete(struct sss_iface_addr *address_it,
> > >                              uint8_t remove_af)
> > >   {
> > >  -    while (address_it != NULL) {
> > >  -        struct sockaddr_storage* address;
> >
> > I don't mind in this particular case, but generally I think that it's a good practice to minimize variable scope as much as possible.
> > >  +    struct sockaddr_storage* address;
> > >
> > >  +    while (address_it != NULL) {
> > >           address = sss_iface_addr_get_address(address_it);
> > >
> > >           /* skip addresses that are not to be deleted */
> > >  @@ -450,6 +449,7 @@ sdap_get_address_to_delete(struct sss_iface_addr *address_it,
> > >
> > >           address_it = sss_iface_addr_get_next(address_it);
> > >       }
> > >  +
> > >       return address_it;
> > >   }
> > >
> > > Then the code would look OK to me.
> > > _______________________________________________
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > Style changes done. Please see attached patch.
>
> ACK code-wise and based on simple testing. Did you have a chance to run
> downstream tests? If yes, I'll push the patch..

I've asked Dan to run them, but it seems there are some technical troubles not related to the patches. I'm afraid we have to wait a little longer for results.

[Jakub Hrozek]

On Mon, Sep 21, 2015 at 05:46:07PM +0200, Jakub Hrozek wrote:
> > Style changes done. Please see attached patch.
>
> ACK code-wise and based on simple testing. Did you have a chance to run
> downstream tests? If yes, I'll push the patch..

The downstream test machinery seems to have issues at the moment, so
I've pushed the patch based on my testing as
eeac17ebbe38f16deaa8599231cccfc97aaac85c