All points taken care off. I will push a new version now.
Diff comments:
> diff --git a/modules/database/src/ioc/db/dbAccess.c b/modules/database/src/ioc/db/dbAccess.c
> index 19f6038..9b149dd 100644
> --- a/modules/database/src/ioc/db/dbAccess.c
> +++ b/modules/database/src/ioc/db/dbAccess.c
> @@ -943,6 +943,11 @@ long dbGet(DBADDR *paddr, short dbrType,
> } else {
> DBADDR localAddr = *paddr; /* Structure copy */
>
> + if (pfl->no_elements < 1) {
> + status = S_db_badField;
> + goto done;
> + }
> +
Correct, no_elements is always equal to pfl->no_elements here.
I had added the test in the block where it crashed, but it should be possible to do it earlier, costing a few ticks maybe. Oops, When doing so, dbCaLinkTest hangs.
I am a bit reluctant to add new error codes but I have added S_db_emptyArray now.
Complexity comes for free. Simplicity is what one has to struggle for. I hope we can do something about this some day.
> localAddr.field_type = pfl->field_type;
> localAddr.field_size = pfl->field_size;
> localAddr.no_elements = pfl->no_elements;
> diff --git a/modules/database/src/ioc/db/dbDbLink.c b/modules/database/src/ioc/db/dbDbLink.c
> index f2cb0c5..688cb7a 100644
> --- a/modules/database/src/ioc/db/dbDbLink.c
> +++ b/modules/database/src/ioc/db/dbDbLink.c
> @@ -160,47 +165,82 @@ static long dbDbGetValue(struct link *plink, short dbrType, void *pbuffer,
> long *pnRequest)
> {
> struct pv_link *ppv_link = &plink->value.pv_link;
> - DBADDR *paddr = ppv_link->pvt;
> + dbChannel *chan = linkChannel(plink);
> + DBADDR *paddr = &chan->addr;
> dbCommon *precord = plink->precord;
> + db_field_log *pfl = NULL;
> long status;
>
> /* scan passive records if link is process passive */
> if (ppv_link->pvlMask & pvlOptPP) {
> - status = dbScanPassive(precord, paddr->precord);
> + status = dbScanPassive(precord, dbChannelRecord(chan));
> if (status)
> return status;
> }
>
> - if (ppv_link->getCvt && ppv_link->lastGetdbrType == dbrType) {
> - status = ppv_link->getCvt(paddr->pfield, pbuffer, paddr);
> - } else {
> - unsigned short dbfType = paddr->field_type;
> + if (ppv_link->getCvt && ppv_link->lastGetdbrType == dbrType)
> + {
> + /* shortcut: scalar with known conversion, no filter */
> + status = ppv_link->getCvt(dbChannelField(chan), pbuffer, paddr);
> + }
> + else if (dbChannelFinalElements(chan) == 1 && (!pnRequest || *pnRequest == 1)
> + && dbChannelSpecial(chan) != SPC_DBADDR
> + && dbChannelSpecial(chan) != SPC_ATTRIBUTE
> + && ellCount(&chan->filters) == 0)
> + {
> + /* simple scalar: set up shortcut */
> + unsigned short dbfType = dbChannelFinalFieldType(chan);
>
> if (dbrType < 0 || dbrType > DBR_ENUM || dbfType > DBF_DEVICE)
> return S_db_badDbrtype;
>
> - if (paddr->no_elements == 1 && (!pnRequest || *pnRequest == 1)
> - && paddr->special != SPC_DBADDR
> - && paddr->special != SPC_ATTRIBUTE) {
> - ppv_link->getCvt = dbFastGetConvertRoutine[dbfType][dbrType];
> - status = ppv_link->getCvt(paddr->pfield, pbuffer, paddr);
> - } else {
> - ppv_link->getCvt = NULL;
> - status = dbGet(paddr, dbrType, pbuffer, NULL, pnRequest, NULL);
> - }
> + ppv_link->getCvt = dbFastGetConvertRoutine[dbfType][dbrType];
> ppv_link->lastGetdbrType = dbrType;
> + status = ppv_link->getCvt(dbChannelField(chan), pbuffer, paddr);
The salar path is only taken if *pnRequest is already 1. Why set it again?
The shortcut is only taken when the salar path was taken before, so again *pnRequest is already 1.
> + }
> + else
> + {
> + /* filter, array, or special */
> + ppv_link->getCvt = NULL;
> +
> + /* For the moment, empty arrays are not supported by EPICS */
> + if (dbChannelFinalElements(chan) <= 0) /* empty array request */
> + return S_db_badField;
Is it guaranteed? I have removed the test now.
> +
> + if (ellCount(&chan->filters)) {
> + /* If filters are involved in a read, create field log and run filters */
> + pfl = db_create_read_log(chan);
> + if (!pfl)
> + return S_db_noMemory;
> +
> + pfl = dbChannelRunPreChain(chan, pfl);
> + pfl = dbChannelRunPostChain(chan, pfl);
> + }
> +
> + status = dbChannelGet(chan, dbrType, pbuffer, NULL, pnRequest, pfl);
> +
> + if (pfl)
> + db_delete_field_log(pfl);
> +
> + if (status)
> + return status;
> +
> + if (pnRequest && *pnRequest <= 0) /* empty array result */
> + return S_db_badField;
> }
>
> - if (!status && precord != paddr->precord)
> + if (!status && precord != dbChannelRecord(chan))
> recGblInheritSevr(plink->value.pv_link.pvlMask & pvlOptMsMode,
> - plink->precord, paddr->precord->stat, paddr->precord->sevr);
> + plink->precord,
> + dbChannelRecord(chan)->stat, dbChannelRecord(chan)->sevr);
> return status;
> }
>
> static long dbDbGetControlLimits(const struct link *plink, double *low,
> double *high)
> {
> - DBADDR *paddr = (DBADDR *) plink->value.pv_link.pvt;
> + dbChannel *chan = linkChannel(plink);
> + DBADDR *paddr = &chan->addr;
> struct buffer {
> DBRctrlDouble
> double value;
> diff --git a/modules/database/src/ioc/db/dbEvent.h b/modules/database/src/ioc/db/dbEvent.h
> index 374e849..175b0e4 100644
> --- a/modules/database/src/ioc/db/dbEvent.h
> +++ b/modules/database/src/ioc/db/dbEvent.h
> @@ -50,6 +50,7 @@ epicsShareFunc int db_post_events (
> typedef void * dbEventCtx;
>
> typedef void EXTRALABORFUNC (void *extralabor_arg);
> +void db_init_event_freelists (void);
Done. Does it make sense to have xxxPriv.h header files for such private APIs?
> epicsShareFunc dbEventCtx db_init_events (void);
> epicsShareFunc int db_start_events (
> dbEventCtx ctx, const char *taskname, void (*init_func)(void *),
> diff --git a/modules/database/test/std/rec/linkFilterTest.c b/modules/database/test/std/rec/linkFilterTest.c
> new file mode 100644
> index 0000000..6f38d24
> --- /dev/null
> +++ b/modules/database/test/std/rec/linkFilterTest.c
> @@ -0,0 +1,157 @@
> +/*************************************************************************\
> +* Copyright (c) 2020 Dirk Zimoch
> +* EPICS BASE is distributed subject to a Software License Agreement found
> +* in file LICENSE that is included with this distribution.
> +\*************************************************************************/
> +
> +#include <string.h>
> +
> +#include "dbAccess.h"
> +#include "devSup.h"
> +#include "alarm.h"
> +#include "dbUnitTest.h"
> +#include "errlog.h"
> +#include "epicsThread.h"
> +
> +#include "longinRecord.h"
> +
> +#include "testMain.h"
> +
> +void recTestIoc_registerRecordDeviceDriver(struct dbBase *);
> +
> +static void startTestIoc(const char *dbfile)
> +{
> + testdbPrepare();
> + testdbReadDatabase("recTestIoc.dbd", NULL, NULL);
> + recTestIoc_registerRecordDeviceDriver(pdbbase);
> + testdbReadDatabase(dbfile, NULL, NULL);
> +
> + eltc(0);
> + testIocInitOk();
> + eltc(1);
> +}
> +
> +static void expectProcSuccess(const char *rec)
> +{
> + char fieldname[20];
> + testDiag("expecting success from %s", rec);
> + sprintf(fieldname, "%s.PROC", rec);
> + testdbPutFieldOk(fieldname, DBF_LONG, 1);
> + sprintf(fieldname, "%s.SEVR", rec);
> + testdbGetFieldEqual(fieldname, DBF_LONG, NO_ALARM);
> + sprintf(fieldname, "%s.STAT", rec);
> + testdbGetFieldEqual(fieldname, DBF_LONG, NO_ALARM);
> +}
> +
> +static void expectProcFailure(const char *rec)
> +{
> + char fieldname[20];
> + testDiag("expecting failure S_db_badField %#x from %s", S_db_badField, rec);
> + sprintf(fieldname, "%s.PROC", rec);
> + testdbPutFieldFail(S_db_badField, fieldname, DBF_LONG, 1);
> + sprintf(fieldname, "%s.SEVR", rec);
> + testdbGetFieldEqual(fieldname, DBF_LONG, INVALID_ALARM);
> + sprintf(fieldname, "%s.STAT", rec);
> + testdbGetFieldEqual(fieldname, DBF_LONG, LINK_ALARM);
> +}
> +
> +static void changeRange(long start, long stop, long step)
> +{
> + char linkstring[60];
> + if (step)
> + sprintf(linkstring, "src.[%ld:%ld:%ld]", start, step, stop);
> + else if (stop)
> + sprintf(linkstring, "src.[%ld:%ld]", start, stop);
> + else
> + sprintf(linkstring, "src.[%ld]", start);
> + testDiag("modifying link: %s", linkstring);
> + testdbPutFieldOk("ai.INP", DBF_STRING, linkstring);
> + testdbPutFieldOk("wf.INP", DBF_STRING, linkstring);
> +}
> +
> +static double buf[] = {1, 2, 3, 4, 5, 6, 7, 8, 0, 0, 2, 4, 6};
Yes. But does it really matter? It is test code. Done anyway.
> +
> +static void expectRange(long start, long end)
> +{
> + long n = end-start+1;
> + testdbGetFieldEqual("ai.VAL", DBF_DOUBLE, buf[start]);
> + testdbGetFieldEqual("wf.NORD", DBF_LONG, n);
> + testdbGetArrFieldEqual("wf.VAL", DBF_DOUBLE, n+2, n, buf+start);
> +}
> +
> +#if 0
> +static void expectEmptyArray(void)
> +{
> + /* empty arrays are now allowed at the moment */
> + testDiag("expecting empty array");
> + testdbGetFieldEqual("wf.NORD", DBF_LONG, 0);
> +}
> +#endif
Done.
> +
> +MAIN(linkFilterTest)
> +{
> + testPlan(98);
> + startTestIoc("linkFilterTest.db");
> +
> + testDiag("PINI");
> + expectRange(2,4);
> +
> + testDiag("modify range");
> + changeRange(3,6,0);
> + expectProcSuccess("ai");
> + expectProcSuccess("wf");
> + expectRange(3,6);
> +
> + testDiag("backward range");
> + changeRange(5,3,0);
> + expectProcFailure("ai");
> + expectProcFailure("wf");
> +
> + testDiag("step 2");
> + changeRange(1,6,2);
> + expectProcSuccess("ai");
> + expectProcSuccess("wf");
> + expectRange(10,12);
> +
> + testDiag("range start beyond src.NORD");
> + changeRange(8,9,0);
> + expectProcFailure("ai");
> + expectProcFailure("wf");
> +
> + testDiag("range end beyond src.NORD");
> + changeRange(3,9,0);
> + expectProcSuccess("ai");
> + expectProcSuccess("wf");
> + expectRange(3,7); /* clipped range */
> +
> + testDiag("range start beyond src.NELM");
> + changeRange(11,12,0);
> + expectProcFailure("ai");
> + expectProcFailure("wf");
> +
> + testDiag("range end beyond src.NELM");
> + changeRange(4,12,0);
> + expectProcSuccess("ai");
> + expectProcSuccess("wf");
> + expectRange(4,7); /* clipped range */
> +
> + testDiag("single value beyond src.NORD");
> + changeRange(8,0,0);
> + expectProcFailure("ai");
> + expectProcFailure("wf");
> +
> + testDiag("single value");
> + changeRange(5,0,0);
> + expectProcSuccess("ai");
> + expectProcSuccess("wf");
> + expectRange(5,5);
> +
> + testDiag("single beyond rec.NELM");
> + changeRange(12,0,0);
> + expectProcFailure("ai");
> + expectProcFailure("wf");
> +
> + testIocShutdownOk();
> + testdbCleanup();
> + return testDone();
> +}
--
https://code.launchpad.net/~dirk.zimoch/epics-base/+git/epics-base/+merge/378968
Your team EPICS Core Developers is subscribed to branch epics-base:7.0.
- References:
- [Merge] ~dirk.zimoch/epics-base:dbChannelForDBLinks into epics-base:7.0 Dirk Zimoch via Core-talk
- Navigate by Date:
- Prev:
Build failed: epics-base base-integration-483 AppVeyor via Core-talk
- Next:
Build completed: epics-base base-defaultMessageQueue-21 AppVeyor via Core-talk
- Index:
2002
2003
2004
2005
2006
2007
2008
2009
2010
2011
2012
2013
2014
2015
2016
2017
2018
2019
<2020>
2021
2022
2023
2024
- Navigate by Thread:
- Prev:
Re: [Merge] ~dirk.zimoch/epics-base:dbChannelForDBLinks into epics-base:7.0 Dirk Zimoch via Core-talk
- Next:
Re: [Merge] ~dirk.zimoch/epics-base:dbChannelForDBLinks into epics-base:7.0 mdavidsaver via Core-talk
- Index:
2002
2003
2004
2005
2006
2007
2008
2009
2010
2011
2012
2013
2014
2015
2016
2017
2018
2019
<2020>
2021
2022
2023
2024
|
·
Home
·
News
·
About
·
Base
·
Modules
·
Extensions
·
Distributions
·
Download
·