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/