stringfields: Refactor to allow fields to be added to the end of structures

String fields are great, except that you can't add new ones without breaking
ABI compatibility because it shifts down everything else in the structure.
The only alternative is to add your own char * field to the end of the
structure and manage the memory yourself which isn't ideal, especially since
you then can't use the OPT_STRINGFIELD_T type.

Background:

The reason string fields had to be declared inside the
AST_DECLARE_STRING_FIELDS block was to facilitate iteration over all declared
fields for initialization, compare and copy.  Since AST_DECLARE_STRING_FIELDS
declared the pool, then the fields, then the manager, you could use the offsets
of the pool and manager and iterate over the sequential addresses in between to
access the fields. The actual pool, field allocation and field set operations
don't actually care where the field is.  It's just iteration over the fields
that was the problem.

Solution: Extended String Fields

An extended string field is one that is declared outside the
AST_DECLARE_STRING_FIELDS block but still (anywhere) inside the parent
structure.  Other than using AST_STRING_FIELD_EXTENDED instead of
AST_STRING_FIELD, it looks the same as other string fields.  It's storage comes
from the pool and it participates in string field compare and copy operations
peformed on the parent structure. It's also a valid target for the
OPT_STRINGFIELD_T aco option type.

Implementation:

To keep track of the extended fields and make sure that ABI isn't broken, the
existing embedded_pool pointer in the manager structure was repurposed to be a
pointer to a separate header structure that contains the embedded_pool pointer
plus a vector of fields.  The length of the manager structure didn't change and
the embedded_pool pointer isn't used in the macros, only the stringfields C
code.  A side benefit of this is that changing the header structure in the
future won't break ABI.

ast_string_fields_init initializes the normal string fields and appends them to
the vector, and subsequent calls to ast_string_field_init_extended initialize
and append the extended fields. Cleanup, ast_string_fields_cmp, and
ast_string_fields_copy can now work on the vector instead of sequentially
traversing the addresses between the pool and manager.

The total size of a structure using string fields didn't change, whether using
extended fields or not, nor have the offsets of any structure members, either
inside the original block or outside.  Adding an extended field to the end of a
structure is the same as adding a char *.

Details:

The stringfield C code was pulled out from utils.c and into stringfields.c.
It just made sense.

Additional work was done in ast_string_field_init and
ast_calloc_with_stringfields to handle the allocation of the new header
structure and the vector, and the associated cleanup.  In the process some
additional NULL pointer checking was added.

A lot of work was done in stringfields.h since the logic for compare and copy
is there.  Documentation was added as well as somne additional NULL checking.

The ability to call ast_calloc_with_stringfields with a number of structures
greater than 1 never really worked.  Well, the calloc worked but there was no
way to access the additional structures or clean them up.  It was agreed that
there was no use case for requesting more than 1 structure so an ast_assert
was added to prevent it and the iteration code removed.

Testing:

The stringfield unit tests were updated to test both normal and extended
fields.  Tests for ast_string_field_ptr_set_by_fields and
ast_calloc_with_stringfields were also added.

As an ABI test, 13 was compiled from git and the res_pjsip_* modules, except
res_pjsip itself, saved off.  The patch was then added and a full compile and
install was performed.  Then the older res_pjsip_* moduled were copied over the
installed versions so res_pjsip was new and the rest were old.  No issues.

contact->aor, which is a char * at the end of contact, was then changed to an
extended string field and a recompile and reinstall was performed, again
leaving stock versions of the the res_pjsip_* modules.  Again, no issues with
the res_pjsip_* modules using the old stringfield implementation and with
contact->aor as a char *, and res_pjsip itself using the new stringfield
implementation and contact->aor being an extended string field.

Finally, several existing string fields were converted to extended string
fields to test OPT_STRINGFIELD_T.  Again, no issues.

Change-Id: I235db338c5b178f5a13b7946afbaa5d4a0f91d61

1 files changed, 159 insertions, 46 deletions

diff --git a/tests/test_stringfields.c b/tests/test_stringfields.c
index 2eeb83d45..e57f8d7e8 100644
--- a/tests/test_stringfields.c
+++ b/tests/test_stringfields.c
@@ -24,7 +24,6 @@
  *
  * Test module for string fields API
  * \ingroup tests
