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.