Browse code

cfg framework: fix the freeing of the replaced strings

The replaced strings and the memory block of the replaced
group instances cannot be freed when the old configuration
block is freed. There might be a child process using an even older
configuration that references to the same string value or to the same
group instance that is beeing replaced. Hence, as long as there
is any child process with an older configuration, the replaced
strings cannot be freed.

The fix is to link the replaced strings to the per-child process
callback list instead of the old cfg block. When the last child process
updates its configuration, it also frees the old string values.

Miklos Tirpak authored on 15/09/2011 15:05:36
Showing 3 changed files
... ...
@@ -649,7 +649,7 @@ error:
649 649
 	if (cfg_shmized) CFG_WRITER_UNLOCK();
650 650
 	if (block) cfg_block_free(block);
651 651
 	if (new_array) shm_free(new_array);
652
-	if (child_cb) cfg_child_cb_free(child_cb);
652
+	if (child_cb) cfg_child_cb_free_list(child_cb);
653 653
 	if (replaced) shm_free(replaced);
654 654
 
655 655
 error0:
... ...
@@ -1235,7 +1235,7 @@ error:
1235 1235
 error0:
1236 1236
 	CFG_CTX_UNLOCK(ctx);
1237 1237
 
1238
-	if (child_cb_first) cfg_child_cb_free(child_cb_first);
1238
+	if (child_cb_first) cfg_child_cb_free_list(child_cb_first);
1239 1239
 	if (replaced) shm_free(replaced);
1240 1240
 
1241 1241
 	return -1;
... ...
@@ -401,7 +401,7 @@ void cfg_destroy(void)
401 401
 	cfg_free_selects();
402 402
 
403 403
 	if (cfg_child_cb_first) {
404
-		if (*cfg_child_cb_first) cfg_child_cb_free(*cfg_child_cb_first);
404
+		if (*cfg_child_cb_first) cfg_child_cb_free_list(*cfg_child_cb_first);
405 405
 		shm_free(cfg_child_cb_first);
406 406
 		cfg_child_cb_first = NULL;
407 407
 	}
... ...
@@ -533,7 +533,7 @@ void cfg_child_destroy(void)
533 533
 			if (*cfg_child_cb_first == prev_cb) {
534 534
 				/* yes, this process was blocking the deletion */
535 535
 				*cfg_child_cb_first = cfg_child_cb;
536
-				shm_free(prev_cb);
536
+				cfg_child_cb_free_item(prev_cb);
537 537
 			}
538 538
 		} else {
539 539
 			/* no need to continue, because there is at least
... ...
@@ -793,6 +793,26 @@ void cfg_install_global(cfg_block_t *block, void **replaced,
793 793
 	
794 794
 	CFG_REF(block);
795 795
 
796
+	if (replaced) {
797
+		/* The replaced array is specified, it has to be linked to the child cb structure.
798
+		 * The last child process processing this structure will free the old strings and the array. */
799
+		if (cb_first) {
800
+			cb_first->replaced = replaced;
801
+		} else {
802
+			/* At least one child cb structure is needed. */
803
+			cb_first = cfg_child_cb_new(NULL, NULL, NULL, 0 /* gname, name, cb, type */);
804
+			if (cb_first) {
805
+				cb_last = cb_first;
806
+				cb_first->replaced = replaced;
807
+			} else {
808
+				LOG(L_ERR, "ERROR: cfg_install_global(): not enough shm memory\n");
809
+				/* Nothing more can be done here, the replaced strings are still needed,
810
+				 * they cannot be freed at this moment.
811
+				 */
812
+			}
813
+		}
814
+	}
815
+
796 816
 	CFG_LOCK();
797 817
 
798 818
 	old_cfg = *cfg_global;
... ...
@@ -803,11 +823,8 @@ void cfg_install_global(cfg_block_t *block, void **replaced,
803 823
 
804 824
 	CFG_UNLOCK();
805 825
 	
806
-	if (old_cfg) {
807
-		if (replaced) (old_cfg)->replaced = replaced;
826
+	if (old_cfg)
808 827
 		CFG_UNREF(old_cfg);
809
-	}
810
-
811 828
 }
812 829
 
813 830
 /* creates a structure for a per-child process callback */
... ...
@@ -854,7 +871,7 @@ cfg_child_cb_t *cfg_child_cb_new(str *gname, str *name,
854 871
 }
855 872
 
856 873
 /* free the memory allocated for a child cb list */
