On 05/04/2010 06:44 AM, Sumit Bose wrote:
> Hi,
>
> my comments are in line.
>
> bye,
> Sumit
>
>> @@ -727,6 +728,7 @@ libsss_ldap_la_SOURCES = \
>> providers/ldap/sdap_child_helpers.c \
>> providers/ldap/sdap_fd_events.c \
>> providers/ldap/sdap.c \
>> + providers/ipa/ipa_dyndns.c \
>
> Why does the LDAP provider need ipa_dyndns.c ?
That's a mistake. That should have been removed.
>
>> util/user_info_msg.c \
>> util/sss_ldap.c \
>> util/sss_krb5.c
>> @@ -788,6 +790,7 @@ libsss_ipa_la_SOURCES = \
>> providers/ipa/ipa_auth.c \
>> providers/ipa/ipa_access.c \
>> providers/ipa/ipa_timerules.c \
>> + providers/ipa/ipa_dyndns.c \
>> providers/ldap/ldap_id.c \
>> providers/ldap/ldap_id_enum.c \
>> providers/ldap/ldap_id_cleanup.c \
>
>
>> diff --git a/src/providers/ipa/ipa_dyndns.c b/src/providers/ipa/ipa_dyndns.c
>> new file mode 100644
>> index
>> 0000000000000000000000000000000000000000..c9f64bed50647e8a25dab109491de63abd9035b9
>> --- /dev/null
>> +++ b/src/providers/ipa/ipa_dyndns.c
>> +
>> +struct ipa_ipv4 {
>> + struct ipa_ipv4 *next;
>> + struct ipa_ipv4 *prev;
>> +
>> + struct sockaddr_in *addr;
>> + bool matched;
>> +};
>> +
>> +struct ipa_ipv6 {
>> + struct ipa_ipv6 *next;
>> + struct ipa_ipv6 *prev;
>> +
>> + struct sockaddr_in6 *addr;
>> + bool matched;
>> +};
>
> I would recommend to only have one list with a family (AF_INET,
> AF_INET6, ...) flag and a struct sockaddr_storage to hold the address.
>
Yeah, the separate lists were because originally I was only going to
base updates on the lookup_family option, but I changed that. I'll
combine them.
>> +
>> +struct ipa_dyndns_ctx {
>> + struct ipa_options *ipa_ctx;
>> + char *hostname;
>> + struct ipa_ipv4 *ipv4;
>> + struct ipa_ipv6 *ipv6;
>> + int child_status;
>> +};
>
>
>
>> +
>> + if (iface) {
>> + /* Get the IP addresses associated with the
>> + * specified interface
>> + */
>> + errno = 0;
>> + ret = getifaddrs(&ifaces);
>> + if (ret == -1) {
>> + ret = errno;
>> + DEBUG(0, ("Could not read interfaces [%d][%s]\n",
>> + ret, strerror(ret)));
>> + goto failed;
>> + }
>> +
>> + for(ifa = ifaces; ifa != NULL; ifa=ifa->ifa_next) {
>> + /* Some interfaces don't have an ifa_addr */
>> + if (!ifa->ifa_addr) continue;
>> +
>
> Although it is safe here I think using a
> switch(ifa->ifa_addr->sa_family) would make the code more clear, e.g
>
> if (strcasecmp(ifa->ifa_name, iface) == 0)
> switch(ifa->ifa_addr->sa_family == AF_INET)
> ....
>
I'll clean that up.
>> + /* Add IPv4 addresses to the list */
>> + if(ifa->ifa_addr->sa_family == AF_INET&&
>> + strcasecmp(ifa->ifa_name, iface) == 0) {
>> + /* Add this address to the IPv4 list */
>> + address4 = talloc_zero(state, struct ipa_ipv4);
>> + if (!address4) {
>> + goto failed;
>> + }
>> +
>> + address4->addr = talloc_memdup(address4, ifa->ifa_addr,
>> + sizeof(struct sockaddr_in));
>> + if(address4->addr == NULL) {
>> + goto failed;
>> + }
>> + DLIST_ADD(state->ipv4, address4);
>> + }
>> +
>> + /* Add IPv6 addresses to the list */
>> + if(ifa->ifa_addr->sa_family == AF_INET6&&
>> + strcasecmp(ifa->ifa_name, iface) == 0) {
>> + /* Add this address to the IPv6 list */
>> + address6 = talloc_zero(state, struct ipa_ipv6);
>> + if (!address6) {
>> + goto failed;
>> + }
>> +
>> + address6->addr = talloc_memdup(address6, ifa->ifa_addr,
>> + sizeof(struct sockaddr_in6));
>> + if(!address6->addr) {
>> + goto failed;
>> + }
>> + DLIST_ADD(state->ipv6, address6);
>> + }
>> + }
>> +
>> + freeifaddrs(ifaces);
>> + }
>> +
>> + else {
>> + /* Get the file descriptor for the primary LDAP connection */
>> + ret = get_fd_from_ldap(ctx->id_ctx->gsh->ldap,&fd);
>> + if (ret != EOK) {
>> + goto failed;
>> + }
>> +
>> + ret = getsockname(fd,&sa,&sa_len);
>> + if (ret == -1) {
>> + DEBUG(0,("Failed to get socket name\n"));
>> + goto failed;
>> + }
>> +
>
> switch(sa.sa_family) ?
Agreed.
>
>> + if (sa.sa_family == AF_INET) {
>> + address4 = talloc(state, struct ipa_ipv4);
>> + if (!address4) {
>> + goto failed;
>> + }
>> + address4->addr = talloc_memdup(address4,&sa,
>> + sizeof(struct sockaddr_in));
>> + if(address4->addr == NULL) {
>> + goto failed;
>> + }
>> + DLIST_ADD(state->ipv4, address4);
>> + }
>> +
>> + if (sa.sa_family == AF_INET6) {
>> + address6 = talloc(state, struct ipa_ipv6);
>> + if (!address6) {
>> + goto failed;
>> + }
>> + address6->addr = talloc_memdup(address6,&sa,
>> + sizeof(struct sockaddr_in6));
>> + if(address6->addr == NULL) {
>> + goto failed;
>> + }
>> + DLIST_ADD(state->ipv6, address6);
>> + }
>> + }
>> +
>> +
>
>
>
>> +static int create_nsupdate_message(struct ipa_nsupdate_ctx *ctx)
>> +{
>> + int ret, i;
>> + char *servername;
>> + char *zone;
>> + char ip_addr[128];
>> + const char *ip;
>> + struct ipa_ipv4 *a_record;
>> + struct ipa_ipv6 *aaaa_record;
>> +
>> + servername = dp_opt_get_string(ctx->dyndns_ctx->ipa_ctx->basic,
>> + IPA_SERVER);
>> + if (!servername) {
>> + return EIO;
>> + }
>> +
>> + zone = dp_opt_get_string(ctx->dyndns_ctx->ipa_ctx->basic,
>> + IPA_DOMAIN);
>> + if (!zone) {
>> + return EIO;
>> + }
>> +
>> + /* The DNS zone for IPA is the lower-case
>> + * version of hte IPA domain
>> + */
>> + for(i = 0; zone[i] != '\0'; i++) {
>> + zone[i] = tolower(zone[i]);
>> + }
>> +
>> + /* Add the server and zone headers */
>> + ctx->update_msg = talloc_asprintf(ctx, "server %s\nzone %s.\n",
>> + servername,
>> + zone);
>> + if (ctx->update_msg == NULL) {
>> + ret = ENOMEM;
>> + goto done;
>> + }
>
> I think you never check if both lists are empty. Does it make sense to
> send only the delete messages in this case?
>
Yes. If both lists are empty then we should remove the entries from DNS
so that they aren't pointing at potentially the wrong address (which may
or may not be a different host)
>> +
>> + /* Remove any existing entries */
>> + ctx->update_msg = talloc_asprintf_append(ctx->update_msg,
>> + "update delete %s. in
>> A\nsend\n"
>> + "update delete %s. in
>> AAAA\nsend\n",
>> + ctx->dyndns_ctx->hostname,
>> + ctx->dyndns_ctx->hostname);
>> + if (ctx->update_msg == NULL) {
>> + ret = ENOMEM;
>> + goto done;
>> + }
>> +
>> + /* Determine if we're updating IPv4, IPv6 or both */
>> + if (ctx->dyndns_ctx->ipv4) {
>> + /* At least one A record */
>> + DLIST_FOR_EACH(a_record, ctx->dyndns_ctx->ipv4) {
>> + ip = inet_ntop(a_record->addr->sin_family,
>> +&a_record->addr->sin_addr,
>> + ip_addr, 128);
>> + if (ip == NULL) {
>> + ret = EIO;
>> + goto done;
>> + }
>> +
>> + /* Format the A record update */
>> + ctx->update_msg = talloc_asprintf_append(
>> + ctx->update_msg,
>> + "update add %s. 86400 in A %s\n",
>> + ctx->dyndns_ctx->hostname, ip_addr);
>> + if (ctx->update_msg == NULL) {
>> + ret = ENOMEM;
>> + goto done;
>> + }
>> + }
>> +
>> + ctx->update_msg = talloc_asprintf_append(ctx->update_msg, "send\n");
>> + if (ctx->update_msg == NULL) {
>> + ret = ENOMEM;
>> + goto done;
>> + }
>> + }
>> +
>> + if (ctx->dyndns_ctx->ipv6) {
>> + /* At least one AAAA record */
>> + DLIST_FOR_EACH(aaaa_record, ctx->dyndns_ctx->ipv6) {
>> + ip = inet_ntop(aaaa_record->addr->sin6_family,
>> +&aaaa_record->addr->sin6_addr,
>> + ip_addr, 128);
>> + if (ip == NULL) {
>> + ret = EIO;
>> + goto done;
>> + }
>> +
>> + /* Format the AAAA message */
>> + ctx->update_msg = talloc_asprintf_append(
>> + ctx->update_msg,
>> + "update add %s. 86400 in AAAA %s\n",
>> + ctx->dyndns_ctx->hostname, ip_addr);
>> + if(ctx->update_msg == NULL) {
>> + ret = ENOMEM;
>> + goto done;
>> + }
>> + }
>> + ctx->update_msg = talloc_asprintf_append(ctx->update_msg, "send\n");
>> + if(ctx->update_msg == NULL) {
>> + ret = ENOMEM;
>> + goto done;
>> + }
>> + }
>> +
>> + ret = EOK;
>> +
>> +done:
>> + return ret;
>> +}
>> +
>> +
>> +
>> + ret = pipe(pipefd_to_child);
>> + if (ret == -1) {
>> + err = errno;
>> + DEBUG(1, ("pipe failed [%d][%s].\n", errno, strerror(errno)));
>
> use err instead of errno as you have done below
Whoops.
>
>> + return NULL;
>> + }
>> +
>> + pid = fork();
>> +
>> + if (pid == 0) { /* child */
>> + args[0] = talloc_strdup(ctx, NSUPDATE_PATH);
>> + args[1] = talloc_strdup(ctx, "-g");
>> + args[2] = NULL;
>> + if (args[0] == NULL || args[1] == NULL) {
>> + return NULL;
>> + }
>> +
>> + close(pipefd_to_child[1]);
>> + ret = dup2(pipefd_to_child[0], STDIN_FILENO);
>> + if (ret == -1) {
>> + err = errno;
>> + DEBUG(1, ("dup2 failed [%d][%s].\n", err, strerror(err)));
>> + return NULL;
>> + }
>> +
>> + errno = 0;
>> + ret = execv(NSUPDATE_PATH, args);
>> + if(ret == -1) {
>> + err = errno;
>> + DEBUG(1, ("execv failed [%d][%s].\n", err, strerror(err)));
>> + }
>> + return NULL;
>> + }
>> +
>> + else if (pid> 0) { /* parent */
>> + close(pipefd_to_child[0]);
>> +
>> + ctx->pipefd_to_child = pipefd_to_child[1];
>> +
>> + /* Write the update message to the nsupdate child */
>> + subreq = write_pipe_send(req,
>> + ctx->dyndns_ctx->ipa_ctx->id_ctx->be->ev,
>> + (uint8_t *)ctx->update_msg,
>> + strlen(ctx->update_msg)+1,
>> + ctx->pipefd_to_child);
>> + if (subreq == NULL) {
>> + return NULL;
>> + }
>> + tevent_req_set_callback(subreq, ipa_dyndns_stdin_done, req);
>> +
>> + /* Set up SIGCHLD handler */
>> + ret = child_handler_setup(req,
>> + ctx->dyndns_ctx->ipa_ctx->id_ctx->be->ev,
>> + pid, ipa_dyndns_child_handler,
>> req,&ctx->sige);
>> + if (ret != EOK) {
>> + return NULL;
>> + }
>> + talloc_steal(ctx, ctx->sige);
>> +
>> + /* Set up timeout handler */
>> + tv = tevent_timeval_current_ofs(15,0);
>
> it would be nice to have a #define IPA_DNYDNS_TIMEOUT 15
Yeah, I meant to do that, then promptly forgot :)
>
>> + ctx->timeout_handler = tevent_add_timer(
>> + ctx->dyndns_ctx->ipa_ctx->id_ctx->be->ev,
>> + req, tv, ipa_dyndns_timeout, req);
>> + if(ctx->timeout_handler == NULL) {
>> + return NULL;
>> + }
>> + }
>> +
>> + else { /* error */
>> + err = errno;
>> + DEBUG(1, ("fork failed [%d][%s].\n", errno, strerror(errno)));
>
> use err, see above
Thanks.
>
>> + return NULL;
>> + }
>> +
>> + return req;
>> +}
>> +
>
Thanks for the review. Updated patch coming soon.
--
Stephen Gallagher
RHCE 804006346421761
Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
