| Message ID | 20211022031456.373471-1-hj.tedd.an@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Luiz Von Dentz |
| Headers | show |
| Series | [BlueZ] src/device: Fix glib assertion | expand |
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=568303
---Test result---
Test Summary:
CheckPatch FAIL 0.47 seconds
GitLint FAIL 0.31 seconds
Prep - Setup ELL PASS 46.06 seconds
Build - Prep PASS 0.24 seconds
Build - Configure PASS 8.38 seconds
Build - Make PASS 201.97 seconds
Make Check PASS 8.62 seconds
Make Distcheck PASS 240.72 seconds
Build w/ext ELL - Configure PASS 8.57 seconds
Build w/ext ELL - Make PASS 191.77 seconds
Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script with rule in .checkpatch.conf
Output:
[BlueZ] src/device: Fix glib assertion
WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#87:
bluetoothd[370193]: GLib: g_file_set_contents: assertion 'error == NULL || *error == NULL' failed
/github/workspace/src/12576965.patch total: 0 errors, 1 warnings, 7 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
/github/workspace/src/12576965.patch has style problems, please review.
NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
##############################
Test: GitLint - FAIL
Desc: Run gitlint with rule in .gitlint
Output:
[BlueZ] src/device: Fix glib assertion
7: B1 Line exceeds max length (97>80): "bluetoothd[370193]: GLib: g_file_set_contents: assertion 'error == NULL || *error == NULL' failed"
9: B1 Line exceeds max length (105>80): "bluetoothd[370193]: #1 g_logv+0x21c (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6) [0x7f314559455c]"
10: B1 Line exceeds max length (103>80): "bluetoothd[370193]: #2 g_log+0x93 (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6) [0x7f3145594743]"
11: B1 Line exceeds max length (117>80): "bluetoothd[370193]: #3 g_file_set_contents+0x68 (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6) [0x7f3145577a68]"
12: B1 Line exceeds max length (86>80): "bluetoothd[370193]: #4 store_device_info_cb+0x3b6 (src/device.c:475) [0x55be0a3591e6]"
13: B1 Line exceeds max length (122>80): "bluetoothd[370193]: #5 g_main_context_dispatch+0x14e (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6) [0x7f314558d04e]"
14: B1 Line exceeds max length (95>80): "bluetoothd[370193]: #6 ?? (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6) [0x7f314558d400]"
15: B1 Line exceeds max length (113>80): "bluetoothd[370193]: #7 g_main_loop_run+0x83 (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6) [0x7f314558d6f3]"
16: B1 Line exceeds max length (90>80): "bluetoothd[370193]: #8 mainloop_run+0x19 (src/shared/mainloop-glib.c:68) [0x55be0a394149]"
17: B1 Line exceeds max length (105>80): "bluetoothd[370193]: #9 mainloop_run_with_signal+0xf0 (src/shared/mainloop-notify.c:190) [0x55be0a3945b0]"
19: B1 Line exceeds max length (89>80): "bluetoothd[370193]: #11 __libc_start_main+0xf3 (../csu/libc-start.c:342) [0x7f314521e0b3]"
20: B1 Line exceeds max length (95>80): "bluetoothd[370193]: #12 _start+0x2e (/home/han1/work/dev/bluez/src/bluetoothd) [0x55be0a2dac7e]"
---
Regards,
Linux Bluetooth
Hi Tedd, On 2021-10-21 20:14:56, Tedd Ho-Jeong An wrote: > From: Tedd Ho-Jeong An <tedd.an@intel.com> > > This patch fixes the assertion by resetting the GError variable to NULL. > > bluetoothd[370193]: GLib: g_file_set_contents: assertion 'error == NULL || *error == NULL' failed > bluetoothd[370193]: ++++++++ backtrace ++++++++ > bluetoothd[370193]: #1 g_logv+0x21c (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6) [0x7f314559455c] > bluetoothd[370193]: #2 g_log+0x93 (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6) [0x7f3145594743] > bluetoothd[370193]: #3 g_file_set_contents+0x68 (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6) [0x7f3145577a68] > bluetoothd[370193]: #4 store_device_info_cb+0x3b6 (src/device.c:475) [0x55be0a3591e6] > bluetoothd[370193]: #5 g_main_context_dispatch+0x14e (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6) [0x7f314558d04e] > bluetoothd[370193]: #6 ?? (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6) [0x7f314558d400] > bluetoothd[370193]: #7 g_main_loop_run+0x83 (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6) [0x7f314558d6f3] > bluetoothd[370193]: #8 mainloop_run+0x19 (src/shared/mainloop-glib.c:68) [0x55be0a394149] > bluetoothd[370193]: #9 mainloop_run_with_signal+0xf0 (src/shared/mainloop-notify.c:190) [0x55be0a3945b0] > bluetoothd[370193]: #10 main+0xabc (src/main.c:1212) [0x55be0a2d9d9c] > bluetoothd[370193]: #11 __libc_start_main+0xf3 (../csu/libc-start.c:342) [0x7f314521e0b3] > bluetoothd[370193]: #12 _start+0x2e (/home/han1/work/dev/bluez/src/bluetoothd) [0x55be0a2dac7e] > bluetoothd[370193]: +++++++++++++++++++++++++++ Is it desired to have a `Fixes: 6a154cd08 ("device: Fix unchecked return value")` annotation for BlueZ patches, just like is done for the kernel? > --- > src/device.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/device.c b/src/device.c > index d5aae9576..f3cd69003 100644 > --- a/src/device.c > +++ b/src/device.c > @@ -399,6 +399,7 @@ static gboolean store_device_info_cb(gpointer user_data) > error("Unable to load key file from %s: (%s)", filename, > gerr->message); > g_error_free(gerr); > + gerr = NULL; You should also correct all the other g_error_free paths that were introduced recently, whenever this gerr pointer is reused. I'm running into this assertion in quite a few places such as update_bredr_services. Note that Luiz fixed this in a different way for store_device_info_cb in 8b6b9b775 ("adapter: Fix storing IRK causing invalid read") by actually bailing on error, instead of merely printing it and continuing. When those "Fix unchecked return value" patches were made, was every call-site evaluated to need a return instead of continuing? That's the intended action of those coverity warnings, not just printing them for the sake of it. I would have sent patches to correct this, but as shown in Luiz' patch linked above the desired error-handling behaviour will differ per calling-site. As far as I'm aware some of these errors such as the key file not yet existing are normal, but invalid file contents are not (hence why a create_file seems to be performed instead of allowing all errors through). This also seems to be the desired solution for update_bredr_services. Who's responsible for going through all the `Fix unchecked return value` patches and cleaning up their error-handling properly akin to 8b6b9b775? > } > > g_key_file_set_string(key_file, "General", "Name", device->name); > -- > 2.25.1 > - Marijn
diff --git a/src/device.c b/src/device.c index d5aae9576..f3cd69003 100644 --- a/src/device.c +++ b/src/device.c @@ -399,6 +399,7 @@ static gboolean store_device_info_cb(gpointer user_data) error("Unable to load key file from %s: (%s)", filename, gerr->message); g_error_free(gerr); + gerr = NULL; } g_key_file_set_string(key_file, "General", "Name", device->name);
From: Tedd Ho-Jeong An <tedd.an@intel.com> This patch fixes the assertion by resetting the GError variable to NULL. bluetoothd[370193]: GLib: g_file_set_contents: assertion 'error == NULL || *error == NULL' failed bluetoothd[370193]: ++++++++ backtrace ++++++++ bluetoothd[370193]: #1 g_logv+0x21c (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6) [0x7f314559455c] bluetoothd[370193]: #2 g_log+0x93 (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6) [0x7f3145594743] bluetoothd[370193]: #3 g_file_set_contents+0x68 (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6) [0x7f3145577a68] bluetoothd[370193]: #4 store_device_info_cb+0x3b6 (src/device.c:475) [0x55be0a3591e6] bluetoothd[370193]: #5 g_main_context_dispatch+0x14e (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6) [0x7f314558d04e] bluetoothd[370193]: #6 ?? (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6) [0x7f314558d400] bluetoothd[370193]: #7 g_main_loop_run+0x83 (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.6) [0x7f314558d6f3] bluetoothd[370193]: #8 mainloop_run+0x19 (src/shared/mainloop-glib.c:68) [0x55be0a394149] bluetoothd[370193]: #9 mainloop_run_with_signal+0xf0 (src/shared/mainloop-notify.c:190) [0x55be0a3945b0] bluetoothd[370193]: #10 main+0xabc (src/main.c:1212) [0x55be0a2d9d9c] bluetoothd[370193]: #11 __libc_start_main+0xf3 (../csu/libc-start.c:342) [0x7f314521e0b3] bluetoothd[370193]: #12 _start+0x2e (/home/han1/work/dev/bluez/src/bluetoothd) [0x55be0a2dac7e] bluetoothd[370193]: +++++++++++++++++++++++++++ --- src/device.c | 1 + 1 file changed, 1 insertion(+)