Experimental Physics and
Industrial Control System

"Johnson, Andrew N." <[email protected]> · Wed, 8 Aug 2018 16:26:30 +0000

Hi Till,

I’m okay with stretching this conversation out, you’re the one who’s asking for a solution...

The dbAddr is designed to hold all the information you want and I don’t mind giving out a pointer to one as long as its lifetime is clear. A callback makes that obvious, whereas the original dbGetPdbAddrFromLink() obscures it.

Your suggestion to add a routine dbGetLinkBufPtr() suffers from the exact same problem, because while the other two dbGet... routines return information that is current at the time of the call, for another link type such as dbCa that could change immediately after they return. In 3.16 or 7.0 we already added another API with a callback which requires the link type to hold its internal mutex for the duration of the callback, so it can fetch data and metadata from the link atomically (e.g. value and timestamp).

Are you looking for this new API to appear in the 3.15 series? We can’t add a new LSET method here because we already added the other one in the later series, but I’d be okay with doing this as a special case for the earlier versions and adding the new LSET method in the later series to preserve backwards compatibility in out link types.

- Andrew

-- 
Sent from my iPad

> On Aug 6, 2018, at 3:14 PM, Till Straumann <[email protected]> wrote:
> 
> Sorry for being silent - I had to attend to other matters...
> 
> Andrew's earlier post
> 
> "My assumption is that they're wanting direct access to a large block of data without copying it..."
> 
> is right on the spot.
> 
> From my point of view either variant
> 
> dbGetPdbAddrFromLink(DBLINK *, DBADDR*);
> 
> doDbAddr(struct link *plink, dbLinkDbAddrCallback rtn,
>        void *priv);
> 
> with either
> 
> typedef long (*dbLinkDbAddrCallback)(const dbAddr *paddr, void *priv);
> 
> or
> 
> typedef long (*dbLinkDbAddrCallback)(void *pBuffer, int dbfType, long nelements, void *priv);
> 
> would be acceptable. If you really want to hide DBADDR then the last
> variant would probably be preferable.
> 
> The very first option (dbGetPdbAddrFromLink)
> would pretty much restore 3.14 which I thought you wanted to avoid?
> 
> I'd also argue that (isn't the number of elements also mutable across
> locked operations ?) since there are already
> 
> long
> dbGetNelements(const struct link*, long *);
> 
> and
> 
> int
> dbGetLinkDBFType(const struct link*);
> 
> that simply adding
> 
> int
> dbGetLinkBufPtr(const struct link *, void **);
> 
> could be enough since the user should know that all the
> results are only valid while he/she holds the lock
> (a respective comment in dbLink.h might be a good idea).
> 
> Thanks for looking into this
> - Till
> 
>> On 08/06/2018 11:26 AM, Michael Davidsaver wrote:
>>> On 08/06/2018 10:01 AM, Andrew Johnson wrote:
>>>> On 08/06/2018 11:15 AM, Michael Davidsaver wrote:
>>>>> On 08/04/2018 08:43 AM, Johnson, Andrew N. wrote:
>>>>> I would be wary of just handing out the dbAddr* pointer though,
>>>>> since the link could get changed without any notice, so I would
>>>>> like to add a little more enforcement. Some link types could even
>>>>> use this to provide access to their raw data buffers, so I’m
>>>>> wondering about an API like doLocked() where the caller gives us a
>>>>> function pointer which we call back with a dbAddr*.
>>>> My thinking has been something like:
>>>> 
>>>>> int dbGetPdbAddrFromLink(DBLINK *plink, DBADDR *pout);
>>>> which would be used like
>>>> 
>>>>> DBLINK *plink = ...;
>>>>> DBADDR addr;
>>>>> dbScanLock(plink->precord);
>>>>> if(dbGetPdbAddrFromLink(plink, &addr)==0) {
>>>>>    // do something with addr
>>>> Though this would look different in the context of an aSub function,
>>>> the point is that this avoids returning a pointer to the internal
>>>> struct dbAddr by making a copy.  This copy remains valid in
>>>> perpetuity, although the assurance that both records are covered by
>>>> the same lock expires as soon as the source record is unlocked.
>>> I don't believe it's legal to call any current dbLink.h function unless
>>> you're already holding the lock for the record that owns that link, so
>>> IMO your dbScanLock() should be superfluous.
>> Yup, I was just being explicit.
>> 
>>> Copying a dbAddr also copies its pfield member pointer. If the target
>>> field of the link is using double-buffering that pointer isn't going to
>>> be valid after the next buffer-switch, but your API doesn't indicate that.
>> True.
>> 
>>> That's why I'd prefer to have the user provide a callback function; it
>>> should be obvious that the information provided is only valid for the
>>> duration of the call (we can also avoid making a copy of the dbAddr):
>>> 
>>> typedef long (*dbLinkDbAddrCallback)(const dbAddr *paddr, void *priv);
>>> long dbLinkDoDbAddr(struct link *plink, dbLinkDbAddrCallback rtn,
>>>         void *priv);
>> I don't see how this is significantly different than copying the dbAddr.
>> As you point out, the complexity comes from handling dbAddr*.  And this
>> can only be avoided by extracting the relevant information.  Of course
>> this requires a better idea of what "relevant" is for Till.
>> 
>> If we want to go the callback route, maybe the better way to handle this
>> is to take the signature of lset::getValue() as a starting point?  Passing
>> a triple of buffer pointer, DBF code, and element count.
>> 
>>> This would call a new lset routine which only DB links would implement;
>>> other link types would just return an error (unless they are willing to
>>> give out access to their local buffer for the link data, in which case
>>> they could create a dbAddr for that buffer).
>> Can we defer talk of an API change until there is a second link type which
>> wants to do this?  DB links are already handled as a special case in other
>> ways (locking).
>> 
>>> In dbDbLink.c the implementation is just:
>>> 
>>> static long doDbAddr(struct link *plink, dbLinkDbAddrCallback rtn,
>>>         void *priv)
>>> {
>>>     DBADDR *paddr = (DBADDR *) plink->value.pv_link.pvt;
>>> 
>>>     return rtn(paddr, priv);
>>> }
>>> 
>>> 
>>> Till, nothing more is likely to happen on this topic without your
>>> feedback, would this API be sufficient to implement what you need?
>> I'm not planning to take any immediate action.
> 

Experimental Physics and Industrial Control System

Experimental Physics and
Industrial Control System