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.