SSSD / sssd

Clone

#3786 Few krb5_locator code improvements
Merged 5 years ago by fidencio. Opened 5 years ago by lslebodn.
SSSD/ lslebodn/sssd krb5_locator  into  master

file modified
+48 -45 
@@ -63,9 +63,9 @@ 
 

  #define SSSD_KRB5_LOCATOR_DEBUG "SSSD_KRB5_LOCATOR_DEBUG"
 

  #define SSSD_KRB5_LOCATOR_DISABLE "SSSD_KRB5_LOCATOR_DISABLE"
 

  #define DEBUG_KEY "[sssd_krb5_locator] "
 

- #define PLUGIN_DEBUG(body) do { \
 

+ #define PLUGIN_DEBUG(format, ...) do { \
 

      if (ctx->debug) { \
 

-         plugin_debug_fn body; \
 

+         plugin_debug_fn(format, ##__VA_ARGS__); \
 

      } \
 

  } while(0)
 

  
@@ -82,7 +82,10 @@ 
 

      bool disabled;
 

  };
 

  

- void plugin_debug_fn(const char *format, ...)
 

+ #ifdef HAVE_FUNCTION_ATTRIBUTE_FORMAT
 

+ __attribute__((format(printf, 1, 2)))
 

+ #endif
 

+ static void plugin_debug_fn(const char *format, ...)
 

  {
 

      va_list ap;
 

      char *s = NULL;
 
@@ -236,26 +239,26 @@ 
 

                  port = strtol(port_str, &endptr, 10);
 

                  if (errno != 0) {
 

                      ret = errno;
 

-                     PLUGIN_DEBUG(("strtol failed on [%s]: [%d][%s], "
 

-                                 "assuming default.\n", port_str, ret,
 

-                                                        strerror(ret)));
 

+                     PLUGIN_DEBUG("strtol failed on [%s]: [%d][%s], "
 

+                                  "assuming default.\n",
 

+                                  port_str, ret, strerror(ret));
 

                      port = 0;
 

                  }
 

                  if (*endptr != '\0') {
 

-                     PLUGIN_DEBUG(("Found additional characters [%s] in port "
 

-                                 "number [%s], assuming default.\n", endptr,
 

-                                                                     port_str));
 

+                     PLUGIN_DEBUG("Found additional characters [%s] in port "
 

+                                  "number [%s], assuming default.\n",
 

+                                  endptr, port_str);
 

                      port = 0;
 

                  }
 

  

                  if (port < 0 || port > 65535) {
 

-                     PLUGIN_DEBUG(("Illegal port number [%ld], assuming "
 

-                                   "default.\n", port));
 

+                     PLUGIN_DEBUG("Illegal port number [%ld], assuming "
 

+                                  "default.\n", port);
 

                      port = 0;
 

                  }
 

              } else {
 

-                 PLUGIN_DEBUG(("Illegal port number [%s], assuming default.\n",
 

-                             port_str));
 

+                 PLUGIN_DEBUG("Illegal port number [%s], assuming default.\n",
 

+                              port_str);
 

                  port = 0;
 

              }
 

          }
 
@@ -270,7 +273,7 @@ 
 

              addr_str++;
 

          }
 

  

-         PLUGIN_DEBUG(("Found [%s][%d].\n", addr_str, port));
 

+         PLUGIN_DEBUG("Found [%s][%ld].\n", addr_str, port);
 

  

          l[c].addr = strdup(addr_str);
 

          if (l[c].addr == NULL) {
 
@@ -314,7 +317,7 @@ 
 

              name_tmpl = KPASSWDINFO_TMPL;
 

              break;
 

          default:
 

-             PLUGIN_DEBUG(("Unsupported service [%d].\n", svc));
 

+             PLUGIN_DEBUG("Unsupported service [%d].\n", svc);
 

              return EINVAL;
 

      }
 

  
@@ -323,13 +326,13 @@ 
 

  

      krb5info_name = calloc(1, len + 1);
 

      if (krb5info_name == NULL) {
 

-         PLUGIN_DEBUG(("malloc failed.\n"));
 

+         PLUGIN_DEBUG("calloc failed.\n");
 

          return ENOMEM;
 

      }
 

  

      ret = snprintf(krb5info_name, len, name_tmpl, realm);
 

      if (ret < 0) {
 

-         PLUGIN_DEBUG(("snprintf failed.\n"));
 

+         PLUGIN_DEBUG("snprintf failed.\n");
 

          ret = EINVAL;
 

          goto done;
 

      }
 
@@ -337,8 +340,8 @@ 
 

  

      fd = open(krb5info_name, O_RDONLY);
 

      if (fd == -1) {
 

-         PLUGIN_DEBUG(("open failed [%s][%d][%s].\n",
 

-                       krb5info_name, errno, strerror(errno)));
 

+         PLUGIN_DEBUG("open failed [%s][%d][%s].\n",
 

+                      krb5info_name, errno, strerror(errno));
 

          ret = errno;
 

          goto done;
 

      }
 
