[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/ce74898f/attachment.bin>


More information about the sssd-devel mailing list