This is the first phase of the merging of Monitor and Operation threads in NativeProcessLinux
(which is necessary since the two threads race inside Linux kernel). Here, I reimplement the
Monitor thread do use non-blocking waitpid calls, which enables later addition of code from the
operation thread.
Details
Diff Detail
Event Timeline
Please see my comments.
source/Plugins/Process/Linux/NativeProcessLinux.cpp | ||
---|---|---|
1158 | There is no likelihood of partial read here, right? | |
1173 | Could you split this method into 2 smaller methods - for example, ReadSignal and ProcessSignal? | |
1248 | Should we return from this call if size == 0? | |
1250 | Could you use 'x' to check for exit command? | |
1275 | s/NativeProcessLinux::/NativeProcessLinux::Monitor | |
1283 | What do you think about handling commands prior to signals? Otherwise, if there is constant stream of signals exit may take time. | |
4007 | s/NULL/nullptr | |
4011 | "Process attach" - is it still relevant in this context? | |
4021 | You may return here if !IsJoinable(). |
source/Plugins/Process/Linux/NativeProcessLinux.cpp | ||
---|---|---|
1248–1254 | Why do we need this switch here? | |
1270 | (nit: ::select()) | |
3998 | Please use a unique_ptr and do a release when we pass ownership to the other thread. I would suggest to use the same semantics for closing monitro_fd also. | |
source/Plugins/Process/Linux/NativeProcessLinux.h | ||
200 | Do we need to define this class in the header file? |
source/Plugins/Process/Linux/NativeProcessLinux.cpp | ||
---|---|---|
1158 | Reading from the fd actually handles the signal, so it would be very strange to "partially" handle it. I have added this logging statement just in case... | |
1173 | Done. | |
1248 | I have changed this to use end-of-file signal (size==0) as the exit signal. | |
1248–1254 | That was me preparing for more commands in later patches. In any case, it is gone now... | |
1250 | I will use end-of-file as exit command. This means there are no supported commands (yet). Changed to log if we receive an unknown command. | |
1270 | What is the policy on prefixing stuff with :: ? | |
1275 | Good catch. | |
1283 | Sounds unlikely, but I can do that just in case. | |
3998 | That's not as easy as it looks like. Let's say I put the monitor in a unique_ptr and I call LaunchThread(..., monitor_up.release(), ...). I was actually looking for a file descriptor RAII class, but I could not find any. LLVM has ScopedHandle for windows file handles, but there is no linux/posix equivalent. The best solution would be to add one to llvm, but I did not want to go that way at the moment. :( | |
4007 | Done. | |
4011 | Good point, rephrasing. | |
4021 | Refactored. | |
source/Plugins/Process/Linux/NativeProcessLinux.h | ||
200 | It needs access to private methods of NativeProcessLinux (MonitorCallback, at least). |
- respond to review comments
- make pipe fds synchronous (I never relied on them async in the first place)
source/Plugins/Process/Linux/NativeProcessLinux.cpp | ||
---|---|---|
1270 | I don't know (I usually use them with :: to avoid any name conflict in the future but I think we don't have any policy). | |
3998 | I would use a unique_ptr like this (it is cleaner for me but you can leave it as it is now if you like it more): std::unique_ptr<Monitor> monitor(new Monitor(...)) ... m_monitor_thread = ThreadLauncher::LaunchThread("monitor", Monitor::RunMonitor, monitor.get(), NULL); if (m_monitor_thread.IsJoinable()) monitor.release(); else { ... } For the file handle I would suggest to create a RAII based on std::unique_ptr with a lambda here and use it next to the monitor unique_ptr. auto fd_deleter = [](int* fd){close(*fd); *fd = -1;} std::unique_ptr<int, decltype(fd_deleter)> monitor_fd_scope(&m_monitor_fd); |
source/Plugins/Process/Linux/NativeProcessLinux.cpp | ||
---|---|---|
3998 | I have redesigned this a bit. Now the Monitor is owned (via a unique_ptr) by the NativeProcessLinux object, and all the destruction (threads, file descriptors...) happens in the Monitor destructor, which sounds more RAII. What do you make of this? |
Do we need to define this class in the header file?