[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