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

[George Joseph]

George Joseph has posted comments on this change.

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


Patch Set 4:

(7 comments)

> (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.

In master, I'm thinking ALL stringfields should use the extended pattern and scrap all the offset arithmetic. :)

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

Line 319: #define AST_STRING_FIELD_EXTENDED(name) ast_string_field name
> Why not just define it
Because AST_STRING_FIELD is const and AST_STRING_FIELD_EXTENDED can't be initialized if it's const.


Line 351: 	if (((void *)(x)) != NULL) { \
> I don't think the (void *) cast is needed on any of the NULL tolerance chec
It's needed if you pass the address of an auto structure.

struct blah;
ast_string_field_init(&blah, sblah);

will cause a "the comparison will always evaluate as ‘true’ error if not cast to a void *.


Line 583: #define ast_string_fields_cmp(instance1, instance2) \
> This should be turned into a function call into stringfields.c.  The macro 
True.  I was going to move them but I figured someone would complain if I did. :)


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


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

Line 159: 	AST_VECTOR_FREE(&mgr->header->string_fields);
> It looks like you are assuming if needed <= 0 the user is destroying the st
Actually looking at this again, I'm not sure if needed = 0 ever worked correctly.  If you free all but the embedded pool and the embedded pool wasn't big enough to hold all the initial values, the object would be left in an inconsistent state with some strings being valid and others not.

cel_tds, cdr_tds and chan_sip do call it with 0.


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

Line 54: 	} test_struct;
> Original bug:
Done


Line 327: 	inst1 = ast_calloc_with_stringfields(1, struct test_struct, 32);
> This isn't an adequate test of ast_calloc_with_stringfields().  Calloc allo
Never worked with number of structures set to anything other than 1 because there's no way for the caller figure out where subsequent structures start.  There's an internal calculation that determines the best start of the next structure.

I can put an assert(num_structs == 1);


-- 
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: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes