[Asterisk-code-review] stringfields: Refactor to allow fields to be added to the e... (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

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


Patch Set 4: Code-Review-1

(11 comments)

The master version can take advantage of breaking ABI.  Though, the optimization of moving the separately allocated struct ast_string_field_header into the main stringfield block should be done in a separate patch for simplicity.

https://gerrit.asterisk.org/#/c/2477/4//COMMIT_MSG
Commit Message:

Line 55: structure does only that is the same as adding a char *.
s/does only that is/is/


Line 81: contact->aor, which is a char * at then end of contact, was then changed to an
s/at then/at the/


https://gerrit.asterisk.org/#/c/2477/4/include/asterisk/stringfields.h
File include/asterisk/stringfields.h:

Line 159:   ast_string_field_init_extended(x, e);
Shouldn't the (x, e) be (x, blah)


Line 319: #define AST_STRING_FIELD_EXTENDED(name) ast_string_field name
Why not just define it
#define AST_STRING_FIELD_EXTENDED(name) AST_STRING_FIELD(name)


Line 351: 	if (((void *)(x)) != NULL) { \
I don't think the (void *) cast is needed on any of the NULL tolerance checks.  It might even suppress a valid warning of not passing in a pointer.


Line 583: #define ast_string_fields_cmp(instance1, instance2) \
This should be turned into a function call into stringfields.c.  The macro should just get the necessary offsets for the function parameters.  Actually, I think the new function only needs the __field_mgr.header pointer passed in for each instance.

Thinking about what the operation is supposed to be doing, it makes no sense to compare the vector lengths.  They must be the same or we are comparing different structures with who knows what order of field strings.  We should assert in this case because it violates the purpose of the function.

The interesting thing about the new scheme is the cmp and copy functions will work even if an "old" module that uses this scheme interacts with a "new" module that knows about new string fields.


Line 614: #define ast_string_fields_copy(copy, orig) \
Similar comments for this macro.


https://gerrit.asterisk.org/#/c/2477/4/main/stringfields.c
File main/stringfields.c:

Line 49: #define INITIAL_FIELD_VECTOR_SIZE 24
You should set the initial vector size to the number of normal stringfields.  The vector should be expanded only if there are extended stringfields.  Very few if any structs actually have 24 normal stringfields.

Extended string fields should only be used on released branches.  For master, the extended stringfields should be merged into the normal stringfields.  I wonder if there is a way to enforce this on master.


Line 159: 	AST_VECTOR_FREE(&mgr->header->string_fields);
It looks like you are assuming if needed <= 0 the user is destroying the stringfields.  This is not the case.

Freeing the vector should only be done if needed < 0.  If needed == 0 you are flushing the stringfields.  Essentially resetting the stringfields to the initial pool.  In this case, the user should not be re-initing the extended fields.

I don't know if anyone actually calls this function with needed == 0.


https://gerrit.asterisk.org/#/c/2477/4/tests/test_stringfields.c
File tests/test_stringfields.c:

Line 54: 	} test_struct;
Original bug:
test_struct and test_struct2 need to be zeroed lest a unit test failure cause further badness when execution goes to the error label and tries to ast_string_field_free_memory() on random pointers.


Line 327: 	inst1 = ast_calloc_with_stringfields(1, struct test_struct, 32);
This isn't an adequate test of ast_calloc_with_stringfields().  Calloc allocates an array of structs.  ast_calloc_with_stringfields() also allocates an array of structs with stringfields.  However, I don't think the array property of calloc() is actually used anywhere in Asterisk much less the corresponding array property of ast_calloc_with_stringfields().

In fact actually freeing the array would be problematic.  We don't seem to have the corresponding C++ delete[] operator implemented.


-- 
To view, visit https://gerrit.asterisk.org/2477
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I235db338c5b178f5a13b7946afbaa5d4a0f91d61
Gerrit-PatchSet: 4
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes