Here's a patch removing seq file. Also there's no need in lib/lock.c anymore. diff -ur ./BUGS /tmp/freeradius-client-1.1.6/BUGS --- ./BUGS 2007-06-21 21:35:00.000000000 +0300 +++ /tmp/freeradius-client-1.1.6/BUGS 2010-02-01 16:54:25.203211724 +0200 @@ -3,12 +3,6 @@ Testing can show the presense of bugs, but not their absence. -- Dijkstra -The get_seqnbr function in build_req.c sometimes returns the same -number, if invoked from multiple processes at the same time. This -occurs only if a lot of process try to read the sequence file, -so I suspect a race condition in the locking code, but I can't -find one. Any help is appreciated. - Radlogin respectively send_server in sendserver.c does not honour a ACCESS_CHALLENGE packet from the RADIUS server. diff -ur ./include/freeradius-client.h /tmp/freeradius-client-1.1.6/include/freeradius-client.h --- ./include/freeradius-client.h 2008-02-11 08:54:23.000000000 +0200 +++ /tmp/freeradius-client-1.1.6/include/freeradius-client.h 2010-02-01 16:47:17.084878041 +0200 @@ -426,7 +426,7 @@ /* buildreq.c */ void rc_buildreq(rc_handle *, SEND_DATA *, int, char *, unsigned short, char *, int, int); -unsigned char rc_get_seqnbr(rc_handle *); +unsigned char rc_get_id(); int rc_auth(rc_handle *, uint32_t, VALUE_PAIR *, VALUE_PAIR **, char *); int rc_auth_proxy(rc_handle *, VALUE_PAIR *, VALUE_PAIR **, char *); int rc_acct(rc_handle *, uint32_t, VALUE_PAIR *); diff -ur ./lib/buildreq.c /tmp/freeradius-client-1.1.6/lib/buildreq.c --- ./lib/buildreq.c 2008-03-05 18:35:20.000000000 +0200 +++ /tmp/freeradius-client-1.1.6/lib/buildreq.c 2010-02-01 17:29:46.987179691 +0200 @@ -13,7 +13,7 @@ #include <includes.h> #include <freeradius-client.h> -unsigned char rc_get_seqnbr(rc_handle *); +unsigned char rc_get_id(); /* * Function: rc_buildreq @@ -29,87 +29,26 @@ data->server = server; data->secret = secret; data->svc_port = port; - data->seq_nbr = rc_get_seqnbr(rh); + data->seq_nbr = rc_get_id(); data->timeout = timeout; data->retries = retries; data->code = code; } /* - * Function: rc_guess_seqnbr + * Function: rc_get_id * - * Purpose: return a random sequence number + * Purpose: generate random id * */ -static unsigned char rc_guess_seqnbr(void) +unsigned char rc_get_id() { srandom((unsigned int)(time(NULL)+getpid())); return (unsigned char)(random() & UCHAR_MAX); } /* - * Function: rc_get_seqnbr - * - * Purpose: generate a sequence number - * - */ - -unsigned char rc_get_seqnbr(rc_handle *rh) -{ - FILE *sf; - int tries = 1; - int seq_nbr; - char *seqfile = rc_conf_str(rh, "seqfile"); - - if ((sf = fopen(seqfile, "a+")) == NULL) - { - rc_log(LOG_ERR,"rc_get_seqnbr: couldn't open sequence file %s: %s", seqfile, strerror(errno)); - /* well, so guess a sequence number */ - return rc_guess_seqnbr(); - } - - while (do_lock_exclusive(sf)!= 0) - { - if (errno != EWOULDBLOCK) { - rc_log(LOG_ERR, "rc_get_seqnbr: flock failure: %s: %s", seqfile, strerror(errno)); - fclose(sf); - return rc_guess_seqnbr(); - } - tries++; - if (tries <= 10) - rc_mdelay(500); - else - break; - } - - if (tries > 10) { - rc_log(LOG_ERR,"rc_get_seqnbr: couldn't get lock after %d tries: %s", tries-1, seqfile); - fclose(sf); - return rc_guess_seqnbr(); - } - - rewind(sf); - if (fscanf(sf, "%d", &seq_nbr) != 1) { - rc_log(LOG_ERR,"rc_get_seqnbr: fscanf failure: %s", seqfile); - seq_nbr = rc_guess_seqnbr(); - } - - rewind(sf); - ftruncate(fileno(sf),0); - fprintf(sf,"%d\n", (seq_nbr+1) & UCHAR_MAX); - - fflush(sf); /* fflush because a process may read it between the do_unlock and fclose */ - - if (do_unlock(sf) != 0) - rc_log(LOG_ERR, "rc_get_seqnbr: couldn't release lock on %s: %s", seqfile, strerror(errno)); - - fclose(sf); - - return (unsigned char)seq_nbr; -} - -/* * Function: rc_aaa * * Purpose: Builds an authentication/accounting request for port id client_port
-----Original Message----- From: freeradius-devel-bounces+alex=jajah.com@lists.freeradius.org [mailto:freeradius-devel-bounces+alex=jajah.com@lists.freeradius.org] On Behalf Of Alan DeKok Sent: יום ב 01 פברואר 2010 15:20 To: FreeRadius developers mailing list Subject: Re: freeradius-client sequence file replacement
Alex Massover wrote:
That requires a redesign of the way freeradius-client works, because currently it opens new socket for every request.
Then set the ID field (seq_nbr) to a random number.
Now seq file really limits performance, so for short term I can just drop rc_get_seqnbr() in favor of random number (random code already exists there). Anyway it doesn't matter what id will be there because new socket will be used.
Yes. Set it to a random number. Even rand() here is good enough.
For a long term maybe it worth implementing what you described below, or maybe it's good enough to stay with new socket?
For high performance systems, having multiple packets on one socket would be best.
Alan DeKok. - List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
This mail was received via Mail-SeCure System.
This mail was sent via Mail-SeCure System.