[Asterisk-code-review] func_callerid+res_agi: Fix compile errors related to -Werror=zero-len... (asterisk[master])

[Jaco Kroon]

Jaco Kroon has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/15496 )

Change subject: func_callerid+res_agi: Fix compile errors related to -Werror=zero-length-bounds
......................................................................


Patch Set 1:

> I do agree though that if this approach is the only one chosen then a comment as mentioned should be added.

Agreed.  Happy to add that, but not if:

> Since there are two patch already implemented is there any reason to not commit both?

No, technically both does apply cleanly and can be merged.

However, if both gets merged, then as per yourself, the comment isn't required, or at least the comment should be different in order to state that's it's not *preferable* to access argv directly using static indexes, however, without looking at the actual code, I suspect argv[] is required in order to actually place the char* pointers correctly.  One could of course use other hacks and eliminate argv[] completely from the struct, but those would be less clean in my opinion.  The other possible advantage of applying the other patch is that AST_STANDARD_APP_ARGS (and friends) could be updated to the cleaner/more concise/more clear ARRAY_LEN macro directly on argv.  This would also be "safer" in the sense that if for some reason some additional fields ends up getting added *AFTER* the argv+arglist "union", that will still work.

There is another "pattern" I've noticed, for example in func_odbc whereby 100 args gets created arbitrarily, since that can either use it's "own" array, or argv, one could conceivably create a parse_and_dynamically_allocate_args function, and then a free_dynamic_args which purely relies on argv[0].  I suspect this may be in potential conflict with the other patch to a degree, the advantage of such an approach would be genuinly arbitrary argument count, as well as not waste stack space since these structs are generally allocated on the stack, but, it would come at the cost of a memory allocation + free, and additional extra pointer indirections.  I don't think this is a high priority - anybody dealing with 100+ arguments in the dialplan has got bigger problems in my opinion.  Additionally, this would REQUIRE that the argv "union" be the last in the struct (which of course can be enforced with a compile time assert (typedef void* __argv_must_be_last_element_of_args_struct[(boolean expr returning true if ct assert is true)-1];) but IMHO the effort isn't worth the gain currently.


-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/15496
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I75152cece8a00b7523d542e5ac22796f9595692b
Gerrit-Change-Number: 15496
Gerrit-PatchSet: 1
Gerrit-Owner: Jaco Kroon <jaco at uls.co.za>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-CC: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Thu, 04 Mar 2021 09:22:53 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210304/7b0d8add/attachment-0001.html>