- * \todo need to test ast_calloc_with_stringfields
  */
 
 /*** MODULEINFO
@@ -50,16 +49,16 @@ AST_TEST_DEFINE(string_field_test)
 	struct {
 		AST_DECLARE_STRING_FIELDS (
 			AST_STRING_FIELD(string1);
-			AST_STRING_FIELD(string2);
 		);
+		AST_STRING_FIELD_EXTENDED(string2);
 	} test_struct;
 
 	struct {
 		AST_DECLARE_STRING_FIELDS (
 			AST_STRING_FIELD(string1);
 			AST_STRING_FIELD(string2);
-			AST_STRING_FIELD(string3);
 		);
+		AST_STRING_FIELD_EXTENDED(string3);
 	} test_struct2;
 
 	switch (cmd) {
@@ -74,6 +73,9 @@ AST_TEST_DEFINE(string_field_test)
 		break;
 	}
 
+	memset(&test_struct, 0, sizeof(test_struct));
+	memset(&test_struct2, 0, sizeof(test_struct));
+
 	ast_test_status_update(test, "First things first. Let's see if we can actually allocate string fields\n");
 
 	if (ast_string_field_init(&test_struct, 32)) {
@@ -82,6 +84,7 @@ AST_TEST_DEFINE(string_field_test)
 	} else {
 		ast_test_status_update(test, "All right! Successfully allocated! Now let's get down to business\n");
 	}
+	ast_string_field_init_extended(&test_struct, string2);
 
 	ast_test_status_update(test,"We're going to set some string fields and perform some checks\n");
 
@@ -255,6 +258,8 @@ AST_TEST_DEFINE(string_field_test)
 
 	ast_string_field_init(&test_struct2, 32);
 	ast_test_status_update(test, "Now using a totally separate area of memory we're going to test a basic pool freeing scenario\n");
+	ast_string_field_init_extended(&test_struct2, string3);
+
 
 	ast_string_field_set(&test_struct2, string1, "first");
 	ast_string_field_set(&test_struct2, string2, "second");
@@ -294,15 +299,22 @@ error:
 	return AST_TEST_FAIL;
 }
 
+struct test_struct {
+	int foo;
+	AST_DECLARE_STRING_FIELDS (
+		AST_STRING_FIELD(string1);
+	);
+	int foo2;
+	AST_STRING_FIELD_EXTENDED(string2);
+};
+
 AST_TEST_DEFINE(string_field_aggregate_test)
 {
-	struct test_struct {
-		AST_DECLARE_STRING_FIELDS (
-			AST_STRING_FIELD(string1);
-			AST_STRING_FIELD(string2);
-		);
-		int foo;
-	} inst1, inst2, inst3, inst4;
+	enum ast_test_result_state res = AST_TEST_PASS;
+	struct test_struct *inst1 = NULL;
+	struct test_struct *inst2 = NULL;
+	struct test_struct *inst3 = NULL;
+	struct test_struct *inst4 = NULL;
 
 	switch (cmd) {
 	case TEST_INIT:
@@ -316,88 +328,189 @@ AST_TEST_DEFINE(string_field_aggregate_test)
 		break;
 	}
 
-	ast_string_field_init(&inst1, 32);
-	ast_string_field_init(&inst2, 32);
-	ast_string_field_init(&inst3, 32);
-	ast_string_field_init(&inst4, 32);
+	inst1 = ast_calloc_with_stringfields(1, struct test_struct, 32);
+	if (!inst1) {
+		ast_test_status_update(test, "Unable to allocate structure 1!\n");
+		res = AST_TEST_FAIL;
+		goto error;
+	}
+	ast_string_field_init_extended(inst1, string2);
+
+	inst2 = ast_calloc_with_stringfields(1, struct test_struct, 32);
+	if (!inst2) {
+		ast_test_status_update(test, "Unable to allocate structure 2!\n");
+		res = AST_TEST_FAIL;
+		goto error;
+	}
+	ast_string_field_init_extended(inst2, string2);
+
+	inst3 = ast_calloc_with_stringfields(1, struct test_struct, 32);
+	if (!inst3) {
+		ast_test_status_update(test, "Unable to allocate structure 3!\n");
+		res = AST_TEST_FAIL;
+		goto error;
+	}
+	ast_string_field_init_extended(inst3, string2);
+
+	inst4 = ast_calloc_with_stringfields(1, struct test_struct, 32);
+	if (!inst4) {
+		ast_test_status_update(test, "Unable to allocate structure 4!\n");
+		res = AST_TEST_FAIL;
+		goto error;
+	}
+	ast_string_field_init_extended(inst4, string2);
+
+
+	ast_string_field_set(inst1, string1, "foo");
+	ast_string_field_set(inst1, string2, "bar");
+	inst1->foo = 1;
+
+	ast_string_field_ptr_set_by_fields(inst2->__field_mgr_pool, inst2->__field_mgr, &inst2->string2, "bar");
+	ast_string_field_ptr_set_by_fields(inst2->__field_mgr_pool, inst2->__field_mgr, &inst2->string1, "foo");
+	inst2->foo = 2;
+
+	if (inst3->__field_mgr.header->embedded_pool->prev) {
+		ast_test_status_update(test, "Structure 3 embedded pool should not have a previous pool!\n");
+		res = AST_TEST_FAIL;
+		goto error;
+	}
+
+	ast_string_field_set(inst3, string1, "foo");
+
+	if (inst3->__field_mgr.header->embedded_pool != inst3->__field_mgr_pool) {
+		ast_test_status_update(test, "Structure 3 embedded pool should have been the current pool!\n");
+		res = AST_TEST_FAIL;
+		goto error;
+	}
+
+	if (inst3->__field_mgr.header->embedded_pool->prev) {
+		ast_test_status_update(test, "Structure 3 embedded pool should not have a previous pool!\n");
+		res = AST_TEST_FAIL;
+		goto error;
+	}
+
+	ast_test_status_update(test, "Structures 3 embedded pool initialized successfully.\n");
+
+	/* Exhaust the embedded pool */
+	ast_string_field_set(inst3, string2, "baz 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890");
+	inst3->foo = 3;
 
