Code review comment for lp:~michael-abbott/epics-base/dynamic-array

Hi Michael,

Thanks for your help.

Please add a client side initiated regression test to acctst.c that verifies that the new feature works properly. Sorry, that acctst.c is a messy code for certain but it does serve its purpose of preventing introduction of bugs that have been seen before. This will also have the benefit of not allowing my neglect of parallel changes for these features in the new server :^). Also, running acctst might even help to test the features you have added.

changes in ca/caProto.h
> +# define CA_V412(MINOR) ((MINOR)>=12u) /* Allow zero length in
> requests. */

The following is what I have on the Bazaar branch which was based on the cvs main trunk (this is where I have always added new features in the past so that I didn't mix together patch level changes in the R3.14 branch (i.e. fixes) with feature upgrades).

# define CA_V412(MINOR) ((MINOR)>=12u) /* TCP-based search requests */
# define CA_V413(MINOR) ((MINOR)>=13u) /* channel connectivity response messages */

So we have some conflicts with the new TCP search protocol and some capabilities I have in the new server which allow the service to report the current state of the connectivity it has with its channel.

I propose the following based on the order (in time) that each upgrade might be installed into base (the TCP based search request changes were completed a long time ago):

o V412 - TCP based search requests
o V413 - dynamic payload sized subscription / get callback response message
o V414 - connectivity response messages

I am confused about the purpose of this change (presumably the old code was safer because the client code's compiler can check for access past the end of the array)?

diff --git a/src/ca/db_access.h b/src/ca/db_access.h
-epicsShareExtern const unsigned short dbr_size[LAST_BUFFER_TYPE+1];
+epicsShareExtern const unsigned short dbr_size[];

 /* size for each type's value - array indexed by the DBR_ type code */
-epicsShareExtern const unsigned short dbr_value_size[LAST_BUFFER_TYPE+1];
+epicsShareExtern const unsigned short dbr_value_size[];

Changes to rssrv/camessage.c

I am a bit concerned that if for whatever reason db_get_field_and_count returns an item_count greater than requested then we could have data_size greater than payload_size which could cause the unsigned subtract to overflow here. That could cause memset to copy many (way too many) elements which would almost certainly profoundly crash the IOC. Can we afford the risk? I would add an "if (payload_size - data_size)" sanity check here. Yes, nannying but robust.

returns data_size greater
+ memset(
+ (char *) pPayload + data_size, 0, payload_size - data_size);

Otherwise, I did finally have a very close look today, and it looks good.