@@ -349,15 +352,15 @@ 
 

      len = sss_atomic_read_s(fd, buf, BUFSIZE);
 

      if (len == -1) {
 

          ret = errno;
 

-         PLUGIN_DEBUG(("read failed [%d][%s].\n", ret, strerror(ret)));
 

+         PLUGIN_DEBUG("read failed [%d][%s].\n", ret, strerror(ret));
 

          close(fd);
 

          goto done;
 

      }
 

      close(fd);
 

  

      if (len == BUFSIZE) {
 

-         PLUGIN_DEBUG(("Content of krb5info file [%s] is [%d] or larger.\n",
 

-                       krb5info_name, BUFSIZE));
 

+         PLUGIN_DEBUG("Content of krb5info file [%s] is [%d] or larger.\n",
 

+                      krb5info_name, BUFSIZE);
 

      }
 

  

      switch (svc) {
 
@@ -376,7 +379,7 @@ 
 

              }
 

              break;
 

          default:
 

-             PLUGIN_DEBUG(("Unsupported service [%d].\n", svc));
 

+             PLUGIN_DEBUG("Unsupported service [%d].\n", svc);
 

              ret = EINVAL;
 

              goto done;
 

      }
 
@@ -401,7 +404,7 @@ 
 

          ctx->debug = false;
 

      } else {
 

          ctx->debug = true;
 

-         PLUGIN_DEBUG(("sssd_krb5_locator_init called\n"));
 

+         PLUGIN_DEBUG("sssd_krb5_locator_init called\n");
 

      }
 

  

      dummy = getenv(SSSD_KRB5_LOCATOR_DISABLE);
 
@@ -409,7 +412,7 @@ 
 

          ctx->disabled = false;
 

      } else {
 

          ctx->disabled = true;
 

-         PLUGIN_DEBUG(("SSSD KRB5 locator plugin is disabled.\n"));
 

+         PLUGIN_DEBUG("SSSD KRB5 locator plugin is disabled.\n");
 

      }
 

  

      *private_data = ctx;
 
@@ -424,7 +427,7 @@ 
 

      if (private_data == NULL) return;
 

  

      ctx = (struct sssd_ctx *) private_data;
 

-     PLUGIN_DEBUG(("sssd_krb5_locator_close called\n"));
 

+     PLUGIN_DEBUG("sssd_krb5_locator_close called\n");
 

  

      free_addr_port_list(&(ctx->kdc_addr));
 

      free_addr_port_list(&(ctx->kpasswd_addr));
 
@@ -460,7 +463,7 @@ 
 

      }
 

  

      if (ctx->disabled) {
 

-         PLUGIN_DEBUG(("Plugin disabled, nothing to do.\n"));
 

+         PLUGIN_DEBUG("Plugin disabled, nothing to do.\n");
 

          return KRB5_PLUGIN_NO_HANDLE;
 

      }
 

  
@@ -468,13 +471,13 @@ 
 

          free(ctx->sssd_realm);
 

          ctx->sssd_realm = strdup(realm);
 

          if (ctx->sssd_realm == NULL) {
 

-             PLUGIN_DEBUG(("strdup failed.\n"));
 

+             PLUGIN_DEBUG("strdup failed.\n");
 

              return KRB5_PLUGIN_NO_HANDLE;
 

          }
 

  

          ret = get_krb5info(realm, ctx, locate_service_kdc);
 

          if (ret != EOK) {
 

-             PLUGIN_DEBUG(("get_krb5info failed.\n"));
 

+             PLUGIN_DEBUG("get_krb5info failed.\n");
 

              return KRB5_PLUGIN_NO_HANDLE;
 

          }
 

  
@@ -482,22 +485,22 @@ 
 

              svc == locate_service_master_kdc) {
 

              ret = get_krb5info(realm, ctx, locate_service_kpasswd);
 

              if (ret != EOK) {
 

-                 PLUGIN_DEBUG(("reading kpasswd address failed, "
 

-                               "using kdc address.\n"));
 

+                 PLUGIN_DEBUG("reading kpasswd address failed, "
 

+                              "using kdc address.\n");
 

                  free_addr_port_list(&(ctx->kpasswd_addr));
 

                  ret = copy_addr_port_list(ctx->kdc_addr, true,
 

                                            &(ctx->kpasswd_addr));
 

                  if (ret != EOK) {
 

-                     PLUGIN_DEBUG(("copying address list failed.\n"));
 

+                     PLUGIN_DEBUG("copying address list failed.\n");
 

                      return KRB5_PLUGIN_NO_HANDLE;
 

                  }
 

              }
 

          }
 

      }
 

  

-     PLUGIN_DEBUG(("sssd_realm[%s] requested realm[%s] family[%d] socktype[%d] "
 

-                   "locate_service[%d]\n", ctx->sssd_realm, realm, family,
 

-                                           socktype, svc));
 

