Merge ~dirk.zimoch/epics-base:info_fields into ~epics-core/epics-base/+git/epics-base:3.16

[Dirk Zimoch]

Allows read access to info fields from Channel Access and EPICS database.
Provides a function to list info fields, optionally filtering for field names.

[Andrew Johnson (anj)]

I wish you hadn't combined these two different features into one merge request. I'm Okay with adding iocsh command(s) to find and list info items, but something in me doesn't like the idea of making items visible over the network, so I'm going to have to be persuaded to accept that part.

There are several reasons that come to mind:

* Field and attribute names must be valid C identifiers; info item names have no such restrictions, and an informal standard has developed that suggests they should take the form "<subsystem>:<name>", e.g. "asyn:READBACK", "asyn:FIFO", "Q:group", but nothing enforces this.
* Searches for info items are linear, so slow (searches for record names use a hash table, field names use a binary search; admittedly attribute name searches are linear).
* Sites would have to add support for info items to their IRMIS/Channel Finder/CA Name Server/PV Database etc. or they risk not being able to know for sure whether a specific channel name exists or not.
* Up to now info items have been private to the IOC, so could be used to hold semi-secret information.

[Ralph Lange (ralph-lange)]

I'm with Andrew on this.

Publishing something on the network that was explicitly and documented IOC private ... that would be a major change for info items.

Admittedly there has been more than one moment where I wished these items were visible across CA. They often carry information that might be important when debugging, and IOC consoles are often not easily accessible (at least not in production systems).

Maybe we should rather introduce a second type of info item (pubinfo?) that is known to be accessible through CA (read-only? read/write?) and might have more restrictive naming rules.

Hm. That would be half the way to supporting by-instance fields in records, wouldn't it?

[Dirk Zimoch (dirk.zimoch)]

> I wish you hadn't combined these two different features into one merge
> request. I'm Okay with adding iocsh command(s) to find and list info items,
> but something in me doesn't like the idea of making items visible over the
> network, so I'm going to have to be persuaded to accept that part.

No problem. I can split it.

> There are several reasons that come to mind:
>
> * Field and attribute names must be valid C identifiers; info item names have
> no such restrictions, and an informal standard has developed that suggests
> they should take the form "<subsystem>:<name>", e.g. "asyn:READBACK",
> "asyn:FIFO", "Q:group", but nothing enforces this.

At the moment almost everthing except [{$ should work. Spaces probably won‘t work too. I need to test periods.

> * Searches for info items are linear, so slow (searches for record names use a
> hash table, field names use a binary search; admittedly attribute name
> searches are linear).

The search is done only once at connection time. And I don‘t think that records have a lot of info fields (at the moment). But I can probably optimize if this is an issue.

> * Sites would have to add support for info items to their IRMIS/Channel
> Finder/CA Name Server/PV Database etc. or they risk not being able to know for
> sure whether a specific channel name exists or not.

Yes, if they want to have them „public“.

> * Up to now info items have been private to the IOC, so could be used to hold
> semi-secret information.

Security by obscurity? I would rather consider a ca security solution.

I had primalrily remote debugging in mind.

[Dirk Zimoch (dirk.zimoch)]

> I'm with Andrew on this.
>
> Publishing something on the network that was explicitly and documented IOC
> private ... that would be a major change for info items.

Yes, it is. It is a new debug feature. But I understand the security concerns.
What about a contol variable?

Note that info fields cannot shadow real fields.

>
> Admittedly there has been more than one moment where I wished these items were
> visible across CA. They often carry information that might be important when
> debugging, and IOC consoles are often not easily accessible (at least not in
> production systems).

That was the idea.

> Maybe we should rather introduce a second type of info item (pubinfo?) that is
> known to be accessible through CA (read-only? read/write?) and might have more
> restrictive naming rules.

I don‘t agree. The goal was to read the info for debugging.
I will see what‘s possible with channel access secutity.

> Hm. That would be half the way to supporting by-instance fields in records,
> wouldn't it?

<evil-grin> Exactly what I was thinking. </evil-grin>

[Andrew Johnson (anj)]

https://epics.anl.gov/base/R3-16/1-docs/AppDevGuide/DatabaseDefinition.html#x7-2620006.5 says about record info items that they "can be retrieved by programs running on the IOC (they are not visible via Channel Access at all)."

What would per-instance fields be used for, and could they be read-write? Should they support monitor updates? Do server-side filters work with them? Are they string only, long-string only, or allow other data types too? What are the use cases which we can't easily handle another way?

I don't think we should morph info items into per-instance fields without considering the above questions first, so I'm inclined to reject (that part of) this proposal for now.

I'm also concerned about the field namespace getting filled up (it's already pretty busy). I note though that there are no records in Base or synApps that use the field name 'info', and I see no reason why we couldn't introduce hierarchy into the field name, so with Ralph's suggestion we might have channel names for public info items like "<record>.info.<name>".

If we're going to do that though I would want the "info" part to be another API that allows registration, but at what level (e.g. global, i.e. all records, or per record-type, or per record instance)? Lots to discuss...

[Dirk Zimoch (dirk.zimoch)]

Info items are not per-instance fields. They are constant read-only strings. But this show the way how per-instance fields can be integrated in the future. Real per-instance fields do indeed need proper design and much discussion. I think they are off topic here.

I will split off the CA part of this proposal fow now.

[Dirk Zimoch (dirk.zimoch)]

> https://epics.anl.gov/base/R3-16/1-docs/AppDevGuide/DatabaseDefinition.html#x7
> -2620006.5 says about record info items that they "can be retrieved by
> programs running on the IOC (they are not visible via Channel Access at all)."
>
I am willing to modify that documentation as well :-)

