Browse code

- Call the per-child process callback functions even if the config variables are changed before forking, so the modules/core will not miss the change.

- fixing a very unlikely bug: when a module kept forking and destroying
new processes runtime (only jabber module does so), and two variables
were changed after the forked process called called cfg_update()
and before exited, and both variables had per-child process callback,
and all the other child processes updated their own local config faster
then this one, the list of the callbacks was not released, thus the
memory was not freed.

Miklos Tirpak authored on 08/02/2008 17:09:45
Showing 3 changed files
... ...
@@ -303,16 +303,16 @@ int cfg_set_now(cfg_ctx_t *ctx, str *group_name, str *var_name,
303 303
 
304 304
 	}
305 305
 
306
-	if (cfg_shmized) {
307
-		if (var->def->on_set_child_cb) {
308
-			child_cb = cfg_child_cb_new(var_name,
309
-						var->def->on_set_child_cb);
310
-			if (!child_cb) {
311
-				LOG(L_ERR, "ERROR: cfg_set_now(): not enough shm memory\n");
312
-				goto error0;
313
-			}
306
+	if (var->def->on_set_child_cb) {
307
+		child_cb = cfg_child_cb_new(var_name,
308
+					var->def->on_set_child_cb);
309
+		if (!child_cb) {
310
+			LOG(L_ERR, "ERROR: cfg_set_now(): not enough shm memory\n");
311
+			goto error0;
314 312
 		}
313
+	}
315 314
 
315
+	if (cfg_shmized) {
316 316
 		/* make sure that nobody else replaces the global config
317 317
 		while the new one is prepared */
318 318
 		CFG_WRITER_LOCK();
... ...
@@ -381,6 +381,11 @@ int cfg_set_now(cfg_ctx_t *ctx, str *group_name, str *var_name,
381 381
 		/* flag the variable because there is no need
382 382
 		to shmize it again */
383 383
 		var->flag |= cfg_var_shmized;
384
+
385
+		/* the global config does not have to be replaced,
386
+		but the child callback has to be installed, otherwise the
387
+		child processes will miss the change */
388
+		cfg_install_child_cb(child_cb, child_cb);
384 389
 	}
385 390
 
386 391
 	if (val_type == CFG_VAR_INT)
... ...
@@ -385,28 +385,50 @@ int cfg_child_init(void)
385 385
  */
386 386
 void cfg_child_destroy(void)
387 387
 {
388
+	cfg_child_cb_t	*prev_cb;
389
+
388 390
 	/* unref the local config */
389 391
 	if (cfg_local) {
390 392
 		CFG_UNREF(cfg_local);
391 393
 		cfg_local = NULL;
392 394
 	}
393 395
 
394
-	/* unref the per-process callback list */
395
-	if (atomic_dec_and_test(&cfg_child_cb->refcnt)) {
396
-		/* No more pocess refers to this callback.
397
-		Did this process block the deletion,
398
-		or is there any other process that has not
399
-		reached	prev_cb yet? */
400
-		CFG_LOCK();
401
-		if (*cfg_child_cb_first == cfg_child_cb) {
402
-			/* yes, this process was blocking the deletion */
403
-			*cfg_child_cb_first = cfg_child_cb->next;
404
-			CFG_UNLOCK();
405
-			shm_free(cfg_child_cb);
396
+	if (!cfg_child_cb) return;
397
+
398
+	/* The lock must be held to make sure that the global config
399
+	is not replaced meantime, and the other child processes do not
400
+	leave the old value of *cfg_child_cb_last. Otherwise it could happen,
401
+	that all the other processes move their own cfg_child_cb pointer before
402
+	this process reaches *cfg_child_cb_last, though, it is very unlikely. */
403
+	CFG_LOCK();
404
+
405
+	/* go through the list and check whether there is any item that
406
+	has to be freed (similar to cfg_update_local(), but without executing
407
+	the callback functions) */
408
+	while (cfg_child_cb != *cfg_child_cb_last) {
409
+		prev_cb = cfg_child_cb;
410
+		cfg_child_cb = cfg_child_cb->next;
411
+		atomic_inc(&cfg_child_cb->refcnt);
412
+		if (atomic_dec_and_test(&prev_cb->refcnt)) {
413
+			/* No more pocess refers to this callback.
414
+			Did this process block the deletion,
415
+			or is there any other process that has not
416
+			reached	prev_cb yet? */
417
+			if (*cfg_child_cb_first == prev_cb) {
418
+				/* yes, this process was blocking the deletion */
419
+				*cfg_child_cb_first = cfg_child_cb;
420
+				shm_free(prev_cb);
421
+			}
406 422
 		} else {
407
-			CFG_UNLOCK();
423
+			/* no need to continue, because there is at least
424
+			one process that stays exactly at the same point
425
+			in the list, so it will free the items later */
426
+			break;
408 427
 		}
409 428
 	}
429
+	atomic_dec(&cfg_child_cb->refcnt);
430
+
431
+	CFG_UNLOCK();
410 432
 	cfg_child_cb = NULL;
411 433
 }
412 434
 
... ...
@@ -480,6 +502,18 @@ cfg_block_t *cfg_clone_global(void)
480 502
 	return block;
481 503
 }
482 504
 
505
+/* append new callbacks to the end of the child callback list
506
+ *
507
+ * WARNING: the function is unsafe, either hold CFG_LOCK(),
508
+ * or call the function before forking
509
+ */
510
+void cfg_install_child_cb(cfg_child_cb_t *cb_first, cfg_child_cb_t *cb_last)
511
+{
512
+	/* add the new callbacks to the end of the linked-list */
513
+	(*cfg_child_cb_last)->next = cb_first;
514
+	*cfg_child_cb_last = cb_last;
515
+}
516
+
483 517
 /* installs a new global config
484 518
  *
485 519
  * replaced is an array of strings that must be freed together
... ...
@@ -499,11 +533,8 @@ void cfg_install_global(cfg_block_t *block, char **replaced,
499 533
 	CFG_REF(block);
500 534
 	*cfg_global = block;
501 535
 
502
-	if (cb_first) {
503
-		/* add the new callbacks to the end of the linked-list */
504
-		(*cfg_child_cb_last)->next = cb_first;
505
-		*cfg_child_cb_last = cb_last;
506
-	}
536
+	if (cb_first)
537
+		cfg_install_child_cb(cb_first, cb_last);
507 538
 
508 539
 	CFG_UNLOCK();
509 540
 
... ...
@@ -262,6 +262,13 @@ cfg_block_t *cfg_clone_global(void);
262 262
 /* clones a string to shared memory */
263 263
 int cfg_clone_str(str *src, str *dst);
264 264
 
265
+/* append new callbacks to the end of the child callback list
266
+ *
267
+ * WARNING: the function is unsafe, either hold CFG_LOCK(),
268
+ * or call the function before forking
269
+ */
270
+void cfg_install_child_cb(cfg_child_cb_t *cb_first, cfg_child_cb_t *cb_last);
271
+
265 272
 /* installs a new global config
266 273
  *
267 274
  * replaced is an array of strings that must be freed together