[SSSD] [PATCH] libsss_dbus
Lukas Slebodnik
lslebodn at redhat.com
Fri May 2 11:56:01 UTC 2014
On (02/05/14 12:17), Pavel Březina wrote:
>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.
>
I now, but I wanted to catched any problem in new library, so I enabled all
warnings :-) And for simplification I attached patches.
>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.
Patches look good to me.
Code coverage of library is also nice.
good job.
Reading tracefile ./libsss_dbus.info
|Lines |Functions |
Filename |Rate Num|Rate Num|
===================================================
[sssd/src/lib/dbus/]
sss_dbus.c |79.7% 276|95.2% 21|
sss_dbus_attrs.c |81.8% 66|84.2% 19|
sss_dbus_common.c |71.4% 63|66.7% 6|
sss_dbus_parser.c |68.0% 300| 100% 10|
sss_dbus_utils.c |93.3% 30| 100% 3|
===================================================
Total:|75.0% 735|89.8% 59|
It will be nice to have a review from another developer,
LS
Attaching patch for dlopen test :-)
-------------- next part --------------
>From 9d8ef86f2ba3655428230f41f06097484f60ee0c Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Fri, 2 May 2014 13:43:05 +0200
Subject: [PATCH] TEST: Add libsss_dbus.so to dlopen test
---
src/tests/dlopen-tests.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/tests/dlopen-tests.c b/src/tests/dlopen-tests.c
index 7a218c70ad5ccc640388ac6b933299ceaae384ca..01f96109f27d3827a76080e5caf6985392ff7004 100644
--- a/src/tests/dlopen-tests.c
+++ b/src/tests/dlopen-tests.c
@@ -43,6 +43,9 @@ struct so {
{ "libsss_nss_idmap.so", { LIBPFX"libsss_nss_idmap.so", NULL } },
{ "libnss_sss.so", { LIBPFX"libnss_sss.so", NULL } },
{ "pam_sss.so", { LIBPFX"pam_sss.so", NULL } },
+#ifdef BUILD_IFP
+ { "libsss_dbus.so", { LIBPFX"libsss_dbus.so", NULL } },
+#endif /* BUILD_IFP */
#ifdef BUILD_SUDO
{ "libsss_sudo.so", { LIBPFX"libsss_sudo.so", NULL } },
#endif
--
1.9.0
More information about the sssd-devel
mailing list