r84429 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r84428‎ \| r84429 \| r84430 >
Date:	22:18, 20 March 2011
Author:	platonides
Status:	fixme (Comments)
Tags:	nodeploy
Comment:	Basic stats
Modified paths:	/trunk/extensions/PoolCounter/daemon/Makefile (modified) (history) /trunk/extensions/PoolCounter/daemon/client_data.c (modified) (history) /trunk/extensions/PoolCounter/daemon/locks.c (modified) (history) /trunk/extensions/PoolCounter/daemon/locks.h (modified) (history) /trunk/extensions/PoolCounter/daemon/main.c (modified) (history) /trunk/extensions/PoolCounter/daemon/stats.c (added) (history) /trunk/extensions/PoolCounter/daemon/stats.h (added) (history) /trunk/extensions/PoolCounter/daemon/stats.list (added) (history)

Diff [purge]

Index: trunk/extensions/PoolCounter/daemon/stats.c
—	—	@@ -0,0 +1,40 @@
	2	+#include <stdio.h>
	3	+#include <string.h>
	4	+#include "stats.h"
	5	+
	6	+
	7	+struct stats stats;
	8	+
	9	+#define COMMAND(item) + sizeof(#item) + 2 + MAX_COUNT_LEN
	10	+static char stats_buffer[
	11	+ sizeof("Uptime: 100000 days, 23h 59m 59s") + 2
	12	+ #include "stats.list"
	13	+];
	14	+#undef COMMAND
	15	+
	16	+const char* provide_stats(const char* type)
	17	+{
	18	+ int seconds = time(NULL) - stats.start;
	19	+ int minutes = seconds / 60;
	20	+ seconds %= 60;
	21	+ int hours = minutes / 60;
	22	+ minutes %= 60;
	23	+ unsigned int days = hours / 24;
	24	+
	25	+ int n;
	26	+ n = sprintf( stats_buffer, "uptime: %u days, %dh %dm %ds\n", days, hours, minutes, seconds );
	27	+
	28	+ if ( !strcasecmp( type, "FULL" ) ) {
	29	+ #define COMMAND(item) n += sprintf( stats_buffer + n, #item ": %" PRcount "\n", stats.item );
	30	+ #include "stats.list"
	31	+ #undef COMMAND
	32	+ strcpy( stats_buffer + n, "\n" );
	33	+ } else if ( strcasecmp( type, "UPTIME" ) ) {
	34	+ #define COMMAND(item) if ( !strcasecmp( type, #item ) ) sprintf( stats_buffer, #item ": %" PRcount "\n", stats.item ); else
	35	+ #include "stats.list"
	36	+ #undef COMMAND
	37	+
	38	+ strcpy( stats_buffer, "ERROR WRONG_STAT" );
	39	+ }
	40	+ return stats_buffer;
	41	+}
Property changes on: trunk/extensions/PoolCounter/daemon/stats.c
___________________________________________________________________
Added: svn:eol-style
1	42	+ native
Index: trunk/extensions/PoolCounter/daemon/locks.h
—	—	@@ -39,7 +39,7 @@
40	40	struct client_data;
41	41	void init_lock(struct locks* l);
42	42	void finish_lock(struct locks* l);
43		~~-char* process_line(struct client_data* cli_data, char* line, int line_len);~~
	43	+const char* process_line(struct client_data* cli_data, char* line, int line_len);
44	44	void process_timeout(struct locks* l);
45	45	void remove_client_lock(struct locks* l, int wakeup_anyones);
46	46	void send_client(struct locks* l, const char* msg);
Index: trunk/extensions/PoolCounter/daemon/stats.h
—	—	@@ -0,0 +1,23 @@
	2	+
	3	+#include <time.h>
	4	+#include <stdint.h>
	5	+#include <inttypes.h>
	6	+
	7	+typedef int64_t count_t;
	8	+#define PRcount PRIi64
	9	+#define MAX_COUNT_LEN sizeof("−9223372036854775808")
	10	+
	11	+struct stats {
	12	+ time_t start;
	13	+
	14	+#define COMMAND(item) volatile count_t item;
	15	+#include "stats.list"
	16	+#undef COMMAND
	17	+
	18	+};
	19	+
	20	+extern struct stats stats;
	21	+const char* provide_stats(const char* type);
	22	+
	23	+#define incr_stats(item) stats.item++
	24	+#define decr_stats(item) stats.item--
Property changes on: trunk/extensions/PoolCounter/daemon/stats.h
___________________________________________________________________
Added: svn:eol-style
1	25	+ native
Index: trunk/extensions/PoolCounter/daemon/main.c
—	—	@@ -12,6 +12,7 @@
13	13	#include "client_data.h"
14	14	#include "prototypes.h"
15	15	#include "locks.h"
	16	+#include "stats.h"
16	17
17	18	static int open_sockets = 1; /* Program will automatically close when this reaches 0 */
18	19
—	—	@@ -38,6 +39,8 @@
39	40	return 1;
40	41	}
41	42
	43	+ stats.start = time( NULL );
	44	+