[Ralph Lange (ralph-lange)]

So, one step back...

How about a global variable that controls CA visibility of info items and is set to "invisible" by default.

That ensures compatibility, and any installation that wishes to make things visible can do so by setting the variable from the startup script.

That could go into 3.16. EPICS 7 (as a major step) might even switch the default to "visible", not sure.

I do like the "<record>.info.<name>" notation to keep the namespace clean. and I would leave "info" lowercase in this context, because locally developed record types probably used uppercase letters if they defines a field named "INFO".

Read only, string only, no monitors. Would need support for string and long-string. No access to the private pointer.

[Dirk Zimoch (dirk.zimoch)]

Yes, one step back. I pushed a previous version without CA.

Implementing a control variable would be easy. Maybe 3-state: no access, readonly, write.

Are info items mutable? Should they be? There is dbPutInfo but it is not called at run time (yet).

Long string support worked already. No access to private pointer because CA cannot do that and a pointer has no meaning outside the same address space.

I do not like the idea of .info. so much. But it would be easy enough to implement. Basically any separator starting with . would do, e.g. "<record>..<name>" or "<record>.%<name>" or "<record>.#<name>".

[Ralph Lange (ralph-lange)]

Out of your separator options, I like "<record>..<name>" best.

[Dirk Zimoch (dirk.zimoch)]

In 3.14 any char after the . that is not isalpha() or '_' results in the VAL field being delivered by channel access, dbgf and links. (In 3.15 and 13.16 this is still the case for dbgf and links but not for channel access any more.)

Thus unfortunately <record>..<name> and similar break backward compatibility. An old 3.14 ioc that does not support ca on info items would deliver something wrong (VAL) instead of nothing. The same for any spelling error like <record>...<name>. So the first char after the . must be a letter or underscore.
This also means that my initial idea was not backward compatible because the info name can start with anything, not necessarily a letter.

So I have tested .info. and it works. Only using non-ASCII chars in the name, such as German umlauts result in the expected problems if character encoding of ioc and client don't match. If they match however everything is fine.

To allow selective merging, this will go to a different branch. But I change dbli to use .info. in its output.

71ba16e... by Dirk Zimoch on 2018-03-09

use <record>.info.<name> for info items

[Andrew Johnson (anj)]

We should move discussion of making info items remotely accessible to core-talk, to widen the audience. Please limit further discussion here to the remaining 'dbli' changes.

I committed a streamlined bug-fix for lp: #1754298 to the 3.15 branch, which will conflict with your fix here but I'll clean that up when I merge the branch.

[Andrew Johnson (anj)]

There is a bug in dbNextMatchingInfo(), see comments inline.

How often do you actually use the multiple patterns feature of the dbli command? Is it really that useful, or did you just implement it because you could? None of the other iocsh commands such as dbgrep and dbla support multiple pattern arguments, why is this different?

review: Needs Fixing

[Andrew Johnson (anj)]

Okay, so Launchpad is offsetting my comments in the email copy at least. When I said "doing this check just once" I was talking about the line

    if (!patternlist || !*patternlist) return 0;

