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);--- .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 (!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.