On (01/05/14 17:39), Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/2254
Lukáš already did first round of review for build and packaging stuff. Thank you, I hope I have fixed all your concerns. There might be some more since I moved the library into libsss_dbus and libsss_dbus-devel packages.
The library is built if IFP responder is built.
There is a gcc warning in the first patch. (clang does not complain) src/lib/dbus/sss_dbus_parser.c: In function 'sss_dbus_parse_single_attr': src/lib/dbus/sss_dbus_parser.c:397:8: error: 'ret' may be used uninitialized in this function [-Werror=maybe-uninitialized] if (ret != SSS_DBUS_OK) { ^ src/lib/dbus/sss_dbus_parser.c:287:20: note: 'ret' was declared here sss_dbus_error ret; ^ cc1: all warnings being treated as errors
I initilised this variable to 0 to silence warning.
------------------------------- PATCH sss_dbus: build
%files libsss_dbus ^^^^^^^^^^^^^ rpmbuild expect package with name sssd-libss_dbus and it was not declared. %defattr(-,root,root,-) %{_libdir}/libsss_dbus.so* %{_libdir}/pkgconfig/sss_dbus.pc
Add "-n" before pacakge name. The same situatioin is with "%files libsss_dbus-devel" -%files libsss_dbus +%files -n libsss_dbus
--------------------------------- PATCH sss_dbus: add shortcuts for common use
@@ -101,7 +101,7 @@ sss_dbus_list_domains(sss_dbus_ctx *ctx, //snip
ret = sss_dbus_invoke_list(ctx, "Domains", &object_paths, DBUS_TYPE_INVALID); if (ret != SSS_DBUS_OK) { goto done; }
? /* calculate number of paths acquired and allocate memory for domains */ ? for (size = 0; object_paths[size] != NULL; size++) ? ? domains = _alloc(ctx, char *, size + 1); if (domains == NULL) { ret = SSS_DBUS_OUT_OF_MEMORY; goto done; }
You created memory leak with marked lines (only with default allocator) Another problem is off by one probelm.
Lines are "interpreted" as: for (size = 0; object_paths[size] != NULL; size++) { domains = _alloc(ctx, char *, size + 1); ^^^^^^^ domains are allocated few times, } // size was incremented after last allocation.
You created memory leak in test (tests use default allocator and not talloc) --- a/src/tests/cmocka/test_sss_dbus.c +++ b/src/tests/cmocka/test_sss_dbus.c @@ -1718,6 +1718,7 @@ void test_sss_dbus_list_domains(void **state)
assert_null(out[i]);
+ sss_dbus_free_string_array(ctx, &out); /* messages are unrefed in the library */ }
There were warning "warning: implicit conversion changes signedness" We do not have them enabled by default. I am attaching few patches, which can be squashed to your patchset. Patches shuold fix all mentioned problems in this mail.
LS
PS: It will be nice to have a review from another developer, because this patchset will introduce new public library.