WIP: Rtapi cleanup v2 by hdiethelm · Pull Request #3919 · LinuxCNC/linuxcnc

hdiethelm · 2026-04-09T20:43:18Z

Continuation of #3908 reverted in #3918

Target: Move some classes out of the huge uspace_rtapi_app.cc

uspace_rtapi_main: Contains the main function and helpers
uspace_rtapi_app: Contains the RtapiApp class
uspace_posix: Contains the PosixApp class

Other fixes:

Don't start master just for exit: 371793c
Remove unused rtapi_task::ratio: 3097578
- Set but only read to force a fixed ratio which probably was not the intention
Slightly different lock to avoid needing reference to instance: c8af827
All real time implementations are now library's and handled the same way: 62e5ea6
No hard-coded library paths e1bbaf4

hdiethelm · 2026-04-09T20:47:56Z

Still open:

libuspace-xxx.so.0 -> liblinuxcnc-rtapi-xxx.so ?
- Longest name would be liblinuxcnc-rtap-xenomai-evl.so.0
Check if slightly different lock to avoid needing reference to instance: c8af827 is a good idea
Review

And

In the Submakefile it reads:
$(call TOOBJSDEPS, $(USPACE_POSIX_SRCS)): EXTRAFLAGS += -pthread -fPIC
Shouldn't that be (not adding to EXTRAFLAGS):
$(call TOOBJSDEPS, $(USPACE_POSIX_SRCS)): EXTRAFLAGS = -pthread -fPIC

Hmm, I don't understand makefiles in depth. I just copied what was already there a few lines below and edited it to match my lib. As much as I understand this, it just adds this flags to the global EXTRAFLAGS in Makefile for one compile command only. I did not see any duplicated flags.

NTULINUX · 2026-04-11T07:36:19Z

Works here on Gentoo, rip, system install, clang and gcc builds, whole shebang!

edit: Have not yet tested with RTAI.

hdiethelm · 2026-04-11T10:46:25Z

Thanks for testing!

I renamed the library's to:

liblinuxcnc-uspace-posix.so.0 liblinuxcnc-uspace-xenomai-evl.so.0
liblinuxcnc-uspace-rtai.so.0    liblinuxcnc-uspace-xenomai.so.0

A bit long names, but I think it is fine. But i'm open for other suggestions.

Additionally, I reduced the globals.

I tested all 5 configurations in a VM and they all work. There are two issues but I don't think they are due to this PR:

LXRT doesn't start on an isolated CPU. Workaround: RTAPI_CPU_NUMBER=1 linuxcnc
RTAI doesn't unload the modules in the correct order and fails to unload all

hdiethelm · 2026-04-11T10:54:11Z

@NTULINUX: Do you have a test setup you can share or is it all just manually setup? A series of docker files would be nice, so different OS can be tested if something starts. I messed my VM up slightly by using make install and just after the fact figured out that there is no make uninstall but I was able to restore it.
If there is nothing yet, I can create some, should not be to involved. I often use podman for similar things where I don't want to mess up my host. Podman is similar to docker but it doesn't need root.

NTULINUX · 2026-04-11T13:34:21Z

I have VM images up right now but they're in flux. I'm going to post new VMs with all the right fixes in a bit. Currently tracking this PR here:

#3925

My ebuilds at the moment are broken but we're in the middle of sorting it all out for good. Will share link to new VM soon with everything tied together, cleanly.

src/rtapi/uspace_rtapi_main.cc

BsAtHome · 2026-04-10T21:41:05Z

src/rtapi/uspace_rtapi_main.cc

+    case SIGXCPU:
+        // should not happen - must be handled in RTAPI if enabled
+        rtapi_print_msg(RTAPI_MSG_ERR, "rtapi_app: BUG: SIGXCPU received - exiting\n");
+        exit(0);


You cannot call exit() in a signal handler. You must call _exit()

Also,using printf or puts in signal handlers is not allowed.

Do you have a reference why? I now understand the difference between the two but I did not find anything why _exit() is needed in signal handlers.

Thanks for the reference. Corrected.

BsAtHome · 2026-04-10T21:41:38Z

src/rtapi/uspace_rtapi_main.cc

+
+    case SIGTERM:
+        rtapi_print_msg(RTAPI_MSG_ERR, "rtapi_app: SIGTERM - shutting down\n");
+        exit(0);


exit() -> _exit()

BsAtHome · 2026-04-10T21:41:47Z

src/rtapi/uspace_rtapi_main.cc

+        kill(getpid(), sig);
+        break;
+    }
+    exit(1);


exit() -> _exit()

BsAtHome · 2026-04-10T21:43:36Z

src/rtapi/uspace_rtapi_main.cc

+
+    default: // pretty bad
+        rtapi_print_msg(RTAPI_MSG_ERR, "rtapi_app: caught signal %d - dumping core\n", sig);
+        sleep(1); // let syslog drain


calling sleep() in a signal handler may be problematic because it can be implemented using SIGALRM. Signals in signal is a very bad concept.

Do you have a good solution for this? I don't really understand what the idea behind this code is.

The whole "exit" procedure done in a signal handler is faulty and wrong. You normally run your main handler in a select/poll loop where one of the input descriptors is the read-end of a pipe. You write (one byte) into this pipe from the signal handler and you know what to do outside the signal handler in the main handler loop. Then you can use a simple switch outside the signal handler to determine what to do.

The code "tries" to do a flush, but does it in the wrong place. Anything called in there is bound to be wrong or not guaranteed to work as you need it to. BTW, dumping core should not be preceded by printing a message because all you know is that the printing of a message cause the signal (SEGV for example) and everything is corrupt. Dumping code needs to be just that, dump core.

BsAtHome · 2026-04-10T21:50:42Z

src/rtapi/uspace_rtapi_main.cc

+        //If called in master mode with exit command, no need to start master
+        //and exit again
+        if (args.size() == 1 && args[0] == "exit") {
+            return 0;
+        }


If you exit here, then why perform the socket() and bind() calls?

This code is a bit funny: Bind is used to detect if a master is running. It returns 0 if no master is already running, so a new one is started.

Now I had the case when exiting, everything was initialized again due to master exits automatically as soon as there is no instance any more: Line 417.

Due to that, master was started again just to call exit and "Note: Using POSIX realtime" was shown a second time before exiting when I closed latency test.

But isn't that is just as race prone as any other method?

The concept "the first becomes master" is flawed when you try to exit.

Which kind or race do you have in mind? I think bind is probably atomic, I would have to research it.

The issue I fixed is just: "Don't start a master if none is running just to immediately close it due to the command was exit"

The only that came to my mind is if the there are multiple clients at the same time, one sends exit while the other sends a loadrt for example. In that case, it is random if the master exits and then starts again to execute loadrt or the other way around, loadrt and then exit.

But this "If there is no master, I will become master" feels a bit wrong in general. Better would be: "A master is started at start of linuxcnc and closed at the end" but this is to much for this PR i think and needs probably changes in other places to.

By the time you determine that there is no master, someone else can have become master. By the time you determine that there is a master, it may have exited before sending your command.

But you are right, this needs to be rethought and redone from the ground up. Lets leave it for now.

By the time you determine that there is no master, someone else can have become master.

As long as bind is atomic, this can not happen. If bind is not atomic, two applications could be server on the same socket which would be a linux bug.

By the time you determine that there is a master, it may have exited before sending your command.

That can happen only if the application removes everything and then starts to add new things due to the master exits on exit or if there are no instances any more. Probably no linuxcnc app should do that but you can do that manually in halcmd. I have to test this.

But you are right, this needs to be rethought and redone from the ground up. Lets leave it for now.

Ok, we do that later. I think it's not a bad issue, just hard to read code with possible issues.

BsAtHome · 2026-04-10T21:54:27Z

src/rtapi/uspace_rtapi_main.cc

+                break;
+            if (i == 0)
+                srand48(t0.tv_sec ^ t0.tv_usec);
+            usleep(lrand48() % 100000);


Can there be unforeseen interaction with SIGALRM here? I don't think you want to involve signals. Also, wouldn't there be a minimum wait wanted here? A random wait can return close to zero all the time and you only loop three times.

That one looks funny. Signal will break it, the for loop is only 0.3s (100000 != 1000000) and gettimeofday can fail if you change the system time. Improved.

I did not find a function for diff of timespec, so I added one above main.

BTW: That one could also be simplified by just using (now->tv_sec - start->tv_sec) < 3 but this would be something between 2 and 4s.

BsAtHome · 2026-04-10T21:56:29Z

src/rtapi/uspace_rtapi_main.cc

+    return s;
+}
+
+static int get_fifo_path(char *buf, size_t bufsize) {


Why use char* and size_t? Why not have a std::string& as argument?
And why are there two get_fifo_path() functions with different arguments? Can't they be replaced by one?

The first function gets the path as std::string. The second copies it to a char pointer and checks the size.
That was the original implementation, I found that somewhat convoluted and the return NULL was also not good to read, so I reordered it a bit without changing to much:
https://github.com/LinuxCNC/linuxcnc/blob/master/src/rtapi/uspace_rtapi_app.cc#L503

But the whole cascade of get_fifo_path feels wrong IMO. We should get rid of char buffers as much as we can (everywhere in the code).

You are right, it feels wrong. I tried to improve it by renaming the second to get_fifo_path_to_addr() and do what it says.

BsAtHome · 2026-04-10T21:58:22Z

src/rtapi/uspace_rtapi_main.cc

+    pthread_cancel(queue_thread);
+    pthread_join(queue_thread, nullptr);
+    rtapi_msg_queue.consume_all([](const message_t &m) {
+        fputs(m.msg, m.level == RTAPI_MSG_ALL ? stdout : stderr);


Why fputs()?

It writes out all pending messages before exiting.
Same on line 81 where the normal message writer task function is. It's a bit risky when the 0 termination is missing but otherwise fine. Any sugestions?
m.level == RTAPI_MSG_ALL ? stdout : stderr sends to stdout if the message is from rtapi_print() -> RTAPI_MSG_ALL, otherwise stderr. Looks also fine.

As long as it isn't in a signal handler it should be fine. Just wondering.

However, now you mention rtapi_print (and friends), there are rtapi_print_msg in the signal handler. That may pose a real problem. The default handler prints using stdio and that is a nono in a signal handler.

This one is not in a signal and the standard handler for uspace_rtapi writes to a fifo. Only uspace_ulapi prints directly which is fine due to this has no signal handler.

This "just link a different function with the same name" is hard to track.

But motion.c uses rtapi_set_msg_handler() to change it. I have to track this. I think this is a real time thread.

Yes, motion.c is part of motmod and is part of the interface layer in realtime from commands from non-realtime.

BsAtHome · 2026-04-10T22:08:44Z

src/rtapi/uspace_rtapi_main.cc

+static int read_number(int fd) {
+    int r = 0, neg = 1;
+    char ch;
+
+    while (1) {
+        int res = read(fd, &ch, 1);
+        if (res != 1)
+            return -1;
+        if (ch == '-')
+            neg = -1;
+        else if (ch == ' ')
+            return r * neg;
+        else
+            r = 10 * r + ch - '0';
+    }
+}
+
+static std::string read_string(int fd) {
+    int len = read_number(fd);
+    if (len < 0)
+        throw ReadError();
+    if (!len)
+        return std::string();
+    std::string str(len, 0);
+    if (read(fd, str.data(), len) != len)
+        throw ReadError();
+    return str;
+}
+
+static std::vector<std::string> read_strings(int fd) {
+    std::vector<std::string> result;
+    int count = read_number(fd);
+    if (count < 0)
+        return result;
+    for (int i = 0; i < count; i++) {
+        result.push_back(read_string(fd));
+    }
+    return result;
+}
+
+static void write_number(std::string &buf, int num) {
+    buf = buf + fmt::format("{} ", num);
+}
+
+static void write_string(std::string &buf, const std::string &s) {
+    write_number(buf, s.size());
+    buf += s;
+}
+
+static void write_strings(int fd, const std::vector<std::string> &strings) {
+    std::string buf;
+    write_number(buf, strings.size());
+    for (unsigned int i = 0; i < strings.size(); i++) {
+        write_string(buf, strings[i]);
+    }
+    if (write(fd, buf.data(), buf.size()) != (ssize_t)buf.size())
+        throw WriteError();
+}


This is a socket communication we work with. Why isn't this passed through and handled by the standard sendmsg() and recvmsg() system calls? Wouldn't they be much better at handling this? Or is there and advantage to all this code?

I think it doesn't make a difference. send() / recv() could improve the readability. sendmsg() / recvmsg() is a bit annoying to use for this application due to you have first to fill the struct. What I found so far:
https://www.man7.org/linux/man-pages/man2/send.2.html
https://www.man7.org/linux/man-pages/man2/recv.2.html
The only difference between send() and [write(2)](https://www.man7.org/linux/man-pages/man2/write.2.html) is the presence of flags. With a zero flags argument, send() is equivalent to [write(2)](https://www.man7.org/linux/man-pages/man2/write.2.html).

Just thought that you could use scatter/gather I/O on that you were writing instead of manual serializing.

It would be even nicer to use a real protocol instead of numbers/strings but not for now I think.

Proper serialization, now that's a thought ;-)
Agreed, next future round of fixes can address that.

andypugh · 2026-04-12T18:46:49Z

Some discussion in the Sunday video meet-up has suggested that we should look at incorporating this into the rtapi cleanup:

#918

BsAtHome · 2026-04-12T20:48:25Z

src/rtapi/uspace_rtapi_main.cc

+    len = snprintf(buf + 1, bufsize - 1, "%s", s.c_str());
+    return len;


And then we're back to snprintf(). fmt::format would fit better. Length check can be done where it is relevant.

hdiethelm · 2026-04-12T20:51:06Z

I commented and corrected the things I have changed.
Now do we really want do do all of that in this MR?

I always prefer to have refactoring (moving code around) and bug fixing/functional changes as separated as possible. That makes review and testing easier, you just check that the moved code arrived as it was and so avoids having merge conflicts due to the branch staying open for to long.

But I have to admit, I also did some changes that I just was not able to leave it as it was and which simplified moving code due to removed dependency's and of course created a bug doing so.

BsAtHome · 2026-04-12T20:56:50Z

See: https://www.man7.org/linux/man-pages/man7/signal-safety.7.html for the callyou are allowed to make in signal handlers.

BsAtHome · 2026-04-12T21:04:15Z

I commented and corrected the things I have changed. Now do we really want do do all of that in this MR?

Well, that is a good question. You are doing "cleanup" and that would imply refactor and fixes, IMO.

I always prefer to have refactoring (moving code around) and bug fixing/functional changes as separated as possible. That makes review and testing easier, you just check that the moved code arrived as it was and so avoids having merge conflicts due to the branch staying open for to long.

That is a possibility too, but there are soooo (key got stuck) many problems with this code that it is a good question why not hit those ~~two birds with one stone~~ two lines with one keypress.

But I have to admit, I also did some changes that I just was not able to leave it as it was and which simplified moving code due to removed dependency's and of course created a bug doing so.

And I appreciate the changes! They are very necessary. Before we're done it needs to be tested,of course. But when the code gets better structured, then that should also become easier, I hope.

But, if you want to split it, then I think we need to have a very clear split where you move without actual changes and then refactor. My opinion is that moving code also classifies as refactoring and therefore it would be a missed opportunity if not fixed in one go.

BsAtHome · 2026-04-12T21:10:34Z

BTW, I've been wanting to vacuum this code for a long time but have not gotten arround to that point yet. You know, INI-file reader just done, HAL types/access revamp in the pipeline, tcl9 stuff that still needs fixing, build system cleanup, and so on.

(and also got CI's -Werror merged)

hdiethelm · 2026-04-12T21:31:59Z

I commented and corrected the things I have changed. Now do we really want do do all of that in this MR?

Well, that is a good question. You are doing "cleanup" and that would imply refactor and fixes, IMO.

Might be I did not specify my (initial) intentions well enough. I just found the rtapi_uspace hard to manage while implementing xenomai and wanted to split it up without changing the existing code if not needed. But that diverged anyway already a bit.

With the new structure, it should also be easier to implement rtapi_udp_sendto() or something similar needed for RTNet.

I always prefer to have refactoring (moving code around) and bug fixing/functional changes as separated as possible. That makes review and testing easier, you just check that the moved code arrived as it was and so avoids having merge conflicts due to the branch staying open for to long.

That is a possibility too, but there are soooo (key got stuck) many problems with this code that it is a good question why not hit those two birds with one stone two lines with one keypress.

But I have to admit, I also did some changes that I just was not able to leave it as it was and which simplified moving code due to removed dependency's and of course created a bug doing so.

And I appreciate the changes! They are very necessary. Before we're done it needs to be tested,of course. But when the code gets better structured, then that should also become easier, I hope.

But, if you want to split it, then I think we need to have a very clear split where you move without actual changes and then refactor. My opinion is that moving code also classifies as refactoring and therefore it would be a missed opportunity if not fixed in one go.

I have to look into it a bit more next week if I find time. But from my point of view it would make sense to split the more involved changes in a new PR or also two.

But there is also always some testing effort behind which I don't know how much you have to do from your side.

Also there is: #3925
Just randomly, I found also that DLSYM halpr_find_comp_by_name https://github.com/hdiethelm/linuxcnc-fork/blob/rtapi_cleanup_v2/src/rtapi/uspace_rtapi_main.cc#L112 does not match the signature (char* vs const char*),

I was now wondering if there is not a better solution for this. Often I use the following pattern to define the function and the matching type in the same header and then from then on only FUN_T* for function pointers. So you naturally change both. By some define magic, it should be possible do do both in one, but then it gets hard to read.

typedef int FUN_T(void *, void *);
int fun(void *, void *);

You know of a good way to test if both definitions match?

It would also be nicer to check at compile time if a .so has the needed main/exit functions.

This reverts commit a24f173.

From libuspace-* to liblinuxcnc-uspace-*

BsAtHome reviewed Apr 12, 2026

View reviewed changes

hdiethelm added 9 commits April 13, 2026 21:39

Reapply "Rtapi cleanup"

7df21d0

This reverts commit a24f173.

Cleanup: Library doesn't need a hardcoded path

bdbf604

Cleanup: Get rid of global task_array and add static where possible

352c782

Cleanup: Get rid of globals ruid/euid

98b45d6

Cleanup: Rename libs

4c1a0de

From libuspace-* to liblinuxcnc-uspace-*

Cleanup: Get rid of globals find_rt_cpu_number / set_namef

67d61ba

Cleanup: Review: const std::string &

1ed7649

Cleanup: Correct sizeof()

26ea00a

Cleanup: Correct _exit

f165b58

hdiethelm force-pushed the rtapi_cleanup_v2 branch from 44ab357 to f165b58 Compare April 13, 2026 19:44

hdiethelm added 2 commits April 13, 2026 22:24

Cleanup: Review: get_fifo_path nicer

9dfe1e2

Cleanup: Review: Fix timeout

75925a7

		len = snprintf(buf + 1, bufsize - 1, "%s", s.c_str());
		return len;

Conversation

hdiethelm commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hdiethelm commented Apr 9, 2026

Uh oh!

NTULINUX commented Apr 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hdiethelm commented Apr 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hdiethelm commented Apr 11, 2026

Uh oh!

NTULINUX commented Apr 11, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hdiethelm Apr 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

hdiethelm commented Apr 9, 2026 •

edited

Loading

NTULINUX commented Apr 11, 2026 •

edited

Loading

hdiethelm commented Apr 11, 2026 •

edited

Loading

hdiethelm Apr 12, 2026 •

edited

Loading