-
Notifications
You must be signed in to change notification settings - Fork 183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle syscall failures in wf::compositor_core_impl_t::run #2575
Handle syscall failures in wf::compositor_core_impl_t::run #2575
Conversation
0e6c39d
to
f55e0cb
Compare
Force pushed because I rebased after merging with master (wf-touch update). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall lgtm, but there are a few smaller things to fix
This patch address the compiler warnings: [455/476] Compiling C++ object src/liblibwayfire.a.p/core_core.cpp.o ../../../../github/wayfire/wayfire.git/src/core/core.cpp: In member function ‘virtual pid_t wf::compositor_core_impl_t::run(std::string)’: ../../../../github/wayfire/wayfire.git/src/core/core.cpp:433:10: warning: ignoring return value of ‘int pipe2(int*, int)’ declared with attribute ‘warn_unused_result’ [-Wunused-result] 433 | pipe2(pipe_fd, O_CLOEXEC); | ~~~~~^~~~~~~~~~~~~~~~~~~~ ../../../../github/wayfire/wayfire.git/src/core/core.cpp:467:18: warning: ignoring return value of ‘ssize_t write(int, const void*, size_t)’ declared with attribute ‘warn_unused_result’ [-Wunused-result] 467 | write(pipe_fd[WRITE_END], (void*)(&pid), sizeof(pid)); | ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../../../github/wayfire/wayfire.git/src/core/core.cpp:479:13: warning: ignoring return value of ‘ssize_t read(int, void*, size_t)’ declared with attribute ‘warn_unused_result’ [-Wunused-result] 479 | read(pipe_fd[READ_END], &child_pid, sizeof(child_pid)); | ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In order to "handle" a failure to run a command, I'd normally throw an exception that is caught up stream and pops up an informative window to the user describing the error in detail... but this code is kinda C-like, so I opted to just return 0 as the `pid_t`. The actual PID of a client is NEVER 0, so we can use this perfectly to detect failure. Why handling failure is correct: - If pipe2 would fail (which is possible), it leaves `pipe_fd` untouched and the rest of the code would use uninitialed memory and have Undefined Behaviour. - If write would fail, then the command just failed: we can't report back a PID. - Same for the read: if we fail to read the PID then we can't return its value. Returning 0 and handling that as an error (failure to execute the command) makes sense. - The return value of the executed script was also completely ignored. If the command being run return an non-zero status than now the command is also treated as having failed. It is also possible to never even get to the _exit inside the child. To detect those, WIFEXITED(status) && WEXITSTATUS(status) == 0 are being used. More sophisticated error handling is possible, but unnecessary: failure never happened so far (apparently) and is just very unlikely in general (apart from perhaps trying to execute non-existing scripts), so what we exactly do isn't that important (as long as I got rid of the pesky compiler warnings). Treating every error as a failure to execute the command is OK. Given that `wf::get_core().run(...)` can now return 0 with the meaning that executing the command failed, we also need to handle that gracefully, of course. Hence that `command_callback` now returns a bool and converts the pid_t into a bool (so that zero now means failure) whenever `wf::compositor_core_impl_t::run` is executed. plugins/ipc/ipc.cpp, plugins/ipc/ipc.hpp, plugins/ipc/ipc-method-repository.hpp: `wf::ipc::client_t::send_json` now returns a bool (success). plugins/single_plugins/command.cpp: `command_callback` now returns a bool (success). plugins/ipc/stipc.cpp (stipc_plugin_t::run): Do not report a PID of zero as if that is the PID. src/core/core.cpp: handle syscall errors and return (pid_t)0 upon failure.
f55e0cb
to
bb74450
Compare
Did I deal with the change requests properly (independent of if I resolved them with the commit), I just mean the clicking here on this github page? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now, will test it shortly & merge, thanks!
This patch address the compiler warnings:
[455/476] Compiling C++ object src/liblibwayfire.a.p/core_core.cpp.o ../../../../github/wayfire/wayfire.git/src/core/core.cpp: In member function ‘virtual pid_t wf::compositor_core_impl_t::run(std::string)’: ../../../../github/wayfire/wayfire.git/src/core/core.cpp:433:10: warning: ignoring return value of ‘int pipe2(int*, int)’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
433 | pipe2(pipe_fd, O_CLOEXEC);
| ~~~~~^~~~~~~~~~~~~~~~~~~~
../../../../github/wayfire/wayfire.git/src/core/core.cpp:467:18: warning: ignoring return value of ‘ssize_t write(int, const void*, size_t)’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
467 | write(pipe_fd[WRITE_END], (void*)(&pid), sizeof(pid));
| ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../../github/wayfire/wayfire.git/src/core/core.cpp:479:13: warning: ignoring return value of ‘ssize_t read(int, void*, size_t)’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
479 | read(pipe_fd[READ_END], &child_pid, sizeof(child_pid));
| ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In order to "handle" a failure to run a command, I'd normally throw an exception that is caught up stream and pops up an informative window to the user describing the error in detail... but this code is kinda C-like, so I opted to just return 0 as the
pid_t
. The actual PID of a client is NEVER 0, so we can use this perfectly to detect failure.Why handling failure is correct:
If pipe2 would fail (which is possible), it leaves
pipe_fd
untouched and the rest of the code would use uninitialed memory and have Undefined Behaviour.If write would fail, then the command just failed: we can't report back a PID.
Same for the read: if we fail to read the PID then we can't return its value. Returning 0 and handling that as an error (failure to execute the command) makes sense.
The return value of the executed script was also completely ignored. If the command being run return an non-zero status than now the command is also treated as having failed. It is also possible to never even get to the _exit inside the child. To detect those, WIFEXITED(status) && WEXITSTATUS(status) == 0 are being used.
More sophisticated error handling is possible, but unnecessary: failure never happened so far (apparently) and is just very unlikely in general (apart from perhaps trying to execute non-existing scripts), so what we exactly do isn't that important (as long as I got rid of the pesky compiler warnings). Treating every error as a failure to execute the command is OK.
Given that
wf::get_core().run(...)
can now return 0 with the meaning that executing the command failed, we also need to handle that gracefully, of course. Hence thatcommand_callback
now returns a bool and converts the pid_t into a bool (so that zero now means failure) wheneverwf::compositor_core_impl_t::run
is executed.plugins/ipc/ipc.cpp, plugins/ipc/ipc.hpp, plugins/ipc/ipc-method-repository.hpp:
wf::ipc::client_t::send_json
now returns a bool (success).plugins/single_plugins/command.cpp:
command_callback
now returns a bool (success).plugins/ipc/stipc.cpp (stipc_plugin_t::run):
Do not report a PID of zero as if that is the PID.
src/core/core.cpp:
handle syscall errors and return (pid_t)0 upon failure.