-	ast_string_field_set(&inst1, string1, "foo");
-	ast_string_field_set(&inst1, string2, "bar");
-	inst1.foo = 1;
+	if (inst3->__field_mgr_pool == inst3->__field_mgr.header->embedded_pool) {
+		ast_test_status_update(test, "Structure 3 embedded pool should not have been the current pool!\n");
+		res = AST_TEST_FAIL;
+		goto error;
+	}
 
-	ast_string_field_set(&inst2, string2, "bar");
-	ast_string_field_set(&inst2, string1, "foo");
-	inst2.foo = 2;
+	if (inst3->__field_mgr.header->embedded_pool != inst3->__field_mgr_pool->prev) {
+		ast_test_status_update(test, "Structure 3 embedded pool should be the current pool's previous!\n");
+		res = AST_TEST_FAIL;
+		goto error;
+	}
 
-	ast_string_field_set(&inst3, string1, "foo");
-	ast_string_field_set(&inst3, string2, "baz");
-	inst3.foo = 3;
+	ast_test_status_update(test, "Structures 3 additional pool initialized successfully.\n");
 
-	ast_string_field_set(&inst4, string1, "faz");
-	ast_string_field_set(&inst4, string2, "baz");
-	inst4.foo = 3;
+	ast_string_field_set(inst4, string1, "faz");
+	/* Exhaust the embedded pool */
+	ast_string_field_set(inst4, string2, "baz 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890 1234567890");
+	inst4->foo = 4;
 
