Re: Concerns about link-support-2 branch

[Andrew Johnson <anj@aps.anl.gov>]

Hi Michael,

On 03/07/2017 08:57 AM, Michael Davidsaver wrote:
> Maybe I was rambling too much.  Let me be clear.  I see no regression in
> this change.

Ok, *now* I think I understand what you're after.

> I don't like the existing behavior of the dbGet*() calls.  I've realized
> that the code I've been writing is open to race conditions w/ CA_LINK.
> I want to see this "fixed".  I agree that fixing dbCa.c is out of scope
> for your branch.  However, I think that such a fix will be complicated
> by introducing another layer of public API.

Maybe not.

Does the attached patch add sufficient functionality for your needs? I
haven't tested it yet, but I think the basic functionality is right.

You pass a callback routine to dbLinkDoLocked(plink, ...) and (assuming
the new lset::doLocked() method is supported by the link type) your
routine then gets called while any mutex associated with the link is
locked. dbLinkDoLocked() also passes down a void *private pointer for
context information, and the callback may then make multiple calls to
dbLink.h routines against this link while that mutex is locked. The
return status gets passed back up to the original caller. Note that
disconnected dbCa links will return -1 without calling the routine.

I prefer using a callback to providing direct access to a link's
lock/unlock capabilities, it's harder for user code to leave a link
locked this way. Link types that provide an lset::doLocked() method must
use a recursive mutex for locking.

- Andrew

-- 
Arguing for surveillance because you have nothing to hide is no
different than making the claim, "I don't care about freedom of
speech because I have nothing to say." -- Edward Snowdon

=== modified file 'src/ioc/db/dbLink.h'
--- src/ioc/db/dbLink.h	2016-09-04 00:21:11 +0000
+++ src/ioc/db/dbLink.h	2017-03-08 21:35:45 +0000
@@ -27,6 +27,8 @@
 
 struct dbLocker;
 
+typedef long (*dbLinkUserCallback)(struct link *plink, void *priv);
+
 typedef struct lset {
     /* Characteristics of the link type */
     const unsigned isConstant:1;
@@ -71,6 +73,9 @@
 
     /* Process */
     void (*scanForward)(struct link *plink);
+
+    /* Atomicity */
+    long (*doLocked)(struct link *plink, dbLinkUserCallback rtn, void *priv);
 } lset;
 
 #define dbGetSevr(link, sevr) \
@@ -117,6 +122,9 @@
         const void *pbuffer, long nRequest);
 epicsShareFunc void dbScanFwdLink(struct link *plink);
 
+epicsShareFunc void dbLinkDoLocked(struct link *plink, dbLinkUserCallback rtn,
+        void *priv);
+
 epicsShareFunc long dbLoadLinkLS(struct link *plink, char *pbuffer,
         epicsUInt32 size, epicsUInt32 *plen);
 epicsShareFunc long dbGetLinkLS(struct link *plink, char *pbuffer,

=== modified file 'src/ioc/db/dbLink.c'
--- src/ioc/db/dbLink.c	2016-09-07 05:47:51 +0000
+++ src/ioc/db/dbLink.c	2017-03-08 21:45:18 +0000
@@ -414,6 +414,18 @@
         plset->scanForward(plink);
 }
 
+long dbLinkDoLocked(struct link *plink, dbLinkUserCallback rtn,
+        void *priv)
+{
+    lset *plset = plink->lset;
+
+    if (!rtn || !plset || !plset->doLocked)
+        return S_db_noLSET;
+
+    return plset->doLocked(plink, rtn, priv);
+}
+
+
 /* Helper functions for long string support */
 
 long dbGetLinkLS(struct link *plink, char *pbuffer, epicsUInt32 size,

=== modified file 'src/ioc/db/dbCa.c'
--- src/ioc/db/dbCa.c	2016-09-04 00:21:11 +0000
+++ src/ioc/db/dbCa.c	2017-03-08 21:46:49 +0000
@@ -684,6 +684,17 @@
     return gotAttributes ? 0 : -1;
 }
 
