Fix: dispatching any action causes actmon/actlog to crash with SIGSEGV

[merlea]

• Rename Svr -> Srv
• Always use 2 arguments for callback_done signature

A possible fix for #2829 basically setting all the job_done, action_done callbacks to have a signature void (*ast)(void *astprm, char *msg) and setting msg to NULL when applicable. Using this fix with the case described in #2829 leads to no error.

[zack-vii]

I think we need to add a test maybe using actlog (terminal version of actmon).
What i found is that actmon/actlog dont handle null either .. further they need to be protected in case the action monitor server (mdsip) becomes offline otherwise it may generate a stack overflow.

[zack-vii]

If the ACTION_MONITOR server becomes unreachabel/offline The CheckIN will eventually call MessageAst with a NULL for message. This will cause if (!parseMsg(reply, event)) to skip and call CheckIN recursively until the program stack overflows.

We need to be able to detect this from the callback msg somehow, possibly by returning a special message instead. alternatively the CheckIN must be called from a async outside of MessageAst with some wait time to prevent a runaway process at 100% as well as the stack overflow due to recursion.

[merlea]

OK. I removed the recursive calls to CheckIn since it already had a loop with pauses.

I also put some stubs for tests of actlog based on the original issue. We could compare the output of actlog with the expected one. I am not too familiar with the tests setup so let me know how best to integrate this.

[joshStillerman]

retest this please

[WhoBrokeTheBuild]

By changing the definition of the ast parameter to include arguments, you've triggered some compile errors. You can run a build/test with:

./deploy/build.sh --os=bootstrap
./deploy/build.sh --os=ubuntu24 --test

And here is the error from the Jenkins build:

gcc -Wall -Wextra -Werror   -g -DWITH_DEBUG_SYMBOLS -O0 -fvisibility=hidden  -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -fpic -shared-libgcc -fsigned-char -fno-strict-aliasing   -DLIBPREFIX=MDSplus -pthread -I/workspace/tests/64/_include -I/opt/jenkins/workspace/MDSplus_PR-2835/debian10-64/_include -I/workspace/tests/64/include -I/opt/jenkins/workspace/MDSplus_PR-2835/debian10-64/include  -m64 -c -o tcl_dispatch.o /opt/jenkins/workspace/MDSplus_PR-2835/debian10-64/tcl/tcl_dispatch.c

/opt/jenkins/workspace/MDSplus_PR-2835/debian10-64/tcl/tcl_dispatch.c: In function 'TclDispatch_command':

/opt/jenkins/workspace/MDSplus_PR-2835/debian10-64/tcl/tcl_dispatch.c:510:36: error: passing argument 5 of 'ServerDispatchCommand' from incompatible pointer type [-Werror=incompatible-pointer-types]

                                    CommandDone, c.cmd, iostatusp, NULL, 0);

                                    ^~~~~~~~~~~

In file included from /opt/jenkins/workspace/MDSplus_PR-2835/debian10-64/tcl/tcl_dispatch.c:35:

/opt/jenkins/workspace/MDSplus_PR-2835/debian10-64/include/servershr.h:41:63: note: expected 'void (*)(void *, char *)' but argument is of type 'void (*)(DispatchedCommand *, char *)' {aka 'void (*)(struct <anonymous> *, char *)'}

                                         char *command, void (*ast)(void *, char *),

                                                        ~~~~~~~^~~~~~~~~~~~~~~~~~~~

If you include a fix for that, we will rerun the Jenkins build. Otherwise, we will look into this when we have time.

Thank you for investigating this so thoroughly, we greatly appreciate your help :)

[zack-vii]

sorry i totally missed this. for your tests comparing output with expected output i think the tditest tests are a good candidate to copy from.

[merlea]

@WhoBrokeTheBuild I should have fixed the compilation issue.
I still have to work on the integration of tests, but perhaps we can rerun the Jenkins build to check that the compilation indeed goes through.

[WhoBrokeTheBuild]

Retest this please

[merlea]

OK. I have integrated the tests with the test suite.

I think this is now ready for a final review. @WhoBrokeTheBuild @zack-vii

[merlea]

OK. I have integrated the tests with the test suite.

I think this is now ready for a final review. @WhoBrokeTheBuild @zack-vii

@WhoBrokeTheBuild can we trigger the tests again please?

[WhoBrokeTheBuild]

I'm going to do some local verification early next week, and then I'll trigger the tests and we'll get this merged, thanks again for all your hard work!

[merlea]

Ping @WhoBrokeTheBuild , just making sure we don't forget this PR...

[heidthecamp]

Retest this please

[WhoBrokeTheBuild]

Once these are done, and the tests pass, I think we should merge this

[heidthecamp]

Retest this please

[merlea]

I implemented the requested changes but I expect some similar problems to #2906 in the tests for windows for example.

[WhoBrokeTheBuild]

Awesome, thank you! We really appreciate your patience while we work to get these merged 🎉
I spent a bit of time on it today, and I have some changes ready for you, I'll attach a diff.

The big things were:

• Conditionally prefix commands with wine
• Use ${TYPE}_SERVER variables to hold the localhost:${port} for each of the mdsip services (this means you don't have to hardcode the port numbers, and they will work when $TEST_PORT_OFFSET is set)
• Switch from pkill to kill, recording each process PID, which means we can run in the docker images that don't have pkill
• Add --strip-trailing-cr to diff so that it works with the windows build

merlea-bugfix-servershr-diff.txt

[WhoBrokeTheBuild]

Oh and I forgot to mention / include it in the diff, but please move the ports close to the range of other allocated ports in ports.csv, your other PR uses up through 8020 so I would use 8021-8023, cheers

[merlea]

Oh and I forgot to mention / include it in the diff, but please move the ports close to the range of other allocated ports in ports.csv, your other PR uses up through 8020 so I would use 8021-8023, cheers

OK. Done. I think I covered everything, some parts I had worked on before you sent the diff, so they are slightly different.

[heidthecamp]

Retest this please