-	if (ast_string_fields_cmp(&inst1, &inst2)) {
+	if (ast_string_fields_cmp(inst1, inst2)) {
 		ast_test_status_update(test, "Structures 1/2 should be equal!\n");
+		res = AST_TEST_FAIL;
 		goto error;
 	} else {
 		ast_test_status_update(test, "Structures 1/2 are equal as expected.\n");
 	}
 
-	if (!ast_string_fields_cmp(&inst1, &inst3)) {
+	if (!ast_string_fields_cmp(inst1, inst3)) {
 		ast_test_status_update(test, "Structures 1/3 should be different!\n");
+		res = AST_TEST_FAIL;
 		goto error;
 	} else {
 		ast_test_status_update(test, "Structures 1/3 are different as expected.\n");
 	}
 
-	if (!ast_string_fields_cmp(&inst2, &inst3)) {
+	if (!ast_string_fields_cmp(inst2, inst3)) {
 		ast_test_status_update(test, "Structures 2/3 should be different!\n");
+		res = AST_TEST_FAIL;
 		goto error;
 	} else {
 		ast_test_status_update(test, "Structures 2/3 are different as expected.\n");
 	}
 
-	if (!ast_string_fields_cmp(&inst3, &inst4)) {
+	if (!ast_string_fields_cmp(inst3, inst4)) {
 		ast_test_status_update(test, "Structures 3/4 should be different!\n");
+		res = AST_TEST_FAIL;
 		goto error;
 	} else {
 		ast_test_status_update(test, "Structures 3/4 are different as expected.\n");
 	}
 
-	if (ast_string_fields_copy(&inst1, &inst3)) {
-		ast_test_status_update(test, "Copying from structure 3 to structure 4 failed!\n");
+	if (ast_string_fields_copy(inst1, inst3)) {
+		ast_test_status_update(test, "Copying from structure 3 to structure 1 failed!\n");
+		res = AST_TEST_FAIL;
 		goto error;
 	} else {
-		ast_test_status_update(test, "Copying from structure 3 to structure 4 succeeded!\n");
+		ast_test_status_update(test, "Copying from structure 3 to structure 1 succeeded!\n");
 	}
 
 	/* inst1 and inst3 should now be equal and inst1 should no longer be equal to inst2 */
-	if (ast_string_fields_cmp(&inst1, &inst3)) {
+	if (ast_string_fields_cmp(inst1, inst3)) {
 		ast_test_status_update(test, "Structures 1/3 should be equal!\n");
+		res = AST_TEST_FAIL;
 		goto error;
 	} else {
 		ast_test_status_update(test, "Structures 1/3 are equal as expected.\n");
 	}
 
-	if (!ast_string_fields_cmp(&inst1, &inst2)) {
+	if (!ast_string_fields_cmp(inst1, inst2)) {
 		ast_test_status_update(test, "Structures 1/2 should be different!\n");
-		goto error;
+		res = AST_TEST_FAIL;
 	} else {
 		ast_test_status_update(test, "Structures 1/2 are different as expected.\n");
 	}
 
-	ast_string_field_free_memory(&inst1);
-	ast_string_field_free_memory(&inst2);
-	ast_string_field_free_memory(&inst3);
-	ast_string_field_free_memory(&inst4);
-	return AST_TEST_PASS;
+	ast_test_status_update(test, "Reset but don't free.\n");
+
+	ast_string_field_init(inst1, AST_STRINGFIELD_RESET);
+	ast_string_field_init(inst2, AST_STRINGFIELD_RESET);
+	ast_string_field_init(inst3, AST_STRINGFIELD_RESET);
+	ast_string_field_init(inst4, AST_STRINGFIELD_RESET);
+
+	if (ast_string_fields_cmp(inst1, inst2)) {
+		ast_test_status_update(test, "Structures 1/2 should be the same (empty)!\n");
+		res = AST_TEST_FAIL;
+	} else {
+		ast_test_status_update(test, "Structures 1/2 are the same (empty) as expected.\n");
+	}
+
+	if (inst4->__field_mgr.header->embedded_pool != inst4->__field_mgr_pool) {
+		ast_test_status_update(test, "Structure 4 embedded pool should have been the current pool!\n");
+		res = AST_TEST_FAIL;
+		goto error;
+	} else {
+		ast_test_status_update(test, "Structure 4 embedded pool is the current pool as expected.\n");
+	}
+
+	if (inst4->__field_mgr.header->embedded_pool->prev) {
+		ast_test_status_update(test, "Structure 4 embedded pool should not have a previous pool!\n");
+		res = AST_TEST_FAIL;
+		goto error;
+	} else {
+		ast_test_status_update(test, "Structure 4 embedded pool does not have a previous as expected.\n");
+	}
+
 error:
-	ast_string_field_free_memory(&inst1);
-	ast_string_field_free_memory(&inst2);
-	ast_string_field_free_memory(&inst3);
-	ast_string_field_free_memory(&inst4);
-	return AST_TEST_FAIL;
+	ast_string_field_free_memory(inst1);
+	ast_free(inst1);
+	ast_string_field_free_memory(inst2);
+	ast_free(inst2);
+	ast_string_field_free_memory(inst3);
+	ast_free(inst3);
+	ast_string_field_free_memory(inst4);
+	ast_free(inst4);
+
+	return res;
 }
 
 static int unload_module(void)