Code review comment for ~dirk.zimoch/epics-base:dbChannelForDBLinks

Revision history for this message
Ben Franksen (bfrk) wrote :

I have an additional comment/question about the code in dbDbLink.c, function dbDbGetValue, particularly the following case distinction:

    /* If filters are involved in a read, create field log and run filters */
    if (ellCount(&chan->filters)) {
        db_field_log *pfl;

        /*
         * For the moment, empty arrays are not supported by EPICS.
         * See the remark in src/std/filters/arr.c for details.
         */
        if (dbChannelFinalElements(chan) <= 0) /* empty array request */
            return S_db_badField;
        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);
        db_delete_field_log(pfl);
        if (status)
            return status;
        if (pnRequest && *pnRequest <= 0) /* empty array result */
            return S_db_badField;
    } else if (ppv_link->getCvt && ppv_link->lastGetdbrType == dbrType) {
        status = ppv_link->getCvt(dbChannelField(chan), pbuffer, paddr);
    } else {
        unsigned short dbfType = dbChannelFinalFieldType(chan);

        if (dbrType < 0 || dbrType > DBR_ENUM || dbfType > DBF_DEVICE)
            return S_db_badDbrtype;

        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(dbChannelField(chan), pbuffer, paddr);
        } else {
            ppv_link->getCvt = NULL;
            status = dbGet(paddr, dbrType, pbuffer, NULL, pnRequest, NULL);
        }
        ppv_link->lastGetdbrType = dbrType;
    }

Why handle the first case completely separate from the other two cases? If we have no filters then dbChannelRunPre/PostChain should be a no-op. Furthermore, shouldn't the actions done in the last case (the final "else" branch) also be done in the first case i.e. when we have filters?

The second case ("else if") is clearly an optimization. The code tells me that this optimization is not valid when there are filters, but it is unclear (to me at least) why this is the case.

I think this code should be re-written from scratch, after careful consideration of the expected behavior and possible optimization/caching.

« Back to merge proposal