+     PLUGIN_DEBUG("sssd_realm[%s] requested realm[%s] family[%d] socktype[%d] "
 

+                  "locate_service[%d]\n",
 

+                  ctx->sssd_realm, realm, family, socktype, svc);
 

  

      switch (svc) {
 

          case locate_service_kdc:
 
@@ -547,7 +550,7 @@ 
 

          memset(port_str, 0, PORT_STR_SIZE);
 

          ret = snprintf(port_str, PORT_STR_SIZE-1, "%u", port);
 

          if (ret < 0 || ret >= (PORT_STR_SIZE-1)) {
 

-             PLUGIN_DEBUG(("snprintf failed.\n"));
 

+             PLUGIN_DEBUG("snprintf failed.\n");
 

              return KRB5_PLUGIN_NO_HANDLE;
 

          }
 

  
@@ -557,31 +560,31 @@ 
 

  

          ret = getaddrinfo(addr[c].addr, port_str, &ai_hints, &ai);
 

          if (ret != 0) {
 

-             PLUGIN_DEBUG(("getaddrinfo failed [%d][%s].\n", ret,
 

-                                                             gai_strerror(ret)));
 

+             PLUGIN_DEBUG("getaddrinfo failed [%d][%s].\n",
 

+                          ret, gai_strerror(ret));
 

              if (ret == EAI_SYSTEM) {
 

-                 PLUGIN_DEBUG(("getaddrinfo failed [%d][%s].\n",
 

-                               errno, strerror(errno)));
 

+                 PLUGIN_DEBUG("getaddrinfo failed [%d][%s].\n",
 

+                              errno, strerror(errno));
 

              }
 

              return KRB5_PLUGIN_NO_HANDLE;
 

          }
 

  

-         PLUGIN_DEBUG(("addr[%s:%s] family[%d] socktype[%d]\n", addr[c].addr,
 

-                      port_str, ai->ai_family, ai->ai_socktype));
 

+         PLUGIN_DEBUG("addr[%s:%s] family[%d] socktype[%d]\n",
 

+                      addr[c].addr, port_str, ai->ai_family, ai->ai_socktype);
 

  

          if ((family == AF_UNSPEC || ai->ai_family == family) &&
 

              ai->ai_socktype == socktype) {
 

  

              ret = cbfunc(cbdata, socktype, ai->ai_addr);
 

              if (ret != 0) {
 

-                 PLUGIN_DEBUG(("cbfunc failed\n"));
 

+                 PLUGIN_DEBUG("cbfunc failed\n");
 

                  freeaddrinfo(ai);
 

                  return ret;
 

              } else {
 

-                 PLUGIN_DEBUG(("[%s] used\n", addr[c].addr));
 

+                 PLUGIN_DEBUG("[%s] used\n", addr[c].addr);
 

              }
 

          } else {
 

-             PLUGIN_DEBUG(("[%s] NOT used\n", addr[c].addr));
 

+             PLUGIN_DEBUG("[%s] NOT used\n", addr[c].addr);
 

          }
 

          freeaddrinfo(ai);
 

      }

no initial comment

The patches looks mostly good to me, I just don't know why was the port variable changed from long to int?

The patches looks mostly good to me, I just don't know why was the port variable changed from long to int?

There are two different format strings used for port in the function buf_to_addr_port_list

250 
251                 if (port < 0 || port > 65535) {
252                     PLUGIN_DEBUG(("Illegal port number [%ld], assuming "
253                                   "default.\n", port));
254                     port = 0;
255                 }

and

271         }
272 
273         PLUGIN_DEBUG(("Found [%s][%d].\n", addr_str, port));
274 
275         l[c].addr = strdup(addr_str);

And It was changed to int because it's enough for range port < 0 || port > 65535. In another words, there was a compile time warning :-)

OK, but since the port is converted from string using strtol, wouldn't it be better to use long as it was and use the long int conversion in the DEBUG macros? IIRC on most platforms int == long, so maybe this is academic, but for me it would also feel more natural to use long for the output of strtol.

rebased onto 3569f360c6ec101d50720704d6214d196e60611a
5 years ago

OK, but since the port is converted from string using strtol, wouldn't it be better to use long as it was and use the long int conversion in the DEBUG macros? IIRC on most platforms int == long, so maybe this is academic, but for me it would also feel more natural to use long for the output of strtol.

That's valid argument. I fixed that.

Unfortunately the patches don't apply anymore, can you rebase?

rebased onto 2ca9cb6
5 years ago

Unfortunately the patches don't apply anymore, can you rebase?

Done

ACK

Commit 9680ac9 fixes this pull-request

Pull-Request has been merged by fidencio
5 years ago

Commit aefdf70 fixes this pull-request

Pull-Request has been merged by fidencio
5 years ago

Commit 09dc1d9 fixes this pull-request

Pull-Request has been merged by fidencio
5 years ago

Commit 276f2e3 fixes this pull-request

Pull-Request has been merged by fidencio
5 years ago

Commit 86de91f fixes this pull-request

Pull-Request has been merged by fidencio
5 years ago
Metadata
None
No Tags
Changes Summary 1
+48 -45
file changed
src/krb5_plugin/sssd_krb5_locator_plugin.c
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.