On 05/02/2014 10:19 AM, Lukas Slebodnik wrote:
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.
Hi,
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.
This seems to be a false positive, however I silenced it by setting ret = SSS_DBUS_OK explicitly after the for cycle.
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
Thanks. I am unable to build rpms for some time so I'm always blind in these changes.
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.
It is a missing semicolon - great catch!
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.
There are many signedness warnings in SSSD so I'm building without this warning. But you are right, I should check new code also for this one. I also fixed these warning in unit tests.
Thank you very much for your review. New patches are attached.
LS
PS: It will be nice to have a review from another developer, because this patchset will introduce new public library.