Experimental Physics and
Industrial Control System

Review: Needs Fixing

The bulk of this change look like straightforward mechanical replacement of dbAddr with dbChannel.

I see a potential issue though with the handling of db_field_log which will crash if a filter attempts to drop/free the update being processed.

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;
> +            }

So the result of trying to read a scalar from a zero length array is an error?  (I think this is ok, but just want to call attention)

> +
>              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..ff77c3f 100644
> --- a/modules/database/src/ioc/db/dbDbLink.c
> +++ b/modules/database/src/ioc/db/dbDbLink.c
> @@ -160,30 +165,52 @@ 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;
>      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);
> +    /* If filters are involved in a read, create field log and run filters */
> +    if (ellCount(&chan->filters)) {
> +        long options = 0;
> +        db_field_log fl = {};
> +        fl.ctx = dbfl_context_read;
> +        fl.type = dbfl_type_rec;
> +
> +        /* For the moment, empty arrays are not supported by EPICS */
> +        if (dbChannelFinalElements(chan) > 0)
> +        {
> +            dbChannelRunPreChain(chan, &fl);
> +            dbChannelRunPostChain(chan, &fl);

This isn't the correct way to handle db_field_log.   It needs to be allocated with db_create_read_log() and can't be on the stack as plugins are allowed to db_delete_field_log() it.  cf. read_reply() in camessage.c

> +            status = dbChannelGet(chan, dbrType, pbuffer, &options, pnRequest, &fl);
> +            if (!status && pnRequest && *pnRequest <= 0)
> +                status = S_db_badField;
> +        } else {
> +            status = S_db_badField;
> +        }
> +        if (status) {
> +            recGblSetSevr(precord, LINK_ALARM, UDF_ALARM);
> +        }
> +    } else if (ppv_link->getCvt && ppv_link->lastGetdbrType == dbrType) {
> +        status = ppv_link->getCvt(dbChannelField(chan), pbuffer, paddr);
>      } else {
> -        unsigned short dbfType = paddr->field_type;
> +        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) {
> +        if (dbChannelFinalElements(chan) == 1 && (!pnRequest || *pnRequest == 1)
> +                && dbChannelSpecial(chan) != SPC_DBADDR
> +                && dbChannelSpecial(chan) != SPC_ATTRIBUTE) {
>              ppv_link->getCvt = dbFastGetConvertRoutine[dbfType][dbrType];
> -            status = ppv_link->getCvt(paddr->pfield, pbuffer, paddr);
> +            status = ppv_link->getCvt(dbChannelField(chan), pbuffer, paddr);
>          } else {
>              ppv_link->getCvt = NULL;
>              status = dbGet(paddr, dbrType, pbuffer, NULL, pnRequest, NULL);


-- 
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: Jenkins build is back to stable : epics-base-3.15-win64s-test #274 APS Jenkins via Core-talk

Next: [Bug 1398215] Re: Make output records only write on change Joao Paulo Martins 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 Dirk Zimoch 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