Add to log message which plugin failed to load. #2571
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This changes a message like: [wayfire.git/src/core/plugin-loader.cpp:87] error loading plugin: /usr/lib/libcwd_r.so.9: undefined symbol: elf_nextscn into: [wayfire.git/src/core/plugin-loader.cpp:87] error loading plugin [/usr/lib/wayfire/libpixdecor.so]: /usr/lib/libcwd_r.so.9: undefined symbol: elf_nextscn In other words, not just print the dlerror, but print which plugin failed to load.
Fix fixes the compile warning,
../../../../github/wayfire/wayfire.git/src/debug.cpp: In function ‘std::string read_output(std::string)’:
../../../../github/wayfire/wayfire.git/src/debug.cpp:95:10: warning: ignoring return value of ‘char* fgets(char*, int, FILE*)’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
95 | fgets(buffer, MAX_FUNCTION_NAME, file);
| ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In this case there is a potential problem: if gets returns NULL,
then nothing was read and the buffer is uninitialized. Not only
would this function be returning uninitialized data, an arbitrary
amount of memory could be allocated into `line` (and be returned)
when `std::string line = buffer;` is searching for a terminating
zero in the uninitialized buffer.
I kinda rewrote this function (while I was at it) to not write
first to the stack and then copy that into `line`. Instead the
`std::string` is created with the same space as the buffer was,
and then we write directly into the allocated memory. Afterwards
a `resize()` sets `line` to the correct size - removing any trailing
newline at the same time.
In other words, this code is much faster too (as if that was
needed, but alas)!
|
I tried to create a new PR, but because the previous one isn't merged yet - it seems I can only append to this one :/. |
soreau
reviewed
Feb 7, 2025
| @@ -84,7 +84,7 @@ std::pair<void*, void*> wf::get_new_instance_handle(const std::string& path) | |||
| void *handle = dlopen(path.c_str(), RTLD_NOW | RTLD_GLOBAL); | |||
| if (handle == NULL) | |||
| { | |||
| LOGE("error loading plugin: ", dlerror()); | |||
| LOGE("error loading plugin [", path, "]: ", dlerror()); | |||
There was a problem hiding this comment.
This appears to be the only change that the PR advertises. I'm not sure what the other changes are about, but they don't seem that they should be a part of this pull request.
There was a problem hiding this comment.
That is correct. That was the first commit and PR of four days ago.
After that I made two more PR requests, but there is no way to make separate PR's. It merges everything into this one. I thought that soreau told me to just keep adding stuff, maybe I misunderstood?
There was a problem hiding this comment.
The PR should be about an idea. If a commit is not part of the idea, it should go into a separate PR. If you have one PR that depends on another, you can submit the first one and then in the subsequent PRs, put a message that it depends on the first PR (number). Of course the dependent PRs won't build in CI until the first one is merged, but after it is, we can simply rerun CI and most times, you will not need to take further action.
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.
|
I'm closing this PR because I'll split it up into three different PR's. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changes a message like:
[wayfire.git/src/core/plugin-loader.cpp:87] error loading plugin: /usr/lib/libcwd_r.so.9: undefined symbol: elf_nextscn
into:
[wayfire.git/src/core/plugin-loader.cpp:87] error loading plugin [/usr/lib/wayfire/libpixdecor.so]: /usr/lib/libcwd_r.so.9: undefined symbol: elf_nextscn
In other words, not just print the dlerror, but print which plugin failed to load.