d59426f... by Dirk Zimoch on 2018-03-12

bugfix: dbNextMatchingInfo did not find infos in the first record type

2e1a45e... by Dirk Zimoch on 2018-03-12

In vxWorks dbli function don't rely on NULL as last argument. Use fixed 12 arguments instead

932f9d9... by Dirk Zimoch on 2018-03-12

Make clear that patternlist argument of dbNextMatchingInfo must be NULL terminated

3356729... by Dirk Zimoch on 2018-03-12

merge with 3.16 branch

704fd02... by Dirk Zimoch on 2018-03-12

typo

[Andrew Johnson (anj)]

Core Meeting: We need convincing why you need more than one pattern, only supporting one would simplify the code. If you have a use-case for multiple patterns please explain. We don't like the 12-arg thing (which should be inside #ifdef vxWorks to make that dependency obvious).

review: Needs Fixing

[Dirk Zimoch (dirk.zimoch)]

I'm pretty sure I had a use case but I forgot what it was.
I will change it and simplify the code.

4b2e159... by Dirk Zimoch on 2018-05-22

simplify 'dbli' by accepting only one pattern

[Andrew Johnson (anj)]

Merging, with a change to the output formatting from this:

epics> dbli
anj:ai1.info.name1 "value1"
anj:ai1.info.name2 "value2"
anj:ai1.info.name3 "value3" 0x1c0f5e8

to this:

epics> dbli
anj:ai1 info(name1, "value1")
anj:ai1 info(name2, "value2")
anj:ai1 info(name3, "value3", 0x1c0f5e8)

review: Approve

diff --git a/documentation/RELEASE_NOTES.html b/documentation/RELEASE_NOTES.html
index a4a09c7..28f3a8b 100644
--- a/documentation/RELEASE_NOTES.html
+++ b/documentation/RELEASE_NOTES.html
@@ -17,6 +17,11 @@
 
 -->
 
+<h3>Access to info fields</h3>
+
+<p>The new command line function <code>dbli</code> lists and filters info fields.
+The new API function <code>dbNextMatchingInfo</code> iterates over info fields.</p>
+
 <h3>Restore use of ledlib for VxWorks command editing</h3>
 
 <p>The epicsReadline refactoring work described below unfortunately disabled the
diff --git a/src/ioc/db/dbIocRegister.c b/src/ioc/db/dbIocRegister.c
index 07c9836..4d0b88c 100644
--- a/src/ioc/db/dbIocRegister.c
+++ b/src/ioc/db/dbIocRegister.c
@@ -152,6 +152,12 @@ static const iocshArg * const dbnrArgs[1] = {&dbnrArg0};
 static const iocshFuncDef dbnrFuncDef = {"dbnr",1,dbnrArgs};
 static void dbnrCallFunc(const iocshArgBuf *args) { dbnr(args[0].ival);}
 
+/* dbli */
+static const iocshArg dbliArg0 = { "pattern",iocshArgString};
+static const iocshArg * const dbliArgs[1] = {&dbliArg0};
+static const iocshFuncDef dbliFuncDef = {"dbli",1,dbliArgs};
+static void dbliCallFunc(const iocshArgBuf *args) { dbli(args[0].sval);}
+
 /* dbla */
 static const iocshArg dblaArg0 = { "pattern",iocshArgString};
 static const iocshArg * const dblaArgs[1] = {&dblaArg0};
@@ -415,6 +421,7 @@ void dbIocRegister(void)
     iocshRegister(&dblFuncDef,dblCallFunc);
     iocshRegister(&dbnrFuncDef,dbnrCallFunc);
     iocshRegister(&dblaFuncDef,dblaCallFunc);
+    iocshRegister(&dbliFuncDef,dbliCallFunc);
     iocshRegister(&dbgrepFuncDef,dbgrepCallFunc);
     iocshRegister(&dbgfFuncDef,dbgfCallFunc);
     iocshRegister(&dbpfFuncDef,dbpfCallFunc);
diff --git a/src/ioc/db/dbTest.c b/src/ioc/db/dbTest.c
index 4bbd406..2f8d406 100644
--- a/src/ioc/db/dbTest.c
+++ b/src/ioc/db/dbTest.c
@@ -257,7 +257,28 @@ long dbla(const char *pmask)
     dbFinishEntry(pdbentry);
     return 0;
 }
