On Wed, 2012-05-30 at 08:23 -0400, Stephen Gallagher wrote: > This patch was submitted by Shantanu Goel in a Trac ticket. Sending to > the list for review. I haven't yet had a chance to dig into it myself > yet, but the concept as described in > https://fedorahosted.org/sssd/ticket/1354 is sound, so I'd like to see > this get cleaned up and included.
Comments inline, but I can already say I want more comments in the code, there are none. > > > > > > > differences > between files > attachment > (sssd-1.5.1-expire-idle-connection.diff) > > --- .gen-patch.27968/src/confdb/confdb.h 2012-04-20 > 14:01:34.477465315 -0400 > +++ sssd-1.5.1/src/confdb/confdb.h 2012-04-20 14:01:45.069779773 > -0400 > @@ -78,6 +78,7 @@ > #define CONFDB_NSS_VETOED_SHELL "vetoed_shells" > #define CONFDB_NSS_ALLOWED_SHELL "allowed_shells" > #define CONFDB_NSS_SHELL_FALLBACK "shell_fallback" > +#define CONFDB_NSS_IDLE_TIMEOUT "idle_timeout" > > /* PAM */ > #define CONFDB_PAM_CONF_ENTRY "config/pam" > --- .gen-patch.27968/src/responder/common/responder_common.c 2012-04-20 > 14:01:34.478465251 -0400 > +++ sssd-1.5.1/src/responder/common/responder_common.c 2012-04-20 > 14:01:45.070779708 -0400 > @@ -88,9 +88,82 @@ > return EOK; > } > > +static void idle_handler(struct tevent_context *, > + struct tevent_timer *, > + struct timeval, > + void *); > + > +static void start_timer(struct resp_ctx *rctx, struct timeval *tvp) > +{ > + struct timeval tv; > + > + if (rctx->idle_timeout <= 0) > + return; > + if (rctx->timer) > + return; > + if (!rctx->cctx) > + return; Style guidelines violations ^^ We use: if (x) { blah } > + if (!tvp) { > + tvp = &tv; > + gettimeofday(tvp, NULL); > + } Please use tevent_timeval_current() here ^ > + tvp->tv_sec += rctx->idle_timeout; > + rctx->timer = tevent_add_timer(rctx->cctx->ev, rctx, > + *tvp, idle_handler, rctx); > + if (!rctx->timer) > + DEBUG(0, ("could not start idle timer\n")); > +} > + > +static void idle_handler(struct tevent_context *ev, > + struct tevent_timer *te, > + struct timeval current_time, > + void *data) > +{ > + struct resp_ctx *rctx = data; > + struct cli_ctx *cctx = rctx->cctx; > + int n = 0; > + > + while (cctx) { > + struct cli_ctx *cctx_next = cctx->next; > + > + if (cctx->active || cctx->creq) > + cctx->active = 0; > + else if (talloc_free(cctx) == 0) > + n++; sever style violation ^^ as well, plus I want a comment on top that explain why we are doing an either or. Also what's the point of this check on talloc_free() ? It is not like we have any options left if it fails. Also please use a for loop and a continue after the if (), will be much more readable. > + cctx = cctx_next; > + } > + if (n) > + DEBUG(2, ("terminated %d idle connecions\n", n)); braces ^^ > + /* > + * The timer event is freed by the library. > + */ > + rctx->timer = NULL; > + start_timer(rctx, ¤t_time); > +} > + > +static void client_add(struct cli_ctx *cctx) > +{ > + struct resp_ctx *rctx = cctx->rctx; > + > + cctx->next = rctx->cctx; > + cctx->prev = &rctx->cctx; > + rctx->cctx = cctx; > + if (cctx->next) > + cctx->next->prev = &cctx->next; use DLIST_* macros here ^^ and elswhere, I do not want to see custom linked list manipulation unless unavoidable (and in that case I want to see a comment that explains why it is unavoidable. > + start_timer(rctx, NULL); > +} > + > static int client_destructor(struct cli_ctx *ctx) > { > if (ctx->cfd > 0) close(ctx->cfd); style violation, pluse return of close is not checked, check and fire a debug message in case of error. > + if (ctx->prev) { > + *ctx->prev = ctx->next; > + if (ctx->next) > + ctx->next->prev = ctx->prev; > + ctx->next = NULL; > + ctx->prev = NULL; > + } use DLIST_* > return 0; > } > > @@ -132,6 +205,8 @@ > { > int ret; > > + cctx->active++; > + > ret = sss_packet_send(cctx->creq->out, cctx->cfd); > if (ret == EAGAIN) { > /* not all data was sent, loop again */ > @@ -155,6 +230,8 @@ > { > int ret; > > + cctx->active++; > + Why these active++ up here, it feels wrong. I haven't checked this throghly but I would think either an accessor function should be used or even just fold this into client_add()/start_timer() > if (!cctx->creq) { > cctx->creq = talloc_zero(cctx, struct cli_request); > if (!cctx->creq) { > @@ -298,6 +375,8 @@ > cctx->ev = ev; > cctx->rctx = rctx; > > + client_add(cctx); > + > talloc_set_destructor(cctx, client_destructor); > > DEBUG(4, ("Client connected to privileged pipe!\n")); > @@ -356,6 +435,8 @@ > cctx->ev = ev; > cctx->rctx = rctx; > > + client_add(cctx); > + > talloc_set_destructor(cctx, client_destructor); > > DEBUG(4, ("Client connected!\n")); > @@ -585,24 +666,27 @@ > return EIO; > } > > -int sss_process_init(TALLOC_CTX *mem_ctx, > - struct tevent_context *ev, > - struct confdb_ctx *cdb, > - struct sss_cmd_table sss_cmds[], > - const char *sss_pipe_name, > - const char *sss_priv_pipe_name, > - const char *confdb_service_path, > - const char *svc_name, > - uint16_t svc_version, > - struct sbus_interface *monitor_intf, > - const char *cli_name, > - struct sbus_interface *dp_intf, > - struct resp_ctx **responder_ctx) > +int sss_process_init_timeout(TALLOC_CTX *mem_ctx, > + struct tevent_context *ev, > + struct confdb_ctx *cdb, > + struct sss_cmd_table sss_cmds[], > + const char *sss_pipe_name, > + const char *sss_priv_pipe_name, > + const char *confdb_service_path, > + const char *svc_name, > + uint16_t svc_version, > + struct sbus_interface *monitor_intf, > + const char *cli_name, > + struct sbus_interface *dp_intf, > + struct resp_ctx **responder_ctx, > + time_t idle_timeout) > { > struct resp_ctx *rctx; > struct sss_domain_info *dom; > int ret; > > + DEBUG(2, ("connection idle timeout %d\n", idle_timeout)); > + > rctx = talloc_zero(mem_ctx, struct resp_ctx); > if (!rctx) { > DEBUG(0, ("fatal error initializing resp_ctx\n")); > @@ -614,6 +698,7 @@ > rctx->sock_name = sss_pipe_name; > rctx->priv_sock_name = sss_priv_pipe_name; > rctx->confdb_service_path = confdb_service_path; > + rctx->idle_timeout = idle_timeout; > > ret = confdb_get_domains(rctx->cdb, &rctx->domains); > if (ret != EOK) { > @@ -666,6 +751,27 @@ > return EOK; > } > > +int sss_process_init(TALLOC_CTX *mem_ctx, > + struct tevent_context *ev, > + struct confdb_ctx *cdb, > + struct sss_cmd_table sss_cmds[], > + const char *sss_pipe_name, > + const char *sss_priv_pipe_name, > + const char *confdb_service_path, > + const char *svc_name, > + uint16_t svc_version, > + struct sbus_interface *monitor_intf, > + const char *cli_name, > + struct sbus_interface *dp_intf, > + struct resp_ctx **responder_ctx) > +{ > + return sss_process_init_timeout(mem_ctx, ev, cdb, sss_cmds, > + sss_pipe_name, sss_priv_pipe_name, > + confdb_service_path, svc_name, > + svc_version, monitor_intf, > cli_name, > + dp_intf, responder_ctx, 0); > +} Why not simply add the timeval and keep the original name ? In what case do we want to idle out only for nss stuff ? I think we always want to idle out for any client connection, whtehr it is for nss, pam, ssh, sudo, etc... > int sss_dp_get_domain_conn(struct resp_ctx *rctx, const char *domain, > struct be_conn **_conn) > { > --- .gen-patch.27968/src/responder/common/responder.h 2012-04-20 > 14:01:34.478465251 -0400 > +++ sssd-1.5.1/src/responder/common/responder.h 2012-04-20 > 14:01:45.070779708 -0400 > @@ -90,6 +90,11 @@ > struct sss_names_ctx *names; > > void *pvt_ctx; > + > + struct cli_ctx *cctx; > + > + time_t idle_timeout; > + struct tevent_timer *timer; > }; > > /* Needed for the NSS responder */ > @@ -120,6 +125,11 @@ > int netgrent_cur; > > time_t pam_timeout; > + > + int active; > + > + struct cli_ctx *next; > + struct cli_ctx **prev; > }; > > struct sss_cmd_table { > @@ -142,6 +152,21 @@ > struct sbus_interface *dp_intf, > struct resp_ctx **responder_ctx); > > +int sss_process_init_timeout(TALLOC_CTX *mem_ctx, > + struct tevent_context *ev, > + struct confdb_ctx *cdb, > + struct sss_cmd_table sss_cmds[], > + const char *sss_pipe_name, > + const char *sss_priv_pipe_name, > + const char *confdb_service_path, > + const char *svc_name, > + uint16_t svc_version, > + struct sbus_interface *monitor_intf, > + const char *cli_name, > + struct sbus_interface *dp_intf, > + struct resp_ctx **responder_ctx, > + time_t idle_timeout); > + > int sss_parse_name(TALLOC_CTX *memctx, > struct sss_names_ctx *snctx, > const char *orig, char **domain, char **name); > --- .gen-patch.27968/src/responder/nss/nsssrv.c 2012-04-20 > 14:01:34.478465251 -0400 > +++ sssd-1.5.1/src/responder/nss/nsssrv.c 2012-04-20 > 14:01:45.070779708 -0400 > @@ -258,6 +258,7 @@ > struct nss_ctx *nctx; > int ret, max_retries; > int hret; > + int idle_timeout; > > nctx = talloc_zero(mem_ctx, struct nss_ctx); > if (!nctx) { > @@ -273,15 +274,22 @@ > > nss_cmds = get_nss_cmds(); > > - ret = sss_process_init(nctx, ev, cdb, > - nss_cmds, > - SSS_NSS_SOCKET_NAME, NULL, > - CONFDB_NSS_CONF_ENTRY, > - NSS_SBUS_SERVICE_NAME, > - NSS_SBUS_SERVICE_VERSION, > - &monitor_nss_interface, > - "NSS", &nss_dp_interface, > - &nctx->rctx); > + ret = confdb_get_int(cdb, nctx, CONFDB_NSS_CONF_ENTRY, > + CONFDB_NSS_IDLE_TIMEOUT, 0, &idle_timeout); > + if (ret != EOK) { > + DEBUG(0, ("fatal error getting nss config\n")); use new debug codes ^^ (also above and below elsewhere). > + return ret; > + } > + > + ret = sss_process_init_timeout(nctx, ev, cdb, > + nss_cmds, > + SSS_NSS_SOCKET_NAME, NULL, > + CONFDB_NSS_CONF_ENTRY, > + NSS_SBUS_SERVICE_NAME, > + NSS_SBUS_SERVICE_VERSION, > + &monitor_nss_interface, > + "NSS", &nss_dp_interface, > + &nctx->rctx, idle_timeout); > if (ret != EOK) { > return ret; > } > --- .gen-patch.27968/src/sss_client/common.c 2012-04-20 > 14:01:34.479465187 -0400 > +++ sssd-1.5.1/src/sss_client/common.c 2012-04-20 14:01:51.388370756 > -0400 > @@ -38,6 +38,7 @@ > #include <stdbool.h> > #include <stdint.h> > #include <string.h> > +#include <signal.h> > #include <fcntl.h> > #include <poll.h> > > @@ -154,8 +155,8 @@ > } > > /* Write failed */ > - sss_cli_close_socket(); > *errnop = errno; > + sss_cli_close_socket(); > return NSS_STATUS_UNAVAIL; > } > > @@ -264,8 +265,8 @@ > * since the transaction has failed half way > * through. */ > > - sss_cli_close_socket(); > *errnop = errno; > + sss_cli_close_socket(); > ret = NSS_STATUS_UNAVAIL; > goto failed; > } The above 2 sound like some sort of bugfix, so they shouldn't be merged in the same patch, they need a separate patch with a good explanation in the commit message. > @@ -363,12 +364,11 @@ > * 0-3: 32bit unsigned version number > */ > > -static int sss_nss_check_version(const char *socket_name) > +static int sss_nss_check_version(const char *socket_name, int > *errnop) > { > uint8_t *repbuf; > size_t replen; > enum nss_status nret; > - int errnop; > int res = NSS_STATUS_UNAVAIL; > uint32_t expected_version; > struct sss_cli_req_data req; > @@ -379,6 +379,7 @@ > strcmp(socket_name, SSS_PAM_PRIV_SOCKET_NAME) == 0) { > expected_version = SSS_PAM_PROTOCOL_VERSION; > } else { > + *errnop = EFAULT; > return NSS_STATUS_UNAVAIL; > } > > @@ -386,20 +387,20 @@ > req.data = &expected_version; > > nret = sss_nss_make_request_nochecks(SSS_GET_VERSION, &req, > - &repbuf, &replen, &errnop); > + &repbuf, &replen, errnop); > if (nret != NSS_STATUS_SUCCESS) { > return nret; > } > > - if (!repbuf) { > - return res; > - } > - > - if (((uint32_t *)repbuf)[0] == expected_version) { > + if (repbuf && ((uint32_t *)repbuf)[0] == expected_version) { > + *errnop = 0; > res = NSS_STATUS_SUCCESS; > - } > + } else > + *errnop = EFAULT; style > + if (repbuf) > + free(repbuf); useless change, do not do it! free() does not need guards against NULL > - free(repbuf); > return res; > } > > @@ -670,12 +671,11 @@ > > sss_cli_sd = mysd; > > +- if (sss_nss_check_version(socket_name) == NSS_STATUS_SUCCESS) { > + if (sss_nss_check_version(socket_name, errnop) == > NSS_STATUS_SUCCESS) { Line wrapping, plus if you are changin this line then split it in ret = foo() if (ret == bar) { > return SSS_STATUS_SUCCESS; > } > > sss_cli_close_socket(); > - *errnop = EFAULT; > return SSS_STATUS_UNAVAIL; > } > > @@ -686,8 +686,10 @@ > uint8_t **repbuf, size_t *replen, > int *errnop) > { > - enum sss_status ret; > char *envval; > + int retry, restore; > + struct sigaction sa, osa; > + enum nss_status ret = NSS_STATUS_UNAVAIL; > > /* avoid looping in the nss daemon */ > envval = getenv("_SSS_LOOPS"); > @@ -695,12 +697,32 @@ > return NSS_STATUS_NOTFOUND; > } > > - ret = sss_cli_check_socket(errnop, SSS_NSS_SOCKET_NAME); > - if (ret != SSS_STATUS_SUCCESS) { > - return NSS_STATUS_UNAVAIL; > + /* > + * Block off SIGPIPE and retry the command > + * in case the other end closes the pipe on us. > + */ > + sigemptyset(&sa.sa_mask); > + sa.sa_flags = 0; > + sa.sa_handler = SIG_IGN; > + if (sigaction(SIGPIPE, &sa, &osa) == 0) { > + restore = 1; > + retry = 2; > + } else { > + restore = 0; > + retry = 1; > } NACK NACK NACK We do not mess with application signals in a nsswitch library absolutely unacceptable. > + do { > + if (sss_cli_check_socket(errnop, > + SSS_NSS_SOCKET_NAME) == > SSS_STATUS_SUCCESS) > + ret = sss_nss_make_request_nochecks(cmd, rd, repbuf, > + replen, errnop); > + } while (ret == NSS_STATUS_UNAVAIL > + && (*errnop == EPIPE || *errnop == 0) > + && --retry > 0); > + if (restore) > + sigaction(SIGPIPE, &osa, NULL); > > - return sss_nss_make_request_nochecks(cmd, rd, repbuf, replen, > errnop); > + return ret; > } > > errno_t check_server_cred(int sockfd) > > It's a NACK. Simo. -- Simo Sorce * Red Hat, Inc * New York _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel