Merge ~dirk.zimoch/epics-base:FixShellCommands into ~epics-core/epics-base/+git/epics-base:7.0

[Dirk Zimoch]

Fix several ioc shell functions.

The functions 'echo', 'dlload', 'epicsParamShow', 'setIocLogDisable' and 'errlog' which are available in iocsh were missing in the vxWorks shell. Calling 'errlog' caused a crash because of a thread with that name. These functions have now been added to the vxWorks shell.

Several functions showed buggy behaviour or printed hard to understand error messages when being called without arguments.
* 'epicsEnvSet' without arguments used to set *NULL to *NULL causing problems later
* 'dbLoadDatabase' and 'dbLoadRecords' printed: "dbRead opening file (null)"
* 'dbap', 'dbb' and 'dbd' printed: "Record (null) not found"
* 'gft', 'pft' and 'tpn' printed: "Channel couldn't be created"
* 'dbtpn' printed: "dbtpn: No such channel" but without newline.
They all now print a "Usage: ..." message.
BTW: I found that all the tests for those functions did not include missing (NULL) mandatory string arguments. I suggest to add that case to the tests for every shell function.

Not fixed:
* 'var' is not available on the vxWorks shell but would be difficult to implement.
* 'epicsThreadSleep' called on the vxWorks shell without arguments sleeps for an unpredictable period. This is a problem of using double type arguments on the vxWorks shell.

[Andrew Johnson (anj)]

Hi Dirk,

Good idea, a few comments about your specific changes though.

It's only the VxWorks shell (and I guess the RTEMS CEXP shell) that can pass in NULL pointers to some of these commands. Adding the NULL check to any osdEnv.c file except for libcom/src/osi/os/vxWorks/osdEnv.c doesn't make too much sense to me.

I would prefer that we not print anything in dbStateCreate() which is also an API entry point for the dbState subsystem, just return NULL for a NULL input.

Your VxWorks dlload() routine needs a check for a NULL input argument...

Finally a few comments about your Usage messages: Parentheses are not required around arguments to VxWorks or iocsh commands, they're only needed for an IOC using CEXP on RTEMS, and the people who run such IOCs know about the requirement. Parentheses take longer to type and I'd rather we not imply they're needed when they aren't. The iocsh help output shows command usage without parentheses (which are allowed there too), although I wish it prefixed the full command with "Usage:" when shown with argumemts (like you do):

epics> help dbLoadDatabase
dbLoadDatabase 'file name' path substitutions
epics> help dbLoadRecords
dbLoadRecords 'file name' substitutions
epics> help dbb
dbb 'record name'
epics> help dbtpn
dbtpn 'record name' value
epics> help epicsEnvSet
epicsEnvSet name value

That help command only puts quotes around arguments that have spaces in their name, which is different than the requirements of the VxWorks shell (but that is how the iocsh parses arguments). Your use of double-quotes around string arguments shows they are required for VxWorks though, which is fine and correct.

Thanks,

- Andrew

[Dirk Zimoch (dirk.zimoch)]

Hi Andrew,

It all stared when testing EPICS 7 on vxWorks, thus I am a bit focused on vxWorks.

If you do skip a string argument in iocsh, a NULL is passed to the underlying C function if the *Func wrapper does not handle this case separately.

On a Linux IOC I get this:
epics> epicsEnvSet
Missing environment variable name argument.

So this one indeed prints a good error message.

But:
epics> dbLoadDatabase
filename="../../../src/ioc/dbStatic/dbLexRoutines.c" line number=264
dbRead opening file (null)
epics> dbb
   BKPT> Record (null) not found
epics> gft
Channel couldn't be created
epics> dbtpn
dbtpn: No such channelepics>
epics> dbStateCreate
Segmentation fault

I will remove the message from dbStateCreate().

dlload on vxWorks does already print a useful error message:
-> dlload
epicsLoadLibrary failed: S_ioLib_NO_FILENAME
value = 45 = 0x2d = '-'

But I can add a "Usage:" message as well.

You are right about the (). I will remove them.

I will change the usage messages to remove the () but keep the "". The iocsh users probably know that they are not really necessary. What about optional parameters? Put them in []? But that may confuse the inexperienced user who might then type literal [].

Dirk

[Dirk Zimoch (dirk.zimoch)]

dbStateFind() is not a shell command but should probably check for NULL as well. I'll add it.

[Dirk Zimoch (dirk.zimoch)]

'gft', 'pft' and 'tpn' used to print a usage message in 3.14:

usage "pv name","value"

But that had been changed to "Channel couldn't be created" in 3.15.

[Dirk Zimoch (dirk.zimoch)]

Also 'pft' and 'tpn' had not checked for a 'value' argument.
In a Linux IOC:
epics> pft existing_record
Segmentation fault

[Dirk Zimoch (dirk.zimoch)]

Apropos epicsEnvSet: I have got requests for a method to remove environment variables. At the moment that is not possible, but would be useful for some scripts. Currently one can only set environment variables to an empty string, but that does not trigger the default value is $(variable=default) expansions.
I propose to use epicsEnvSet with a missing value argument to delete the variable, but I have to check if it possible for all architectures. Some have unsetenv(), some support setenv("variable") without "=value". In vxWorks 5 I had to use a hack (destroy the name by setting the first char to 0).

Should I add that feature here or make a new pull request?

[Andrew Johnson (anj)]

I think setting an environment variable to an empty string needs to be allowed (without deleting it). Can I request that you add a new command epicsEnvUnset, and since it will affect more than just VxWorks I think a new merge proposal for that from a separate branch would be appropriate. You can tell Launchpad 'this merge proposal depends on that one having already been applied' to save effort and merge conflicts if you need to.

Thanks!

- Andrew

review: Approve

[Dirk Zimoch (dirk.zimoch)]

Setting a variable to an empty string would not be a problem even without a separate command, because "" is different from NULL:

Empty string:
epicsEnvSet "var",""

Delete:
epicsEnvSet "var"

(Work for both vxWorks shell and iocsh.)

But I will implement epicsEnvUnset because it is the cleaner interface.

[Andrew Johnson (anj)]

I really don't like adding "#ifdef vxWorks" blocks like this does. We have mechanisms that avoid having such blocks of OS-specific code by putting OS-specific routines in OS-specific source files or by defining OS-specific macros. As a result I can do a git grep '#ifdef vxWorks' on the 7.0 tree and the only hits it finds are in test programs. Also some of the additions here are duplicating code from other routines in the same file, which is another code smell from this merge request.

Maybe we could add a macro to iocsh.h for routines that should be exported for VxWorks, so that for example the dlload command could be implemented like this:

static const iocshFuncDef dlloadFuncDef = {"dlload", 1, dlloadArgs};
IOCSH_STATIC_FUNC void dlload(const char* name)
{
    if (!name || !*name) {
        printf("usage: dlload \"file\"\n");
    }
    else if (!epicsLoadLibrary(name)) {
        printf("epicsLoadLibrary failed: %s\n", epicsLoadError());
    }
}
static void dlloadCallFunc(const iocshArgBuf *args)
{
    dlload(args[0].sval);
}
static void dlloadRegistar(void) {

The IOCSH_STATIC_FUNC macro would expand to static EPICS_ALWAYS_INLINE on everything except VxWorks where it would be empty (Hmm, should GeSys users get these routines as well?). The body of the function is no longer duplicated, there's nothing here which is VxWorks-specific, and if GeSys does need these functions we only have to edit the iocsh.h header to make them available there too.

I'm *not* suggesting that we should have a modules/database/src/ioc/misc/os/vxWorks/dlload.c file, that would be taking this too far and I think the macro solution should work fine.

Sorry to go back on my earlier approval...

review: Needs Fixing

[Dirk Zimoch (dirk.zimoch)]

OK. I didn't like the code duplication either, but I wanted to keep the change minimal.
Concerning #ifdef <ARCH>, I actually like it more than separate files. Why? To avoid code duplication.

I will try to implement some macro magic...

[Dirk Zimoch (dirk.zimoch)]

Are RTEMS/GeSys users the only ones using Cexp? If not, the functions should probably be global for everyone. How to find out if someone uses GeSys/Cexp? Should I assume this for any RTEMS and RTEMS only, thus making the functions global for vxWorks and RTEMS and static for every other OS?

Can there be a conflict with existing system functions, in particular for 'dlload' and 'echo'? Originally it was this concern why I implemented the functions for vxWorks only.

[Dirk Zimoch (dirk.zimoch)]

Now I have
#if defined(vxWorks) || defined(__rtems__)
centrally in iocsh.h.
If preferred, I can create separate header files for vxWorks and RTEMS in osi/os/xx instead.

[Andrew Johnson (anj)]

Cexp only exists for RTEMS, Till wrote it to replicate the functionality of the VxWorks target shell.

I'm okay with that OS-conditional being in iocsh.h, so this looks good now and I will merge it sometime in the next few days — thanks.

review: Approve

diff --git a/modules/database/src/ioc/db/dbAccess.c b/modules/database/src/ioc/db/dbAccess.c
index cd75351..f52b888 100644
--- a/modules/database/src/ioc/db/dbAccess.c
+++ b/modules/database/src/ioc/db/dbAccess.c
@@ -754,13 +754,22 @@ long dbBufferSize(short dbr_type, long options, long no_elements)
 }
 int dbLoadDatabase(const char *file, const char *path, const char *subs)
 {
+    if (!file) {
+        printf("Usage: dbLoadDatabase \"file\", \"path\", \"subs\"\n");
+        return -1;
+    }
     return dbReadDatabase(&pdbbase, file, path, subs);
 }
 
 int dbLoadRecords(const char* file, const char* subs)
 {
-    int status = dbReadDatabase(&pdbbase, file, 0, subs);
+    int status;
 
+    if (!file) {
+        printf("Usage: dbLoadRecords \"file\", \"subs\"\n");
+        return -1;
+    }
+    status = dbReadDatabase(&pdbbase, file, 0, subs);
     if (!status && dbLoadRecordsHook)
         dbLoadRecordsHook(file, subs);
     return status;
diff --git a/modules/database/src/ioc/db/dbBkpt.c b/modules/database/src/ioc/db/dbBkpt.c
index 8e7bf3a..b3fdd2d 100644
--- a/modules/database/src/ioc/db/dbBkpt.c
+++ b/modules/database/src/ioc/db/dbBkpt.c
@@ -283,6 +283,10 @@ long dbb(const char *record_name)
  /*
   *  Convert name to address
   */
+  if (!record_name) {
+      printf("Usage: dbb \"record_name\"\n");
+      return -1;
+  }
   status = dbNameToAddr(record_name, &addr);
   if (status == S_db_notFound)
       printf("   BKPT> Record %s not found\n", record_name);
@@ -403,6 +407,10 @@ long dbd(const char *record_name)
  /*
   *  Convert name to address
   */
+  if (!record_name) {
+      printf("Usage: dbd \"record_name\"\n");
+      return -1;
+  }
   status = dbNameToAddr(record_name, &addr);
   if (status == S_db_notFound)
       printf("   BKPT> Record %s not found\n", record_name);
@@ -846,6 +854,10 @@ long dbap(const char *record_name)
  /*
   *  Convert name to address
   */
+  if (!record_name) {
+      printf("Usage: dbap \"record_name\"\n");
+      return -1;
+  }
   status = dbNameToAddr(record_name, &addr);
   if (status == S_db_notFound)
       printf("   BKPT> Record %s not found\n", record_name);
diff --git a/modules/database/src/ioc/db/dbNotify.c b/modules/database/src/ioc/db/dbNotify.c
index c718d37..c2420af 100644
--- a/modules/database/src/ioc/db/dbNotify.c
+++ b/modules/database/src/ioc/db/dbNotify.c
@@ -596,9 +596,13 @@ long dbtpn(char *pname, char *pvalue)
     tpnInfo *ptpnInfo;
     processNotify *ppn=NULL;
 
+    if (!pname) {
+        printf("Usage: dbtpn \"name\", \"value\"\n");
+        return -1;
+    }
     chan = dbChannelCreate(pname);
     if (!chan) {
-        printf("dbtpn: No such channel");
+        printf("dbtpn: No such channel\n");
         return -1;
     }
     
diff --git a/modules/database/src/ioc/db/dbState.c b/modules/database/src/ioc/db/dbState.c
index 6de7735..60819a0 100644
--- a/modules/database/src/ioc/db/dbState.c
+++ b/modules/database/src/ioc/db/dbState.c
@@ -37,6 +37,9 @@ dbStateId dbStateFind(const char *name)
     ELLNODE *node;
     dbStateId id;
 
+    if (!name)
+        return NULL;
+
     for (node = ellFirst(&states); node; node = ellNext(node)) {
         id = CONTAINER(node, dbState, node);
         if (strcmp(id->name, name) == 0)
@@ -49,6 +52,9 @@ dbStateId dbStateCreate(const char *name)
 {
     dbStateId id;
 
+    if (!name)
+        return NULL;
+
     if ((id = dbStateFind(name)))
         return id;
 
diff --git a/modules/database/src/ioc/db/db_test.c b/modules/database/src/ioc/db/db_test.c
index 8d7ad31..3d536f0 100644
--- a/modules/database/src/ioc/db/db_test.c
+++ b/modules/database/src/ioc/db/db_test.c
@@ -40,6 +40,10 @@ int gft(const char *pname)
     short type;
     int i;
 
+    if (!pname) {
+        printf("Usage: gft \"pv_name\"\n");
+        return -1;
+    }
     chan = dbChannel_create(pname);
     if (!chan) {
         printf("Channel couldn't be created\n");
@@ -94,6 +98,10 @@ int pft(const char *pname, const char *pvalue)
     unsigned char charvalue;
     double doublevalue;
 
+    if (!pname || !pvalue) {
+        printf("Usage: pft \"pv_name\", \"value\"\n");
+        return -1;
+    }
     chan = dbChannel_create(pname);
     if (!chan) {
         printf("Channel couldn't be created\n");
@@ -223,6 +231,10 @@ int tpn(const char *pname, const char *pvalue)
     tpnInfo *ptpnInfo;
     processNotify *ppn = NULL;
 
+    if (!pname || !pvalue) {
+        printf("Usage: tpn \"pv_name\", \"value\"\n");
+        return -1;
+    }
     chan = dbChannel_create(pname);
     if (!chan) {
         printf("Channel couldn't be created\n");
diff --git a/modules/database/src/ioc/misc/dlload.c b/modules/database/src/ioc/misc/dlload.c
index 5b0591d..ddf04f4 100644
--- a/modules/database/src/ioc/misc/dlload.c
+++ b/modules/database/src/ioc/misc/dlload.c
@@ -9,14 +9,19 @@
 #include "iocsh.h"
 #include "epicsExport.h"
 
+IOCSH_STATIC_FUNC void dlload(const char* name)
+{
+    if (!epicsLoadLibrary(name)) {
+        printf("epicsLoadLibrary failed: %s\n", epicsLoadError());
+    }
+}
+
 static const iocshArg dlloadArg0 = { "path/library.so", iocshArgString};
 static const iocshArg * const dlloadArgs[] = {&dlloadArg0};
 static const iocshFuncDef dlloadFuncDef = {"dlload", 1, dlloadArgs};
 static void dlloadCallFunc(const iocshArgBuf *args)
 {
-    if (!epicsLoadLibrary(args[0].sval)) {
-        printf("epicsLoadLibrary failed: %s\n", epicsLoadError());
-    }
+    dlload(args[0].sval);
 }
 
 static void dlloadRegistar(void) {
diff --git a/modules/libcom/src/iocsh/iocsh.h b/modules/libcom/src/iocsh/iocsh.h
index 3ef3d95..84b38f2 100644
--- a/modules/libcom/src/iocsh/iocsh.h
+++ b/modules/libcom/src/iocsh/iocsh.h
@@ -14,8 +14,15 @@
 #define INCiocshH
 
 #include <stdio.h>
+#include "compilerDependencies.h"
 #include "shareLib.h"
 
+#if defined(vxWorks) || defined(__rtems__)
+#define IOCSH_STATIC_FUNC
+#else
+#define IOCSH_STATIC_FUNC static EPICS_ALWAYS_INLINE
+#endif
+
 #ifdef __cplusplus
 extern "C" {
 #endif
diff --git a/modules/libcom/src/iocsh/libComRegister.c b/modules/libcom/src/iocsh/libComRegister.c
index d3a5cfa..88abdc6 100644
--- a/modules/libcom/src/iocsh/libComRegister.c
+++ b/modules/libcom/src/iocsh/libComRegister.c
@@ -27,6 +27,7 @@
 #include "libComRegister.h"
 
 
+/* date */
 void date(const char *format)
 {
     epicsTimeStamp now;
@@ -42,7 +43,6 @@ void date(const char *format)
     puts(nowText);
 }
 
-/* date */
 static const iocshArg dateArg0 = { "format",iocshArgString};
 static const iocshArg * const dateArgs[] = {&dateArg0};
 static const iocshFuncDef dateFuncDef = {"date", 1, dateArgs};
@@ -52,13 +52,8 @@ static void dateCallFunc (const iocshArgBuf *args)
 }
 
 /* echo */
-static const iocshArg echoArg0 = { "string",iocshArgString};
-static const iocshArg * const echoArgs[1] = {&echoArg0};
-static const iocshFuncDef echoFuncDef = {"echo",1,echoArgs};
-static void echoCallFunc(const iocshArgBuf *args)
+IOCSH_STATIC_FUNC void echo(char* str)
 {
-    char *str = args[0].sval;
-
     if (str)
         dbTranslateEscape(str, str); /* in-place is safe */
     else
@@ -66,6 +61,14 @@ static void echoCallFunc(const iocshArgBuf *args)
     printf("%s\n", str);
 }
 
+static const iocshArg echoArg0 = { "string",iocshArgString};
+static const iocshArg * const echoArgs[1] = {&echoArg0};
+static const iocshFuncDef echoFuncDef = {"echo",1,echoArgs};
+static void echoCallFunc(const iocshArgBuf *args)
+{
+    echo(args[0].sval);
+}
+
 /* chdir */
 static const iocshArg chdirArg0 = { "directory name",iocshArgString};
 static const iocshArg * const chdirArgs[1] = {&chdirArg0};
@@ -111,10 +114,15 @@ static void epicsEnvSetCallFunc(const iocshArgBuf *args)
 }
 
 /* epicsParamShow */
+IOCSH_STATIC_FUNC void epicsParamShow()
+{
+    epicsPrtEnvParams ();
+}
+
 static const iocshFuncDef epicsParamShowFuncDef = {"epicsParamShow",0,NULL};
 static void epicsParamShowCallFunc(const iocshArgBuf *args)
 {
-    epicsPrtEnvParams ();
+    epicsParamShow ();
 }
 
 /* epicsPrtEnvParams */
@@ -148,12 +156,17 @@ static void iocLogInitCallFunc(const iocshArgBuf *args)
 }
 
 /* iocLogDisable */
+IOCSH_STATIC_FUNC void setIocLogDisable(int val)
+{
+    iocLogDisable = val;
+}
+
 static const iocshArg iocLogDisableArg0 = {"(0,1)=>(false,true)",iocshArgInt};
 static const iocshArg * const iocLogDisableArgs[1] = {&iocLogDisableArg0};
 static const iocshFuncDef iocLogDisableFuncDef = {"setIocLogDisable",1,iocLogDisableArgs};
 static void iocLogDisableCallFunc(const iocshArgBuf *args)
 {
-    iocLogDisable = args[0].ival;
+    setIocLogDisable(args[0].ival);
 }
 
 /* iocLogShow */
@@ -197,12 +210,17 @@ static void errlogInit2CallFunc(const iocshArgBuf *args)
 }
 
 /* errlog */
+IOCSH_STATIC_FUNC void errlog(const char *message)
+{
+    errlogPrintfNoConsole("%s\n", message);
+}
+
 static const iocshArg errlogArg0 = { "message",iocshArgString};
 static const iocshArg * const errlogArgs[1] = {&errlogArg0};
 static const iocshFuncDef errlogFuncDef = {"errlog",1,errlogArgs};
 static void errlogCallFunc(const iocshArgBuf *args)
 {
-    errlogPrintfNoConsole("%s\n", args[0].sval);
+    errlog(args[0].sval);
 }
 
 /* iocLogPrefix */
diff --git a/modules/libcom/src/osi/os/Darwin/osdEnv.c b/modules/libcom/src/osi/os/Darwin/osdEnv.c
index ab3f936..e6206c3 100644
--- a/modules/libcom/src/osi/os/Darwin/osdEnv.c
+++ b/modules/libcom/src/osi/os/Darwin/osdEnv.c
@@ -35,6 +35,7 @@
  */
 epicsShareFunc void epicsShareAPI epicsEnvSet (const char *name, const char *value)
 {
+    if (!name) return;
     iocshEnvClear(name);
     setenv(name, value, 1);
 }
diff --git a/modules/libcom/src/osi/os/default/osdEnv.c b/modules/libcom/src/osi/os/default/osdEnv.c
index 682bcc9..b284a86 100644
--- a/modules/libcom/src/osi/os/default/osdEnv.c
+++ b/modules/libcom/src/osi/os/default/osdEnv.c
@@ -36,6 +36,7 @@ epicsShareFunc void epicsShareAPI epicsEnvSet (const char *name, const char *val
 {
     char *cp;
 
+    if (!name) return;
     iocshEnvClear(name);
     
 	cp = mallocMustSucceed (strlen (name) + strlen (value) + 2, "epicsEnvSet");
diff --git a/modules/libcom/src/osi/os/iOS/osdEnv.c b/modules/libcom/src/osi/os/iOS/osdEnv.c
index a32cce5..fdc4570 100644
--- a/modules/libcom/src/osi/os/iOS/osdEnv.c
+++ b/modules/libcom/src/osi/os/iOS/osdEnv.c
@@ -32,6 +32,7 @@
  */
 epicsShareFunc void epicsShareAPI epicsEnvSet (const char *name, const char *value)
 {
+    if (!name) return;
     iocshEnvClear(name);
     setenv(name, value, 1);
 }
diff --git a/modules/libcom/src/osi/os/vxWorks/osdEnv.c b/modules/libcom/src/osi/os/vxWorks/osdEnv.c
index c81f493..9245ae7 100644
--- a/modules/libcom/src/osi/os/vxWorks/osdEnv.c
+++ b/modules/libcom/src/osi/os/vxWorks/osdEnv.c
@@ -37,6 +37,11 @@ epicsShareFunc void epicsShareAPI epicsEnvSet (const char *name, const char *val
 {
     char *cp;
 
+    if (!name) {
+        printf ("Usage: epicsEnvSet \"name\", \"value\"\n");
+        return;
+    }
+
     iocshEnvClear(name);
     
     cp = mallocMustSucceed (strlen (name) + strlen (value) + 2, "epicsEnvSet");