+

+long dbli(const char *pattern)
+{
+    DBENTRY dbentry;
+    void* ptr;
+
+    if (!pdbbase) {
+        printf("No database loaded\n");
+        return 0;
+    }
 
+    dbInitEntry(pdbbase, &dbentry);
+    while (dbNextMatchingInfo(&dbentry, pattern) == 0)
+    {
+        printf("%s.info.%s \"%s\"", dbGetRecordName(&dbentry), dbGetInfoName(&dbentry), dbGetInfoString(&dbentry));
+        if ((ptr = dbGetInfoPointer(&dbentry)) != NULL) printf(" %p", ptr);
+        printf("\n");
+    }
+    dbFinishEntry(&dbentry);
+    return 0;
+}
+

 long dbgrep(const char *pmask)
 {
     DBENTRY dbentry;
diff --git a/src/ioc/db/dbTest.h b/src/ioc/db/dbTest.h
index 6d457b5..a4d90e2 100644
--- a/src/ioc/db/dbTest.h
+++ b/src/ioc/db/dbTest.h
@@ -25,6 +25,8 @@ epicsShareFunc long dbl(
 epicsShareFunc long dbnr(int verbose);
 /* list aliases */
 epicsShareFunc long dbla(const char *pmask);
+/* list infos */
+epicsShareFunc long dbli(const char *patern);
 /*list records with mask*/
 epicsShareFunc long dbgrep(const char *pmask);
 /*get field value*/
diff --git a/src/ioc/db/test/dbStaticTest.db b/src/ioc/db/test/dbStaticTest.db
index c35ff9a..6303672 100644
--- a/src/ioc/db/test/dbStaticTest.db
+++ b/src/ioc/db/test/dbStaticTest.db
@@ -1,7 +1,7 @@
 
 record(x, "testrec") {
-    info("A", "B")
     alias("testalias")
+    info("A", "B")
 }
 
 alias("testrec", "testalias2")
diff --git a/src/ioc/dbStatic/dbStaticLib.c b/src/ioc/dbStatic/dbStaticLib.c
index 4135d7f..a43b49e 100644
--- a/src/ioc/dbStatic/dbStaticLib.c
+++ b/src/ioc/dbStatic/dbStaticLib.c
@@ -2716,6 +2716,32 @@ long dbNextInfo(DBENTRY *pdbentry)
     return (pinfo ? 0 : S_dbLib_infoNotFound);
 }
 
+long dbNextMatchingInfo(DBENTRY *pdbentry, const char *pattern)
+{
+    long status;
+
+    if (!pdbentry->precordType)
+    {
+        status = dbFirstRecordType(pdbentry);
+        goto first;
+    }
+    while(1) {
+        status = dbNextInfo(pdbentry);
+        while (status) {
+            status = dbNextRecord(pdbentry);
+            while (status) {
+                status = dbNextRecordType(pdbentry);
+first:
+                if (status) return status;
+                status = dbFirstRecord(pdbentry);
+            }
+            status = dbFirstInfo(pdbentry); 
+        }
+        if (!pattern || !*pattern) return 0;
+        if (epicsStrGlobMatch(dbGetInfoName(pdbentry), pattern)) return 0;
+    }
+}
+
 long dbFindInfo(DBENTRY *pdbentry,const char *name)
 {
     dbRecordNode *precnode = pdbentry->precnode;
diff --git a/src/ioc/dbStatic/dbStaticLib.h b/src/ioc/dbStatic/dbStaticLib.h
index b2f4a02..92b32d9 100644
--- a/src/ioc/dbStatic/dbStaticLib.h
+++ b/src/ioc/dbStatic/dbStaticLib.h
@@ -188,6 +188,8 @@ epicsShareFunc long dbFirstInfo(DBENTRY *pdbentry);
 epicsShareFunc long dbNextInfo(DBENTRY *pdbentry);
 epicsShareFunc long dbFindInfo(DBENTRY *pdbentry,
     const char *name);
+epicsShareFunc long dbNextMatchingInfo(DBENTRY *pdbentry,
+    const char *pattern);
 epicsShareFunc long dbDeleteInfo(DBENTRY *pdbentry);
 epicsShareFunc const char * dbGetInfoName(DBENTRY *pdbentry);
 epicsShareFunc const char * dbGetInfoString(DBENTRY *pdbentry);