RE: [Merge] lp:~michael-abbott/epics-base/dynamic-array intolp:epics-base

["Jeff Hill" <johill@lanl.gov>]

Hi Nick,

While acctst is the only CA client side initiated testing we have right now,
but I think that we can all agree that there is room for improvement. In
particular, better modularization of the tests and improved integration with
the Andrew's, and Diamond's automated test harnesses would certainly be
beneficial. No doubt the completeness (code coverage) of the testing can be
measured, and improved. As Ralph has mentioned, acctst currently has one
test that requires human intervention forcing a circuit to disconnect. There
are of course many other off normal network situations that are more
difficult to manipulate as external test inputs. For example, we could
probably set up a test forcing the ca client to use an artificially slow
pipe. A test like that might provide better confirmation of some of the
dynamic behaviors in the CA libraries.

> Is this a sensible idea?

Definitely, thanks for volunteering to do the work ;-)

Jeff
______________________________________________________
Jeffrey O. Hill           Email        johill@lanl.gov
LANL MS H820              Voice        505 665 1831
Los Alamos NM 87545 USA   FAX          505 665 5107

Message content: TSPA


> -----Original Message-----
> From: core-talk-bounces@aps.anl.gov [mailto:core-talk-
> bounces@aps.anl.gov] On Behalf Of nick.rees@diamond.ac.uk
> Sent: Thursday, June 24, 2010 2:55 AM
> To: core-talk@aps.anl.gov
> Subject: RE: [Merge] lp:~michael-abbott/epics-base/dynamic-array
> intolp:epics-base
> 
> Jeff,
> 
> You wrote:
> 
> > 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.
> 
> I don't think that acctst is a valid epics test harness. I suggest we
> also look at turning it into one so that it is run with make runtests
> (and also with our automated test system). Is this a sensible idea?
> 
> Cheers,
> 
> Nick Rees
> Principal Software Engineer           Phone: +44 (0)1235-778430
> Diamond Light Source                  Fax:   +44 (0)1235-446713
> 
> > -----Original Message-----
> > From: core-talk-bounces@aps.anl.gov
> > [mailto:core-talk-bounces@aps.anl.gov] On Behalf Of Jeff Hill
> > Sent: 22 June 2010 21:56
> > To: mp+26796@code.launchpad.net
> > Subject: Re: [Merge]
> > lp:~michael-abbott/epics-base/dynamic-array intolp:epics-base
> >
> >
> > Hi Michael,
> >
> > Thanks for your help.
> >
> >
> > 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.
> >
> > --
> > 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.
> >
> >
> 
> --
> This e-mail and any attachments may contain confidential, copyright and
> or privileged material, and are for the use of the intended addressee
> only. If you are not the intended addressee or an authorised recipient of
> the addressee please notify us of receipt by returning the e-mail and do
> not use, copy, retain, distribute or disclose the information in or
> attached to the e-mail.
> Any opinions expressed within this e-mail are those of the individual and
> not necessarily of Diamond Light Source Ltd.
> Diamond Light Source Ltd. cannot guarantee that this e-mail or any
> attachments are free from viruses and we cannot accept liability for any
> damage which you may sustain as a result of software viruses which may be
> transmitted in or with the message.
> Diamond Light Source Limited (company no. 4375679). Registered in England
> and Wales with its registered office at Diamond House, Harwell Science
> and Innovation Campus, Didcot, Oxfordshire, OX11 0DE, United Kingdom
> 
> 
>