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.