+static long doLocked(struct link *plink, dbLinkUserCallback rtn, void *priv)
+{
+    caLink *pca;
+    long status;
+
+    pcaGetCheck
+    status = rtn(plink, priv);
+    epicsMutexUnlock(pca->lock);
+    return status;
+}
+
 static void scanComplete(void *raw, dbCommon *prec)
 {
     caLink *pca = raw;
@@ -727,7 +738,7 @@
     getPrecision, getUnits,
     getAlarm, getTimeStamp,
     dbCaPutLink, dbCaPutAsync,
-    scanForward
+    scanForward, doLocked
 };
 
 static void connectionCallback(struct connection_handler_args arg)

=== modified file 'src/ioc/db/dbDbLink.c'
--- src/ioc/db/dbDbLink.c	2016-09-04 00:21:11 +0000
+++ src/ioc/db/dbDbLink.c	2017-03-08 21:48:52 +0000
@@ -337,6 +337,11 @@
     dbScanPassive(precord, paddr->precord);
 }
 
+static long doLocked(struct link *plink, dbLinkUserCallback rtn, void *priv)
+{
+    return rtn(plink, priv);
+}
+
 static lset dbDb_lset = {
     0, 0, /* not Constant, not Volatile */
     NULL, dbDbRemoveLink,
@@ -348,6 +353,5 @@
     dbDbGetPrecision, dbDbGetUnits,
     dbDbGetAlarm, dbDbGetTimeStamp,
     dbDbPutValue, NULL,
-    dbDbScanFwdLink
+    dbDbScanFwdLink, doLocked
 };
-

=== modified file 'src/ioc/db/dbConstLink.c'
--- src/ioc/db/dbConstLink.c	2016-09-04 00:21:11 +0000
+++ src/ioc/db/dbConstLink.c	2017-03-08 21:50:15 +0000
@@ -135,6 +135,5 @@
     NULL, NULL,
     NULL, NULL,
     NULL, NULL,
-    NULL
+    NULL, NULL
 };
-

=== modified file 'src/std/link/lnkCalc.c'
--- src/std/link/lnkCalc.c	2016-09-07 03:28:36 +0000
+++ src/std/link/lnkCalc.c	2017-03-08 21:52:50 +0000
@@ -597,6 +597,11 @@
     return 0;
 }
 
+static long doLocked(struct link *plink, dbLinkUserCallback rtn, void *priv)
+{
+    return rtn(plink, priv);
+}
+
 
 /************************* Interface Tables *************************/
 
@@ -610,7 +615,7 @@
     lnkCalc_getPrecision, lnkCalc_getUnits,
     lnkCalc_getAlarm, NULL,
     NULL, NULL,
-    NULL
+    NULL, doLocked
 };
 
 static jlif lnkCalcIf = {

=== modified file 'src/std/link/lnkConst.c'
--- src/std/link/lnkConst.c	2016-09-05 19:25:33 +0000
+++ src/std/link/lnkConst.c	2017-03-08 21:51:44 +0000
@@ -551,7 +551,7 @@
     NULL, NULL,
     NULL, NULL,
     NULL, NULL,
-    NULL
+    NULL, NULL
 };
 
 static jlif lnkConstIf = {

=== modified file 'src/ioc/db/test/jlinkz.c'
--- src/ioc/db/test/jlinkz.c	2017-03-03 18:23:36 +0000
+++ src/ioc/db/test/jlinkz.c	2017-03-08 22:38:30 +0000
@@ -146,6 +146,7 @@
     &z_putval,
     NULL, /* putasync */
     NULL, /* forward */
+    NULL, /* doLocked */
 };
 
 static

=== modified file 'src/ioc/db/test/xLink.c'
--- src/ioc/db/test/xLink.c	2016-09-07 05:50:40 +0000
+++ src/ioc/db/test/xLink.c	2017-03-08 22:38:53 +0000
@@ -73,7 +73,7 @@
     NULL, NULL,
     NULL, NULL,
     NULL, NULL,
-    NULL
+    NULL, NULL
 };
 
 static jlif xlinkIf = {