Experimental Physics and
Industrial Control System

Michael Abbott <[email protected]> · Wed, 23 Jun 2010 07:16:26 -0000

A first response before I dig into the code.

From: [email protected] [mailto:[email protected]] On Behalf Of Jeff Hill
> 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.

Good idea.  Ouch, though.  Guess it's not going to be a small amount of work...

> changes in ca/caProto.h
> > +#   define CA_V412(MINOR) ((MINOR)>=12u)  /* Allow zero length in requests. */
> 
> 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

Ok, so I'll just change my CA_V412 tests to CA_V413.

It's a slight nuisance now that we're going by version number rather than using capability bits, but at the end of the day the history of the code will have to be serialised and presumably as you say the 4.12 stuff is ready to be merged with mainstream?

> 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[];

I'll hunt out the precise detail, but basically what happened here was that I needed to import db_access.h into a C file where LAST_BUFFER_TYPE wasn't defined (there are all sorts of exciting definition conflicts around in this area) -- indeed, I think I moved the definition out of a #ifdef block.  So rather than fight further with more potential conflicts I removed the size definition which, as far as I can tell, would only affect applications of sizeof(dbr_size), which there aren't any -- I don't think leaving these out has any other effect.

I'm pretty sure that the C compiler never checks array sizes when generating access code, so I can't think of any impact apart from making sizeof(dbr_size) fail.

> 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. 
> 
> +                memset(
> +                    (char *) pPayload + data_size, 0, payload_size - data_size);

Um.  Well, it's an interesting point.  I guess I'll probably wince and do it, but I'll take a deeper look first.

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

Great, thanks.

What should I do about the base of my work?  Should I try to rebase it onto a more recent EPICS trunk (don't actually know what bzr is capable of in this respect), or just do the extra changes in place?
-- 
https://code.launchpad.net/~michael-abbott/epics-base/dynamic-array/+merge/26796
Your team EPICS Core Developers is requested to review the proposed merge of lp:~michael-abbott/epics-base/dynamic-array into lp:epics-base.

Experimental Physics and Industrial Control System

Experimental Physics and
Industrial Control System