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