On 05/30/2012 09:08 AM, Simo Sorce wrote:
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.
Seems like a big effort. Thank you for the patch! We would gladly accept it when the style, comment and functional concerns are addressed. Please do not be offended, it is just the process we rigorously follow. We would absolutely delighted to get more contributions in future!
Thanks Dmitri
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,
if (ret != EOK) { return ret; }&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);
if (nret != NSS_STATUS_SUCCESS) { return nret; }&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.