xcpmd: Avoid corrupting libevent & infinite loop #75
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
Port the patch provided to include vglass support for xcpmd. Signed-off-by: Eric Chanudet <[email protected]>
Add vglass built sources stubs for the RPC generator. Signed-off-by: Eric Chanudet <[email protected]>
libdmbus was used to pass information between some OpenXT device-model components, mostly surfman and similar. This is no longer in use. Signed-off-by: Eric Chanudet <[email protected]>
vglass: replace surfman RPC in xcpmd, retire libdmbus
Update for the new vglass idl which pass the battery percentage in battery_status_changed, and the ac adapter state in ac_adapter_state_changed. This lets vglass update it's banner battery information. Signed-off-by: Jason Andryuk <[email protected]>
xcpmd: Use vglass idl dbus signals
varstored-watch is a helper binary that starts and monitors an instance of varstored. OXT-1826 Signed-off-by: Nicholas Tsirakis <[email protected]>
[guest-efi] Add varstored-watch
Compiling with security flags enabled, we have the following error: varstored-watch.c: In function 'start_varstored': varstored-watch.c:32:5: error: ignoring return value of 'asprintf', declared with attribute warn_unused_result [-Werror=unused-result] Check the asprintf return value and exit the program when it fails. Signed-off-by: Jason Andryuk <[email protected]>
With security_flags, we have this error: | In function 'strncpy', | inlined from 'acpi_events_initialize' at ../../../../../../../../openxt/xctools/xcpmd/src/acpi-events.c:597:5: | /home/build/openxt/build/tmp-glibc/work/core2-64-oe-linux/xcpmd/0+git999-r0/recipe-sysroot/usr/include/bits/string_fortified.h:106:10: error: '__builtin_strncpy' output truncated before terminating nul copying 21 bytes from a string of the same length [-Werror=stringop-truncation] | 106 | return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest)); | | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ We need strlen + 1 to write the terminating NUL, so do that and remove the manual '\0' assignment. Signed-off-by: Jason Andryuk <[email protected]>
With security_flags, building qmp_helper fails with: warning _FORTIFY_SOURCE requires compiling with optimization (-O) Bitbake is passing in -O2, but the = assignment in the Makefile means the passed in CFLAGS are dropped. Change to += to let the bitbake flags through. Signed-off-by: Jason Andryuk <[email protected]>
With security_flags, building atapi_pt_helper fails with: warning _FORTIFY_SOURCE requires compiling with optimization (-O) Bitbake is passing in -O2, but the = assignment in the Makefile means the passed in CFLAGS are dropped. Change to += to let the bitbake flags through. Signed-off-by: Jason Andryuk <[email protected]>
xcpmd polls the battery every 4 seconds. When running on battery, *something* will have changed in that time. Even on AC, the values have typically changes such that xcpmd is sending the DBus notification. However, the only thing that the UI cares about is the battery percentage. So check the percentage and only send it if has changed. This avoids xcpmd sending a message, but it also removes multiple DBus queries by the UI for more information. It's a little bit of a micro optimization, but it is also easy. Signed-off-by: Jason Andryuk <[email protected]>
Sometimes, logs show "xcpmd: Failed to read /sys/class/power_supply/BAT0/current_now". When this message is triggered, xcpmd is leaking FDs. Close them when that happens. There are other similar code paths that are also updated. Leaking the FDs, xcpmd can use up its FD limit and no longer be able to open the sysfs files. When that happens, there are no more battery updates. An HP laptop reports ENODEV for current_now when on battery power - those errors spam the log and are indicative of the leak scenario. Signed-off-by: Jason Andryuk <[email protected]>
Both update_battery_info & update_battery_status iterate through a battery's sysfs entries and pass the contents to set_battery_info_attribute and set_battery_status_attribute respectively. So the loop runs twice opening and reading files, but only half the entries are used in each loop. Consolidate them together into just update_battery_status that handles both battery_status and battery_into. This removes the redundent opening and reading of sysfs entries. Signed-off-by: Jason Andryuk <[email protected]>
ACPI batteries will return ENODEV when they don't have current_now data. This is seen with an HP Zbook when on battery power. It will spam the logs with: "Failed to read /sys/class/power_supply/BAT0/current_now". Look for ENODEV and suppress the log message in that case. Signed-off-by: Jason Andryuk <[email protected]>
Security Flags
xcpmd: Don't leak FDs
xcpmd: Send battery changed when the percent changes
commit a7fab04 "xcpmd: unify update_battery_info/status" broke xcpmd startup. It aborts with: double free or corruption (top) When update_battery_info() and update_battery_status() were merged, the two closedir() calls were retained. Only one call is correct. Remove the second that leads to the abort(). Fixed: a7fab04 "xcpmd: unify update_battery_info/status" Signed-off-by: Jason Andryuk <[email protected]>
xcpmd: Fix double closedir()
If xcpmd has two whenDarIdleTimeout() rules, during boot xcpmd goes into an infinite loop. xcpmd starts before vglass, so the DBus calls to input fail and the timer is needed to set a retry. The same struct timer is used for both whenDarIdleTimeout() calls, so the sequence of: event_set() evtimer_add() event_set() evtimer_add() acts on the single structure. The second call to event_set corrupts the already queued libevent structures. libevent gets stuck in an infinite loop in timeout_process(). Move the event_set() into add_timer_to_list() so it is only called once when allocating the structure. Also change to a single-shot mode by removing EV_PERSIST. set_timer already requeues itself when necessary. This then means evtimer_del() calls can be removed. Note: There is only a single idle-timeout from vglass, so only the most recent whenDarIdleTimeout() timeout will take affect. Signed-off-by: Jason Andryuk <[email protected]>
|
With the xcpmd inifinte loop, the UI stays waiting on xcpmd and the VM icons don't populate. |
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.
If xcpmd has two whenDarIdleTimeout() rules, during boot xcpmd goes into an infinite loop. xcpmd starts before vglass, so the DBus calls to input fail and the timer is needed to set a retry.
The same struct timer is used for both whenDarIdleTimeout() calls, so the sequence of:
event_set()
evtimer_add()
event_set()
evtimer_add()
acts on the single structure. The second call to event_set corrupts the already queued libevent structures. libevent gets stuck in an infinite loop in timeout_process().
Move the event_set() into add_timer_to_list() so it is only called once when allocating the structure.
Also change to a single-shot mode by removing EV_PERSIST. set_timer already requeues itself when necessary. This then means evtimer_del() calls can be removed.
Note: There is only a single idle-timeout from vglass, so only the most recent whenDarIdleTimeout() timeout will take affect.
Signed-off-by: Jason Andryuk [email protected]