waveformRecord missing PACT=true?

[mdavidsaver]

epics-base/modules/database/src/std/rec/waveformRecord.c

Lines 139 to 143 in 967846b

	if (!pact && prec->pact)
	return 0;
	prec->udf = FALSE;
	recGblGetTimeStampSimm(prec, prec->simm, &prec->siol);

Is waveformRecord missing a prec->pact = TRUE; before line 142? aaiRecord has one.

epics-base/modules/database/src/std/rec/aaiRecord.c

Lines 169 to 174 in 967846b

	if (!pact && prec->pact)
	return 0;
	prec->pact = TRUE;
	prec->udf = FALSE;
	recGblGetTimeStampSimm(prec, prec->simm, &prec->siol);

[anjohnson]

The line that set PACT in the waveformRecord.c's process() routine does appear to have been lost (commit 23d9176 I believe, mea culpa and I will fix it), but I don't think this is actually as big a problem as it might seem.

Every operation that involves a DB link which might cause a different record to process should be saving PACT and setting it to TRUE before it passes control to that other record. This rule exists because records are allowed to do I/O through links before they call the device support when PACT would still be FALSE. I'm not aware of any other ways in which control can be passed to a different record which might end up looping back to the waveform, but I'd be interested to hear if there are any.

If the I/O was asynchronous then PACT will still be true anyway, so that case is safe.

If the I/O was synchronous, the routines that would get called with PACT incorrectly FALSE are recGblGetTimeStampSimm(), monitor() and recGblFwdLink():

1. The timestamp code may call dbGetTimeStampTag() which delegates to the LSET, but all the implementations of the LSET::getTimeStampTag() or LSET::getTimeStamp() routines just copy data AFAICS.
2. The monitor() routine only calls db_post_events() and even if that ends up triggering a CP link it should get run in the context of a different thread, so it couldn't lock the lockset which the waveform belongs to. (This is the only part I'm not 100% sure about.)
3. Finally recGblFwdLink() calls dbScanFwdLink() which delegates to the LSET again; for a DB link dbDbScanFwdLink() calls dbScanPassive() and its processTarget() routine temporarily sets PACT for the duration of the propagation anyway.

Did I miss anything?

[mdavidsaver]

... I'd be interested to hear if there are any.

The call to recGblGetTimeStampSimm() in turn calls dbGetLink(). dbDbGetValue() doesn't do anything with PACT. Although a self-loop through TSEL is possible, it seems unlikely. So this seems like a minor issue to me.

I happened to notice this omission while giving an introduction to record support, just after pointing out how error prone this almost copy+paste code can be. This omission provided an opportune illustration.

[anjohnson]

Sorry I missed that call to dbGetLink() in recGblGetTimeStampSimm() but it doesn't change the situation at all. If PP is set so it needs to process the target record before fetching its data, dbDbGetValue() calls dbScanPassive() which I already described above as setting PACT while control is passed to the other record. Thus I still don't believe a self-loop is possible through TSEL; the code that handles DB links has been self-protecting against loops for a long time (see my 2nd paragraph above for the general principle which still holds).