PHAS is not always respected when SCAN=Event

[Bruno Martins]

There is a bug when adding records to an Event scan list. Adding a record to an event scan list with a lower PHAS than any other previously added record will end up putting it at the *end* of the list (not at the beginning). Triggering the bug:

    $ cat test.db
    record(ai, "SECOND") { field(EVNT, "E") field(SCAN, "Event") field(PHAS, "1") }
    record(ai, "FIRST") { field(EVNT, "E") field(SCAN, "Event") field(PHAS, "0") }
    record(ai, "THIRD") { field(EVNT, "E") field(SCAN, "Event") field(PHAS, "2") }

    $ softIoc -d test.db
    Starting iocInit
    ############################################################################
    ## EPICS R3.15.8-1+0~20200608134952.18+debian10~1.gbp3c2d35
    ## EPICS Base built Jun 8 2020
    ############################################################################
    iocRun: All initialization complete
    epics> scanpel
    Event "E"
     Priority Low
        SECOND
        FIRST
        THIRD
    epics>

The expected order is FIRST, SECOND, THIRD. I attached a patch (for 3.15.8) that I think fixes the issue. I found it in 3.15.8 but it seems to affect 7.0 series as well.

[Andrew Johnson (anj)]

Oops! Accepted by inspecting the code, thanks for the fix! Note that this won't just affect SCAN==Event, all the scan types respect PHAS and could be affected.

This definitely needs to be fixed on the 3.15 branch and merged up.

It ought to be possible to combine the two calls to ellInsert() in that routine. Remove the first call completely and the leading if() statement in Bruno's patch and use (ptemp ? &ptemp->node : NULL) for the second argument.

Bruno's patch also fixes a proto-bug in that the second argument to the current ellAdd() call assumes that offsetof(scan_element, node)==0. I thought we'd fixed most of those but we missed that one and in the similar ellDelete() call lower down - replace (void *)pse with &pse->node.

It would be nice to add some tests to dbScanTest.c for this and similar scan-list manipulations, but that exists only on the 7.0 branch so it would have to be done separately. Codeathon task?

[Martin Konrad (info-martin-konrad)]

I can confirm that Bruno's patch fixes the problem. Andrew, thanks for confirming that &pse->node is the way to go here. I agree that this should be fixed in deleteFromList() as well. I'll take a look at combining the two ellInsert() calls.

[Martin Konrad (info-martin-konrad)]

Please see my merge request for Bruno's patch + minor refactoring to get rid of one of the ellInsert() calls.

[Dirk Zimoch (dirk.zimoch)]

For the record: This bug does not exist in 3.14.

[Martin Konrad (info-martin-konrad)]

Correct, this regression has accidentally been introduced while refactoring this function to improve performance https://git.launchpad.net/epics-base/commit/?h=3.15&id=56d5a5935625074632ee1e9c1b9a6f18a4e06294.