[SSSD] [PATCH] libsss_dbus
Pavel Březina
pbrezina at redhat.com
Fri May 2 10:17:15 UTC 2014
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patches.tgz
Type: application/x-compressed-tar
Size: 42080 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140502/9153475a/attachment.bin>
More information about the sssd-devel
mailing list