857
-void cfg_child_cb_free(cfg_child_cb_t *child_cb_first)
874
+void cfg_child_cb_free_list(cfg_child_cb_t *child_cb_first)
858 875
 {
859 876
 	cfg_child_cb_t	*cb, *cb_next;
860 877
 
... ...
@@ -863,7 +880,7 @@ void cfg_child_cb_free(cfg_child_cb_t *child_cb_first)
863 880
 		cb = cb_next
864 881
 	) {
865 882
 		cb_next = cb->next;
866
-		shm_free(cb);
883
+		cfg_child_cb_free_item(cb);
867 884
 	}
868 885
 }
869 886
 
... ...
@@ -136,10 +136,6 @@ typedef struct _cfg_block {
136 136
 	atomic_t	refcnt;		/*!< reference counter,
137 137
 					the block is automatically deleted
138 138
 					when it reaches 0 */
139
-	void		**replaced;	/*!< set of the strings and other memory segments
140
-					that must be freed
141
-					together with the block. The content depends
142
-					on the block that replaces this one */
143 139
 	unsigned char	vars[1];	/*!< blob that contains the values */
144 140
 } cfg_block_t;
145 141
 
... ...
@@ -159,7 +155,12 @@ typedef struct _cfg_child_cb {
159 155
 						 * <=0 the cb no longer needs to be executed
160 156
 						 */
161 157
 	str			gname, name;	/*!< name of the variable that has changed */
162
-	cfg_on_set_child	cb;	/*!< callback function that has to be called */
158
+	cfg_on_set_child	cb;		/*!< callback function that has to be called */
159
+	void			**replaced;	/*!< set of strings and other memory segments
160
+						that must be freed together with this structure.
161
+						The content depends on the new config block.
162
+						This makes sure that the replaced strings are freed
163
+						after all the child processes release the old configuration. */
163 164
 
164 165
 	struct _cfg_child_cb	*next;
165 166
 } cfg_child_cb_t;
... ...
@@ -274,20 +275,23 @@ void cfg_set_group(cfg_group_t *group,
274 275
 /* copy the variables to shm mem */
275 276
 int cfg_shmize(void);
276 277
 
277
-/* free the memory of a config block */
278
-static inline void cfg_block_free(cfg_block_t *block)
278
+/* free the memory of a child cb structure */
279
+static inline void cfg_child_cb_free_item(cfg_child_cb_t *cb)
279 280
 {
280 281
 	int	i;
281 282
 
282 283
 	/* free the changed variables */
283
-	if (block->replaced) {
284
-		for (i=0; block->replaced[i]; i++)
285
-			shm_free(block->replaced[i]);
286
-		shm_free(block->replaced);
284
+	if (cb->replaced) {
285
+		for (i=0; cb->replaced[i]; i++)
286
+			shm_free(cb->replaced[i]);
287
+		shm_free(cb->replaced);
287 288
 	}
288
-	shm_free(block);
289
+	shm_free(cb);
289 290
 }
290 291
 
292
+#define cfg_block_free(block) \
293
+	shm_free(block)
294
+
291 295
 /* Move the group handle to the specified group instance pointed by dst_ginst.
292 296
  * src_ginst shall point to the active group instance.
293 297
  * Both parameters can be NULL meaning that the src/dst config is the default, 
... ...
@@ -369,13 +373,15 @@ static inline void cfg_update_local(int no_cbs)
369 373
 				/* yes, this process was blocking the deletion */
370 374
 				*cfg_child_cb_first = cfg_child_cb;
371 375
 				CFG_UNLOCK();
372
-				shm_free(prev_cb);
376
+				cfg_child_cb_free_item(prev_cb);
373 377
 			} else {
374 378
 				CFG_UNLOCK();
375 379
 			}
376 380
 		}
377
-		if (atomic_add(&cfg_child_cb->cb_count, -1) >= 0) /* the new value is returned
378
-								by atomic_add() */
381
+		if (cfg_child_cb->cb
382
+			&& (atomic_add(&cfg_child_cb->cb_count, -1) >= 0) /* the new value is returned
383
+									by atomic_add() */
384
+		)
379 385
 			/* execute the callback */
380 386
 			cfg_child_cb->cb(&cfg_child_cb->gname, &cfg_child_cb->name);
381 387
 		/* else the callback no longer needs to be executed */
... ...
@@ -500,7 +506,7 @@ cfg_child_cb_t *cfg_child_cb_new(str *gname, str *name,
500 506
 			unsigned int type);
501 507
 
502 508
 /* free the memory allocated for a child cb list */
503
-void cfg_child_cb_free(cfg_child_cb_t *child_cb_first);
509
+void cfg_child_cb_free_list(cfg_child_cb_t *child_cb_first);
504 510
 
505 511
 /* Allocate memory for a new additional variable
506 512
  * and link it to a configuration group.