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.