42	45	event_set( &listener_ev, listener, EV_READ \| EV_PERSIST, on_connect, NULL );
43	46
44	47	event_add( &listener_ev, NULL );
—	—	@@ -91,6 +94,7 @@
92	95	#endif
93	96
94	97	if ( fd == -1 ) {
	98	+ incr_stats( connect_errors );
95	99	perror( "Error accepting" );
96	100	return;
97	101	}
—	—	@@ -104,6 +108,7 @@
105	109
106	110	cli = new_client_data( fd );
107	111	if ( !cli ) {
	112	+ incr_stats( connect_errors );
108	113	perror( "Couldn't allocate the client data! :(" );
109	114	close( fd );
110	115	return;
Index: trunk/extensions/PoolCounter/daemon/client_data.c
—	—	@@ -6,6 +6,7 @@
7	7	#include <malloc.h>
8	8	#include "client_data.h"
9	9	#include "locks.h"
	10	+#include "stats.h"
10	11
11	12	struct client_data* new_client_data(int fd) {
12	13	struct client_data* cd;
—	—	@@ -98,6 +99,7 @@
99	100
100	101	if ( send( cli_data->fd, msg, len, 0) != len ) {
101	102	perror( "Something failed sending message" );
	103	+ incr_stats( failed_sends );
102	104	}
103	105	/* Wait for answer */
104	106	event_add( &cli_data->ev, NULL );
Index: trunk/extensions/PoolCounter/daemon/stats.list
—	—	@@ -0,0 +1,13 @@
	2	+
	3	+/* Preprocessor template for iterating the stats items */
	4	+COMMAND(total_acquired) /* Total number of acquired locks */
	5	+COMMAND(total_releases) /* Total number of released locks */
	6	+COMMAND(hashtable_entries) /* Number of locks currently held */
	7	+COMMAND(processing_workers) /* Number of workers currently doing work */
	8	+COMMAND(waiting_workers) /* Number of workers currently waiting for a lock */
	9	+COMMAND(connect_errors) /* Clients whose connection couldn't be served */
	10	+COMMAND(failed_sends) /* Number of send() calls which didn't succeed */
	11	+COMMAND(full_queues) /* Number of times locks were refused because the queue already had so many workers */
	12	+COMMAND(lock_mismatch) /* Number of times a user tried to do a lock without a previous release */
	13	+COMMAND(release_mismatch) /* Number of times a user tried to do a release without a previous lock */
	14	+
Property changes on: trunk/extensions/PoolCounter/daemon/stats.list
___________________________________________________________________
Added: svn:eol-style
1	15	+ native
Index: trunk/extensions/PoolCounter/daemon/locks.c
—	—	@@ -5,6 +5,7 @@
6	6	#include "locks.h"
7	7	#include "hash.h"
8	8	#include "client_data.h"
	9	+#include "stats.h"
9	10
10	11	void init_lock(struct locks* l) {
11	12	l->state = UNLOCKED;
—	—	@@ -42,7 +43,7 @@
43	44	return num;
44	45	}
45	46
46		~~-char* process_line(struct client_data* cli_data, char* line, int line_len) {~~
	47	+const char* process_line(struct client_data* cli_data, char* line, int line_len) {
47	48	struct locks* l = &cli_data->client_locks;
48	49	if (line_len > 0 && line[line_len-1] == '\r') {
49	50	line_len--;
—	—	@@ -51,6 +52,7 @@
52	53
53	54	if ( !strncmp( line, "ACQ4ME ", 7 ) \|\| !strncmp( line, "ACQ4ANY ", 8 ) ) {
54	55	if ( l->state != UNLOCKED ) {
	56	+ incr_stats( lock_mismatch );
55	57	return "LOCK_HELD\n";
56	58	}
57	59
—	—	@@ -71,7 +73,7 @@
72	74	if ( !pCounter ) {
73	75	pCounter = malloc( sizeof( *pCounter ) );
74	76	if ( !pCounter ) {
75		~~- fprintf(stderr, "Out of memory\n");~~
	77	+ fprintf( stderr, "Out of memory\n" );
76	78	return "ERROR OUT_OF_MEMORY\n";
77	79	}
78	80	pCounter->htentry.key = strdup( key );
—	—	@@ -84,10 +86,13 @@
85	87	DOUBLE_LLIST_INIT( pCounter->for_anyone );
86	88
87	89	hashtable_insert( primary_hashtable, (struct hashtable_entry *) pCounter );
	90	+ incr_stats( hashtable_entries );
88	91	}
89	92
90		~~- if ( pCounter->count >= maxqueue )~~
	93	+ if ( pCounter->count >= maxqueue ) {
	94	+ incr_stats( full_queues );
91	95	return "QUEUE_FULL\n";
	96	+ }
92	97
93	98	l->parent = pCounter;
94	99	pCounter->count++;
—	—	@@ -95,7 +100,9 @@
96	101	if ( pCounter->processing < workers ) {
97	102	l->state = PROCESSING;
98	103	pCounter->processing++;
	104	+ incr_stats( processing_workers );
99	105	DOUBLE_LLIST_ADD( &pCounter->working, &l->siblings );
	106	+ incr_stats( total_acquired );
100	107	return "LOCKED\n";
101	108	} else {
102	109	struct timeval wait_time;
—	—	@@ -106,6 +113,7 @@
107	114	l->state = WAITING;
108	115	DOUBLE_LLIST_ADD( &pCounter->for_them, &l->siblings );
109	116	}
	117	+ incr_stats( waiting_workers );
110	118
111	119	wait_time.tv_sec = timeout;
112	120	wait_time.tv_usec = 0;
—	—	@@ -115,11 +123,15 @@
116	124	}
117	125	} else if ( !strncmp(line, "RELEASE", 7) ) {
118	126	if ( l->state == UNLOCKED ) {
	127	+ incr_stats( release_mismatch );
119	128	return "NOT_LOCKED\n";
120	129	} else {
121	130	remove_client_lock( l, 1 );
	131	+ incr_stats( total_releases );
122	132	return "RELEASED\n";
123	133	}
	134	+ } else if ( !strncmp( line, "STATS ", 6 ) ) {
	135	+ return provide_stats( line + 6 );
124	136	} else {
125	137	return "ERROR BAD_COMMAND\n";
126	138	}
—	—	@@ -128,6 +140,7 @@
129	141	void process_timeout(struct locks* l) {
130	142	if ( ( l->state == WAIT_ANY ) \|\| ( l->state == WAITING ) ) {
131	143	send_client( l, "TIMEOUT\n" );
	144	+ decr_stats( waiting_workers );
132	145	remove_client_lock( l, 0 );
133	146	}
134	147	}
—	—	@@ -139,6 +152,7 @@
140	153	while ( l->parent->for_anyone.next != &l->parent->for_anyone ) {
141	154	send_client( (void*)l->parent->for_anyone.next, "DONE\n" );
142	155	remove_client_lock( (void*)l->parent->for_anyone.next, 0 );
	156	+ decr_stats( waiting_workers );
143	157	}
144	158	}
145	159
—	—	@@ -162,14 +176,18 @@
163	177	DOUBLE_LLIST_ADD( &l->parent->working, &new_owner->siblings );
164	178	send_client( new_owner, "LOCKED\n" );
165	179	new_owner->state = PROCESSING;
	180	+ incr_stats( total_acquired );
	181	+ decr_stats( waiting_workers );
166	182	} else {
167		~~- l->parent->processing--;~~
	183	+ l->parent->processing--;
	184	+ decr_stats( processing_workers );
168	185	}
169	186	}
170	187
171	188	l->state = UNLOCKED;
172	189	l->parent->count--;
173	190	if ( !l->parent->count ) {
	191	+ decr_stats( hashtable_entries );
174	192	hashtable_remove( l->parent->htentry.parent_hashtable, &l->parent->htentry );
175	193	free( l->parent->htentry.key );
176	194	free( l->parent );
Index: trunk/extensions/PoolCounter/daemon/Makefile
—	—	@@ -1,9 +1,9 @@
2	2	CC=gcc
3	3	DEFINES=-DENDIAN_BIG=0 -DENDIAN_LITTLE=1 -DHAVE_ACCEPT4=1
4	4	CFLAGS=-Wall $(DEFINES)
5		~~-OBJS=main.o client_data.o locks.o hash.o~~
	5	+OBJS=main.o client_data.o locks.o hash.o stats.o
6	6	LINK=-levent
7		~~-HEADERS=prototypes.h client_data.h~~
	7	+HEADERS=prototypes.h client_data.h stats.h stats.list
8	8	DESTDIR ?=
9	9
10	10	poolcounterd: $(OBJS)

Sign-offs

User	Flag	Date
Jeenxxx	inspected	20:42, 10 December 2013 (struck 20:43, 10 December 2013)
Jeenxxx	inspected	20:43, 10 December 2013

Comments

#Comment by Tim Starling (talk | contribs) 04:21, 21 March 2011

If you absolutely must use fixed-size output buffers, please use snprintf() instead of sprintf() and strcpy(), to avoid a buffer overflow.

Your assumption that a short non-blocking write to a TCP socket will always succeed is looking even more shaky now that you have the "STATS FULL". If you used bufferevent_write(), you wouldn't have to worry about either that or about accurate sizing of sprintf/snprintf buffers. Memory management would be handled by libevent.

#Comment by RobLa-WMF (talk | contribs) 22:18, 30 January 2012

This is "nodeploy" only because it doesn't automatically get deployed with the rest of 1.19, but needs to be packaged and pushed.

#Comment by Platonides (talk | contribs) 16:41, 31 January 2012

This is already running in the cluster. Looking at r85311 it seems to have been packaged, too. Maybe it's not in puppet , though.

Status & tagging log

15:57, 13 February 2012 Reedy (talk | contribs) changed the tags for r84429 [removed: 1.18ok]
22:18, 30 January 2012 RobLa-WMF (talk | contribs) changed the tags for r84429 [added: nodeploy]
00:42, 24 August 2011 Tim Starling (talk | contribs) changed the tags for r84429 [added: 1.18ok]
04:21, 21 March 2011 Tim Starling (talk | contribs) changed the status of r84429 [removed: new added: fixme]