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.
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.
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.
------------------------------- 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
--------------------------------- 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.
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.
LS
PS: It will be nice to have a review from another developer, because this patchset will introduce new public library.
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.
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.
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 :-)
On 01.05.2014 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.
As noted on IRC, I don't think this should be a public sssd API. As internal code of the OpenLMI IFP provider it makes sense.
But remember the public API *is* DBus itself. Callers of the sssd DBus API should use their local DBus facility in their language or toolkit to talk with sssd (ie: GDBus or python-dbus, or what have you).
I realize that sssd at present has no such local DBus facility in its toolkit (libdbus doesn't count, it's too low level) and that having such a client library internal to sssd makes sense. If it could be based on the codegen work that would be even better.
But, back to the point: I would caution against making it a public API and having it become an fixture that people turn to when trying to talk with SSSD.
OpenLMI already has means to talk DBus to various services. If implementing a provider as part of the OpenLMI project, those facilities should be used. On the other hand if the provider is part of sssd itself then there's no need for this code to be public API.
In addition, shouldn't such a general sssd DBus client helper library meet the needs of simplifying the internal P2P DBus calls inside of sssd itself? Jakub and I have done a lot of work to simplify implementing such calls, but invoking DBus methods remains as error prone and complex as ever.
Now that may be out of scope ... But if so, this should remain a part of the OpenLMI IFP provider for now, and not a public API of SSSD.
I'll review this as an internal provider code. If it was to be public generic extensible stable API there's a whole lot more issues, such as thread-safety, non-extensible structs, constness, mainloop async integration ... and other assumptions which prevent use beyond the scope of the few GetXxxxYyyy() ListXxxxYyyy() and related DBus methods.
On to specific review.
+sss_dbus_error +sss_dbus_invoke_list(sss_dbus_ctx *ctx, + const char *method, + char ***_object_paths, + int first_arg_type, + ...);
The DBus varargs format is really hard to get right. I would strongly caution propagating it further. In particular booleans and ints are tedious to pass in a platform-correct manner.
+const char * +sss_dbus_get_iface_for_object(const char *object_path);
This is an odd function. There are often (usually, if you count o.f.D.Properties) multiple interfaces at an object path in DBus. Among other things this is how a DBus API is extensible (versioned interfaces).
+sss_dbus_error +sss_dbus_send_message(sss_dbus_ctx *ctx, + DBusMessage *msg, + DBusMessage **_reply);
Is the blocking behavior intentional? You likely want to be able to at a minimum pass a timeout.
+ attrs = _alloc(ctx, sss_dbus_attr *, 2); + if (attrs == NULL) { + ret = SSS_DBUS_OUT_OF_MEMORY; + goto done; + } + + memset(attrs, '\0', sizeof(sss_dbus_attr *) * 2);
I see this pattern a lot. It would be less error prone to have an allocation macro which zeroed the allocated memory and use that when appropriate.
+#define get_attr(attrs, name, rtype, field, out) \
These sort of macros don't stand out as macros. They behave very unlike functions (causing the caller to return, etc...) so I would suggest using upper case or something like that.
+if BUILD_IFP +lib_LTLIBRARIES += libsss_dbus.la +pkgconfig_DATA += src/lib/dbus/sss_dbus.pc + +libsss_dbus_la_SOURCES = \ + src/lib/dbus/sss_dbus.c \ + src/lib/dbus/sss_dbus_attrs.c \ + src/lib/dbus/sss_dbus_parser.c \ + src/lib/dbus/sss_dbus_utils.c +libsss_dbus_la_LIBADD = \ + $(DBUS_LIBS) +libsss_dbus_la_LDFLAGS = \ + -Wl,--version-script,$(srcdir)/src/lib/dbus/sss_dbus.exports \ + -version-info 0:0:0 + +dist_noinst_DATA += src/lib/dbus/sss_dbus.exports + +include_HEADERS += \ + src/lib/dbus/sss_dbus.h +endif
See above...
+/** + * String dictionary + */ +typedef struct { + char *key; + char **values; + unsigned int num_values; +} sss_dbus_str_dict;
Shouldn't this be represented by a hash table on the client side?
Cheers,
Stef
On 05/05/2014 02:06 PM, Stef Walter wrote:
On 01.05.2014 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.
As noted on IRC, I don't think this should be a public sssd API. As internal code of the OpenLMI IFP provider it makes sense.
But remember the public API *is* DBus itself. Callers of the sssd DBus API should use their local DBus facility in their language or toolkit to talk with sssd (ie: GDBus or python-dbus, or what have you).
I realize that sssd at present has no such local DBus facility in its toolkit (libdbus doesn't count, it's too low level) and that having such a client library internal to sssd makes sense. If it could be based on the codegen work that would be even better.
But, back to the point: I would caution against making it a public API and having it become an fixture that people turn to when trying to talk with SSSD.
Do we want the InfoPipe provider to be used at all? Sure, big projects and skilled developers can stick with gdbus but why every single project should write the same hundreds lines of code to do the most common tasks? I think having such library will make using the InfoPipe a less pain.
Of course, this library does not completely hide the D-Bus for method calls, it is a compromise between universality and the need to completely reflect our D-Bus interface in this library. However, it still hides D-Bus completely when dealing with properties.
I want to keep the OpenLMI provider clean and bring as many things on SSSD side as possible. This library allows me to do it.
Sure, we don't have to make it a public API, even though it was the original intention. I can make it an OpenLMI client or put it completely into the OpenLMI provider tree.
OpenLMI already has means to talk DBus to various services. If implementing a provider as part of the OpenLMI project, those facilities should be used. On the other hand if the provider is part of sssd itself then there's no need for this code to be public API.
In addition, shouldn't such a general sssd DBus client helper library meet the needs of simplifying the internal P2P DBus calls inside of sssd itself? Jakub and I have done a lot of work to simplify implementing such calls, but invoking DBus methods remains as error prone and complex as ever.
The first design page for this library landed in February, the first code in March. Anyway, this library is a consumer of InfoPipe, I don't see much that can be shared.
Now that may be out of scope ... But if so, this should remain a part of the OpenLMI IFP provider for now, and not a public API of SSSD.
I think Sumit also wanted to use this library, so making it part of OpenLMI provider may break his plans.
Nevertheless, I still vote for making it public.
I'll review this as an internal provider code. If it was to be public generic extensible stable API there's a whole lot more issues, such as thread-safety, non-extensible structs, constness, mainloop async integration ... and other assumptions which prevent use beyond the scope of the few GetXxxxYyyy() ListXxxxYyyy() and related DBus methods.
On to specific review.
+sss_dbus_error +sss_dbus_invoke_list(sss_dbus_ctx *ctx,
const char *method,char ***_object_paths,int first_arg_type,...);The DBus varargs format is really hard to get right. I would strongly caution propagating it further. In particular booleans and ints are tedious to pass in a platform-correct manner.
+const char * +sss_dbus_get_iface_for_object(const char *object_path);
This is an odd function. There are often (usually, if you count o.f.D.Properties) multiple interfaces at an object path in DBus. Among other things this is how a DBus API is extensible (versioned interfaces).
I can make it a private function to get a default interface.
However, the current InfoPipe code does not work with interfaces and from my discussions with Jakub it seems it is not leaning that way.
+sss_dbus_error +sss_dbus_send_message(sss_dbus_ctx *ctx,
DBusMessage *msg,DBusMessage **_reply);Is the blocking behavior intentional?
Yes.
You likely want to be able to at a
minimum pass a timeout.
- attrs = _alloc(ctx, sss_dbus_attr *, 2);
- if (attrs == NULL) {
ret = SSS_DBUS_OUT_OF_MEMORY;goto done;- }
- memset(attrs, '\0', sizeof(sss_dbus_attr *) * 2);
I see this pattern a lot. It would be less error prone to have an allocation macro which zeroed the allocated memory and use that when appropriate.
+#define get_attr(attrs, name, rtype, field, out) \
These sort of macros don't stand out as macros. They behave very unlike functions (causing the caller to return, etc...) so I would suggest using upper case or something like that.
+if BUILD_IFP +lib_LTLIBRARIES += libsss_dbus.la +pkgconfig_DATA += src/lib/dbus/sss_dbus.pc
+libsss_dbus_la_SOURCES = \
- src/lib/dbus/sss_dbus.c \
- src/lib/dbus/sss_dbus_attrs.c \
- src/lib/dbus/sss_dbus_parser.c \
- src/lib/dbus/sss_dbus_utils.c
+libsss_dbus_la_LIBADD = \
- $(DBUS_LIBS)
+libsss_dbus_la_LDFLAGS = \
- -Wl,--version-script,$(srcdir)/src/lib/dbus/sss_dbus.exports \
- -version-info 0:0:0
+dist_noinst_DATA += src/lib/dbus/sss_dbus.exports
+include_HEADERS += \
- src/lib/dbus/sss_dbus.h
+endif
See above...
+/**
- String dictionary
- */
+typedef struct {
- char *key;
- char **values;
- unsigned int num_values;
+} sss_dbus_str_dict;
Shouldn't this be represented by a hash table on the client side?
Cheers,
Stef
Thanks for the review, I will comment on the other things later.
On Mon, May 05, 2014 at 05:08:39PM +0200, Pavel Březina wrote:
On 05/05/2014 02:06 PM, Stef Walter wrote:
On 01.05.2014 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.
As noted on IRC, I don't think this should be a public sssd API. As internal code of the OpenLMI IFP provider it makes sense.
But remember the public API *is* DBus itself. Callers of the sssd DBus API should use their local DBus facility in their language or toolkit to talk with sssd (ie: GDBus or python-dbus, or what have you).
I realize that sssd at present has no such local DBus facility in its toolkit (libdbus doesn't count, it's too low level) and that having such a client library internal to sssd makes sense. If it could be based on the codegen work that would be even better.
But, back to the point: I would caution against making it a public API and having it become an fixture that people turn to when trying to talk with SSSD.
Do we want the InfoPipe provider to be used at all? Sure, big projects and skilled developers can stick with gdbus but why every single project should write the same hundreds lines of code to do the most common tasks? I think having such library will make using the InfoPipe a less pain.
Of course, this library does not completely hide the D-Bus for method calls, it is a compromise between universality and the need to completely reflect our D-Bus interface in this library. However, it still hides D-Bus completely when dealing with properties.
I want to keep the OpenLMI provider clean and bring as many things on SSSD side as possible. This library allows me to do it.
Sure, we don't have to make it a public API, even though it was the original intention. I can make it an OpenLMI client or put it completely into the OpenLMI provider tree.
Also some developers were a bit nervous when we proposed that they need to interact with DBus directly at the C level or were outright rejecting talking to the system bus directly.
It's a bit of Pandora box, though, we'd need to be very strict about this new interface in case somebody comes with an RFE..the DBus interface is /the/ interface...
On 05/05/2014 05:25 PM, Jakub Hrozek wrote:
On Mon, May 05, 2014 at 05:08:39PM +0200, Pavel Březina wrote:
On 05/05/2014 02:06 PM, Stef Walter wrote:
On 01.05.2014 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.
As noted on IRC, I don't think this should be a public sssd API. As internal code of the OpenLMI IFP provider it makes sense.
But remember the public API *is* DBus itself. Callers of the sssd DBus API should use their local DBus facility in their language or toolkit to talk with sssd (ie: GDBus or python-dbus, or what have you).
I realize that sssd at present has no such local DBus facility in its toolkit (libdbus doesn't count, it's too low level) and that having such a client library internal to sssd makes sense. If it could be based on the codegen work that would be even better.
But, back to the point: I would caution against making it a public API and having it become an fixture that people turn to when trying to talk with SSSD.
Do we want the InfoPipe provider to be used at all? Sure, big projects and skilled developers can stick with gdbus but why every single project should write the same hundreds lines of code to do the most common tasks? I think having such library will make using the InfoPipe a less pain.
Of course, this library does not completely hide the D-Bus for method calls, it is a compromise between universality and the need to completely reflect our D-Bus interface in this library. However, it still hides D-Bus completely when dealing with properties.
I want to keep the OpenLMI provider clean and bring as many things on SSSD side as possible. This library allows me to do it.
Sure, we don't have to make it a public API, even though it was the original intention. I can make it an OpenLMI client or put it completely into the OpenLMI provider tree.
Also some developers were a bit nervous when we proposed that they need to interact with DBus directly at the C level or were outright rejecting talking to the system bus directly.
It's a bit of Pandora box, though, we'd need to be very strict about this new interface in case somebody comes with an RFE..the DBus interface is /the/ interface...
I can amend the documentation to make it more clear.
On Mon, May 05, 2014 at 05:38:58PM +0200, Pavel Březina wrote:
On 05/05/2014 05:25 PM, Jakub Hrozek wrote:
On Mon, May 05, 2014 at 05:08:39PM +0200, Pavel Březina wrote:
On 05/05/2014 02:06 PM, Stef Walter wrote:
On 01.05.2014 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.
As noted on IRC, I don't think this should be a public sssd API. As internal code of the OpenLMI IFP provider it makes sense.
But remember the public API *is* DBus itself. Callers of the sssd DBus API should use their local DBus facility in their language or toolkit to talk with sssd (ie: GDBus or python-dbus, or what have you).
I realize that sssd at present has no such local DBus facility in its toolkit (libdbus doesn't count, it's too low level) and that having such a client library internal to sssd makes sense. If it could be based on the codegen work that would be even better.
But, back to the point: I would caution against making it a public API and having it become an fixture that people turn to when trying to talk with SSSD.
Do we want the InfoPipe provider to be used at all? Sure, big projects and skilled developers can stick with gdbus but why every single project should write the same hundreds lines of code to do the most common tasks? I think having such library will make using the InfoPipe a less pain.
Of course, this library does not completely hide the D-Bus for method calls, it is a compromise between universality and the need to completely reflect our D-Bus interface in this library. However, it still hides D-Bus completely when dealing with properties.
I want to keep the OpenLMI provider clean and bring as many things on SSSD side as possible. This library allows me to do it.
Sure, we don't have to make it a public API, even though it was the original intention. I can make it an OpenLMI client or put it completely into the OpenLMI provider tree.
Also some developers were a bit nervous when we proposed that they need to interact with DBus directly at the C level or were outright rejecting talking to the system bus directly.
It's a bit of Pandora box, though, we'd need to be very strict about this new interface in case somebody comes with an RFE..the DBus interface is /the/ interface...
I can amend the documentation to make it more clear.
The idea was that this library just provide some lookup calls for common stuff like list of trusted domains and some user attributes which goes beyond the POSIX interface on top of the D-BUS API to facilitates adoption. It's a convenience library and there is no plan to wrap the whole D-BUS API in this library.
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On 05/05/2014 06:17 PM, Sumit Bose wrote:
On Mon, May 05, 2014 at 05:38:58PM +0200, Pavel Březina wrote:
On 05/05/2014 05:25 PM, Jakub Hrozek wrote:
On Mon, May 05, 2014 at 05:08:39PM +0200, Pavel Březina wrote:
On 05/05/2014 02:06 PM, Stef Walter wrote:
On 01.05.2014 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.
As noted on IRC, I don't think this should be a public sssd API. As internal code of the OpenLMI IFP provider it makes sense.
But remember the public API *is* DBus itself. Callers of the sssd DBus API should use their local DBus facility in their language or toolkit to talk with sssd (ie: GDBus or python-dbus, or what have you).
I realize that sssd at present has no such local DBus facility in its toolkit (libdbus doesn't count, it's too low level) and that having such a client library internal to sssd makes sense. If it could be based on the codegen work that would be even better.
But, back to the point: I would caution against making it a public API and having it become an fixture that people turn to when trying to talk with SSSD.
Do we want the InfoPipe provider to be used at all? Sure, big projects and skilled developers can stick with gdbus but why every single project should write the same hundreds lines of code to do the most common tasks? I think having such library will make using the InfoPipe a less pain.
Of course, this library does not completely hide the D-Bus for method calls, it is a compromise between universality and the need to completely reflect our D-Bus interface in this library. However, it still hides D-Bus completely when dealing with properties.
I want to keep the OpenLMI provider clean and bring as many things on SSSD side as possible. This library allows me to do it.
Sure, we don't have to make it a public API, even though it was the original intention. I can make it an OpenLMI client or put it completely into the OpenLMI provider tree.
Also some developers were a bit nervous when we proposed that they need to interact with DBus directly at the C level or were outright rejecting talking to the system bus directly.
It's a bit of Pandora box, though, we'd need to be very strict about this new interface in case somebody comes with an RFE..the DBus interface is /the/ interface...
I can amend the documentation to make it more clear.
The idea was that this library just provide some lookup calls for common stuff like list of trusted domains and some user attributes which goes beyond the POSIX interface on top of the D-BUS API to facilitates adoption. It's a convenience library and there is no plan to wrap the whole D-BUS API in this library.
Yep, that's not what it's doing.
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On 05/05/2014 12:28 PM, Pavel Březina wrote:
On 05/05/2014 06:17 PM, Sumit Bose wrote:
On Mon, May 05, 2014 at 05:38:58PM +0200, Pavel Březina wrote:
On 05/05/2014 05:25 PM, Jakub Hrozek wrote:
On Mon, May 05, 2014 at 05:08:39PM +0200, Pavel Březina wrote:
On 05/05/2014 02:06 PM, Stef Walter wrote:
On 01.05.2014 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.
As noted on IRC, I don't think this should be a public sssd API. As internal code of the OpenLMI IFP provider it makes sense.
But remember the public API *is* DBus itself. Callers of the sssd DBus API should use their local DBus facility in their language or toolkit to talk with sssd (ie: GDBus or python-dbus, or what have you).
I realize that sssd at present has no such local DBus facility in its toolkit (libdbus doesn't count, it's too low level) and that having such a client library internal to sssd makes sense. If it could be based on the codegen work that would be even better.
But, back to the point: I would caution against making it a public API and having it become an fixture that people turn to when trying to talk with SSSD.
Do we want the InfoPipe provider to be used at all? Sure, big projects and skilled developers can stick with gdbus but why every single project should write the same hundreds lines of code to do the most common tasks? I think having such library will make using the InfoPipe a less pain.
Of course, this library does not completely hide the D-Bus for method calls, it is a compromise between universality and the need to completely reflect our D-Bus interface in this library. However, it still hides D-Bus completely when dealing with properties.
I want to keep the OpenLMI provider clean and bring as many things on SSSD side as possible. This library allows me to do it.
Sure, we don't have to make it a public API, even though it was the original intention. I can make it an OpenLMI client or put it completely into the OpenLMI provider tree.
Also some developers were a bit nervous when we proposed that they need to interact with DBus directly at the C level or were outright rejecting talking to the system bus directly.
It's a bit of Pandora box, though, we'd need to be very strict about this new interface in case somebody comes with an RFE..the DBus interface is /the/ interface...
I can amend the documentation to make it more clear.
The idea was that this library just provide some lookup calls for common stuff like list of trusted domains and some user attributes which goes beyond the POSIX interface on top of the D-BUS API to facilitates adoption. It's a convenience library and there is no plan to wrap the whole D-BUS API in this library.
Yep, that's not what it's doing.
Yes the intent was to make it public so that it can be easily called from a python, ruby or java app to get missing data when an admin is defining roles in the context of the application. So please make it targeted and public. There will be consumers of this API quite soon: Foreman, oVirt, Keystone and others...
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On 06.05.2014 01:51, Dmitri Pal wrote:
On 05/05/2014 12:28 PM, Pavel Březina wrote:
On 05/05/2014 06:17 PM, Sumit Bose wrote:
On Mon, May 05, 2014 at 05:38:58PM +0200, Pavel Březina wrote:
On 05/05/2014 05:25 PM, Jakub Hrozek wrote:
On Mon, May 05, 2014 at 05:08:39PM +0200, Pavel Březina wrote:
On 05/05/2014 02:06 PM, Stef Walter wrote: > On 01.05.2014 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. > > As noted on IRC, I don't think this should be a public sssd API. As > internal code of the OpenLMI IFP provider it makes sense. > > But remember the public API *is* DBus itself. Callers of the sssd > DBus > API should use their local DBus facility in their language or > toolkit to > talk with sssd (ie: GDBus or python-dbus, or what have you).
> I realize that sssd at present has no such local DBus facility in > its > toolkit (libdbus doesn't count, it's too low level) and that > having such > a client library internal to sssd makes sense. If it could be > based on > the codegen work that would be even better. > > But, back to the point: I would caution against making it a > public API > and having it become an fixture that people turn to when trying > to talk > with SSSD.
Do we want the InfoPipe provider to be used at all? Sure, big projects and skilled developers can stick with gdbus but why every single project should write the same hundreds lines of code to do the most common tasks? I think having such library will make using the InfoPipe a less pain.
Of course, this library does not completely hide the D-Bus for method calls, it is a compromise between universality and the need to completely reflect our D-Bus interface in this library. However, it still hides D-Bus completely when dealing with properties.
I want to keep the OpenLMI provider clean and bring as many things on SSSD side as possible. This library allows me to do it.
Sure, we don't have to make it a public API, even though it was the original intention. I can make it an OpenLMI client or put it completely into the OpenLMI provider tree.
Also some developers were a bit nervous when we proposed that they need to interact with DBus directly at the C level or were outright rejecting talking to the system bus directly.
It's a bit of Pandora box, though, we'd need to be very strict about this new interface in case somebody comes with an RFE..the DBus interface is /the/ interface...
I can amend the documentation to make it more clear.
The idea was that this library just provide some lookup calls for common stuff like list of trusted domains and some user attributes which goes beyond the POSIX interface on top of the D-BUS API to facilitates adoption. It's a convenience library and there is no plan to wrap the whole D-BUS API in this library.
Then at a very minimum we should call this library "libsimpleifpclient.so" or something like that... Calling it libsssd_dbus.so is really confusing.
Yep, that's not what it's doing.
Yes the intent was to make it public so that it can be easily called from a python, ruby or java app to get missing data when an admin is defining roles in the context of the application. So please make it targeted and public. There will be consumers of this API quite soon: Foreman, oVirt, Keystone and others...
This is backwards.
It's easy to call DBus from python, java and friends. There's no need to pass that interaction through the bottleneck (albeit simple bottleneck) that this library provides. At the current time this library is not thread-safe, not loop-integratable, not-abi-extensible etc. It would need to look significantly different to provide the foundation for what you provide there. And why? The callers you suggest already have solid ways to call DBus. It's sssd/tevent that doesn't.
Stef
On 05/06/2014 06:58 AM, Stef Walter wrote:
On 06.05.2014 01:51, Dmitri Pal wrote:
On 05/05/2014 12:28 PM, Pavel Březina wrote:
On 05/05/2014 06:17 PM, Sumit Bose wrote:
On Mon, May 05, 2014 at 05:38:58PM +0200, Pavel Březina wrote:
On 05/05/2014 05:25 PM, Jakub Hrozek wrote:
On Mon, May 05, 2014 at 05:08:39PM +0200, Pavel Březina wrote: > On 05/05/2014 02:06 PM, Stef Walter wrote: >> On 01.05.2014 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. >> >> As noted on IRC, I don't think this should be a public sssd API. As >> internal code of the OpenLMI IFP provider it makes sense. >> >> But remember the public API *is* DBus itself. Callers of the sssd >> DBus >> API should use their local DBus facility in their language or >> toolkit to >> talk with sssd (ie: GDBus or python-dbus, or what have you). > >> I realize that sssd at present has no such local DBus facility in >> its >> toolkit (libdbus doesn't count, it's too low level) and that >> having such >> a client library internal to sssd makes sense. If it could be >> based on >> the codegen work that would be even better. >> >> But, back to the point: I would caution against making it a >> public API >> and having it become an fixture that people turn to when trying >> to talk >> with SSSD. > > Do we want the InfoPipe provider to be used at all? Sure, big > projects and skilled developers can stick with gdbus but why every > single project should write the same hundreds lines of code to do > the most common tasks? I think having such library will make using > the InfoPipe a less pain. > > Of course, this library does not completely hide the D-Bus for > method calls, it is a compromise between universality and the need > to completely reflect our D-Bus interface in this library. However, > it still hides D-Bus completely when dealing with properties. > > I want to keep the OpenLMI provider clean and bring as many things > on SSSD side as possible. This library allows me to do it. > > Sure, we don't have to make it a public API, even though it was the > original intention. I can make it an OpenLMI client or put it > completely into the OpenLMI provider tree.
Also some developers were a bit nervous when we proposed that they need to interact with DBus directly at the C level or were outright rejecting talking to the system bus directly.
It's a bit of Pandora box, though, we'd need to be very strict about this new interface in case somebody comes with an RFE..the DBus interface is /the/ interface...
I can amend the documentation to make it more clear.
The idea was that this library just provide some lookup calls for common stuff like list of trusted domains and some user attributes which goes beyond the POSIX interface on top of the D-BUS API to facilitates adoption. It's a convenience library and there is no plan to wrap the whole D-BUS API in this library.
Then at a very minimum we should call this library "libsimpleifpclient.so" or something like that... Calling it libsssd_dbus.so is really confusing.
Yep, that's not what it's doing.
Yes the intent was to make it public so that it can be easily called from a python, ruby or java app to get missing data when an admin is defining roles in the context of the application. So please make it targeted and public. There will be consumers of this API quite soon: Foreman, oVirt, Keystone and others...
This is backwards.
It's easy to call DBus from python, java and friends. There's no need to pass that interaction through the bottleneck (albeit simple bottleneck) that this library provides. At the current time this library is not thread-safe, not loop-integratable, not-abi-extensible etc. It would need to look significantly different to provide the foundation for what you provide there. And why? The callers you suggest already have solid ways to call DBus. It's sssd/tevent that doesn't.
It is thread safe as long as you use separate context for each thread.
Stef _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On 06.05.2014 09:06, Pavel Březina wrote:
On 05/06/2014 06:58 AM, Stef Walter wrote:
On 06.05.2014 01:51, Dmitri Pal wrote:
On 05/05/2014 12:28 PM, Pavel Březina wrote:
On 05/05/2014 06:17 PM, Sumit Bose wrote:
On Mon, May 05, 2014 at 05:38:58PM +0200, Pavel Březina wrote:
On 05/05/2014 05:25 PM, Jakub Hrozek wrote: > On Mon, May 05, 2014 at 05:08:39PM +0200, Pavel Březina wrote: >> On 05/05/2014 02:06 PM, Stef Walter wrote: >>> On 01.05.2014 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. >>> >>> As noted on IRC, I don't think this should be a public sssd >>> API. As >>> internal code of the OpenLMI IFP provider it makes sense. >>> >>> But remember the public API *is* DBus itself. Callers of the sssd >>> DBus >>> API should use their local DBus facility in their language or >>> toolkit to >>> talk with sssd (ie: GDBus or python-dbus, or what have you). >> >>> I realize that sssd at present has no such local DBus facility in >>> its >>> toolkit (libdbus doesn't count, it's too low level) and that >>> having such >>> a client library internal to sssd makes sense. If it could be >>> based on >>> the codegen work that would be even better. >>> >>> But, back to the point: I would caution against making it a >>> public API >>> and having it become an fixture that people turn to when trying >>> to talk >>> with SSSD. >> >> Do we want the InfoPipe provider to be used at all? Sure, big >> projects and skilled developers can stick with gdbus but why every >> single project should write the same hundreds lines of code to do >> the most common tasks? I think having such library will make using >> the InfoPipe a less pain. >> >> Of course, this library does not completely hide the D-Bus for >> method calls, it is a compromise between universality and the need >> to completely reflect our D-Bus interface in this library. However, >> it still hides D-Bus completely when dealing with properties. >> >> I want to keep the OpenLMI provider clean and bring as many things >> on SSSD side as possible. This library allows me to do it. >> >> Sure, we don't have to make it a public API, even though it was the >> original intention. I can make it an OpenLMI client or put it >> completely into the OpenLMI provider tree. > > Also some developers were a bit nervous when we proposed that they > need > to interact with DBus directly at the C level or were outright > rejecting > talking to the system bus directly. > > It's a bit of Pandora box, though, we'd need to be very strict about > this new interface in case somebody comes with an RFE..the DBus > interface is /the/ interface...
I can amend the documentation to make it more clear.
The idea was that this library just provide some lookup calls for common stuff like list of trusted domains and some user attributes which goes beyond the POSIX interface on top of the D-BUS API to facilitates adoption. It's a convenience library and there is no plan to wrap the whole D-BUS API in this library.
Then at a very minimum we should call this library "libsimpleifpclient.so" or something like that... Calling it libsssd_dbus.so is really confusing.
Yep, that's not what it's doing.
Yes the intent was to make it public so that it can be easily called from a python, ruby or java app to get missing data when an admin is defining roles in the context of the application. So please make it targeted and public. There will be consumers of this API quite soon: Foreman, oVirt, Keystone and others...
This is backwards.
It's easy to call DBus from python, java and friends. There's no need to pass that interaction through the bottleneck (albeit simple bottleneck) that this library provides. At the current time this library is not thread-safe, not loop-integratable, not-abi-extensible etc. It would need to look significantly different to provide the foundation for what you provide there. And why? The callers you suggest already have solid ways to call DBus. It's sssd/tevent that doesn't.
It is thread safe as long as you use separate context for each thread.
The memory allocation routines are not thread-safe. It seems this library is to be used in "plugin" environments (openlmi providers, apache modules ...) and in those cases providing global memory allocation overrides are prone to races (both of the threading variety and otherwise).
Cheers,
Stef
On Tue, May 06, 2014 at 06:58:33AM +0200, Stef Walter wrote:
On 06.05.2014 01:51, Dmitri Pal wrote:
On 05/05/2014 12:28 PM, Pavel Březina wrote:
On 05/05/2014 06:17 PM, Sumit Bose wrote:
On Mon, May 05, 2014 at 05:38:58PM +0200, Pavel Březina wrote:
On 05/05/2014 05:25 PM, Jakub Hrozek wrote:
On Mon, May 05, 2014 at 05:08:39PM +0200, Pavel Březina wrote: > On 05/05/2014 02:06 PM, Stef Walter wrote: >> On 01.05.2014 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. >> >> As noted on IRC, I don't think this should be a public sssd API. As >> internal code of the OpenLMI IFP provider it makes sense. >> >> But remember the public API *is* DBus itself. Callers of the sssd >> DBus >> API should use their local DBus facility in their language or >> toolkit to >> talk with sssd (ie: GDBus or python-dbus, or what have you). > >> I realize that sssd at present has no such local DBus facility in >> its >> toolkit (libdbus doesn't count, it's too low level) and that >> having such >> a client library internal to sssd makes sense. If it could be >> based on >> the codegen work that would be even better. >> >> But, back to the point: I would caution against making it a >> public API >> and having it become an fixture that people turn to when trying >> to talk >> with SSSD. > > Do we want the InfoPipe provider to be used at all? Sure, big > projects and skilled developers can stick with gdbus but why every > single project should write the same hundreds lines of code to do > the most common tasks? I think having such library will make using > the InfoPipe a less pain. > > Of course, this library does not completely hide the D-Bus for > method calls, it is a compromise between universality and the need > to completely reflect our D-Bus interface in this library. However, > it still hides D-Bus completely when dealing with properties. > > I want to keep the OpenLMI provider clean and bring as many things > on SSSD side as possible. This library allows me to do it. > > Sure, we don't have to make it a public API, even though it was the > original intention. I can make it an OpenLMI client or put it > completely into the OpenLMI provider tree.
Also some developers were a bit nervous when we proposed that they need to interact with DBus directly at the C level or were outright rejecting talking to the system bus directly.
It's a bit of Pandora box, though, we'd need to be very strict about this new interface in case somebody comes with an RFE..the DBus interface is /the/ interface...
I can amend the documentation to make it more clear.
The idea was that this library just provide some lookup calls for common stuff like list of trusted domains and some user attributes which goes beyond the POSIX interface on top of the D-BUS API to facilitates adoption. It's a convenience library and there is no plan to wrap the whole D-BUS API in this library.
Then at a very minimum we should call this library "libsimpleifpclient.so" or something like that... Calling it libsssd_dbus.so is really confusing.
I think this is a good idea.
Yep, that's not what it's doing.
Yes the intent was to make it public so that it can be easily called from a python, ruby or java app to get missing data when an admin is defining roles in the context of the application. So please make it targeted and public. There will be consumers of this API quite soon: Foreman, oVirt, Keystone and others...
This is backwards.
It's easy to call DBus from python, java and friends. There's no need to pass that interaction through the bottleneck (albeit simple bottleneck) that this library provides. At the current time this library is not thread-safe, not loop-integratable, not-abi-extensible etc. It would need to look significantly different to provide the foundation for what you provide there. And why? The callers you suggest already have solid ways to call DBus. It's sssd/tevent that doesn't.
I agree with Stef here. Currently we have not planned bindings for those languages for the simple library. The intended users with the simple library are existing/legacy applications written in C which might be happier with a "classical" interface. Due to the nature of the languages I would expect that applications written in Ruby, Python or Java will find it more easy to use the DBUS API directly.
bye, Sumit
Stef _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Tue, May 06, 2014 at 09:21:37AM +0200, Sumit Bose wrote:
It's easy to call DBus from python, java and friends. There's no need to pass that interaction through the bottleneck (albeit simple bottleneck) that this library provides. At the current time this library is not thread-safe, not loop-integratable, not-abi-extensible etc. It would need to look significantly different to provide the foundation for what you provide there. And why? The callers you suggest already have solid ways to call DBus. It's sssd/tevent that doesn't.
I agree with Stef here. Currently we have not planned bindings for those languages for the simple library. The intended users with the simple library are existing/legacy applications written in C which might be happier with a "classical" interface. Due to the nature of the languages I would expect that applications written in Ruby, Python or Java will find it more easy to use the DBUS API directly.
bye, Sumit
+1 this is what we've envisioned from the start. The bindings would go directly do the InfoPipe interface, that's also why we're spending so much time on making them work as one would expect from a full DBus API, with getsetters etc. But some users (like Nikos earlier on the list) just wanted a very /simple/ get-key-value-pair API and this library can be it.
On 05/06/2014 03:52 AM, Jakub Hrozek wrote:
On Tue, May 06, 2014 at 09:21:37AM +0200, Sumit Bose wrote:
It's easy to call DBus from python, java and friends. There's no need to pass that interaction through the bottleneck (albeit simple bottleneck) that this library provides. At the current time this library is not thread-safe, not loop-integratable, not-abi-extensible etc. It would need to look significantly different to provide the foundation for what you provide there. And why? The callers you suggest already have solid ways to call DBus. It's sssd/tevent that doesn't.
I agree with Stef here. Currently we have not planned bindings for those languages for the simple library. The intended users with the simple library are existing/legacy applications written in C which might be happier with a "classical" interface. Due to the nature of the languages I would expect that applications written in Ruby, Python or Java will find it more easy to use the DBUS API directly.
bye, Sumit
+1 this is what we've envisioned from the start. The bindings would go directly do the InfoPipe interface, that's also why we're spending so much time on making them work as one would expect from a full DBus API, with getsetters etc. But some users (like Nikos earlier on the list) just wanted a very /simple/ get-key-value-pair API and this library can be it. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
You lost me. Who is the consumer of the library then other than one apache module? Do we have a design page where it is clearly spelled out whom we are targeting with this effort?
On Tue, 2014-05-06 at 06:58 +0200, Stef Walter wrote:
On 06.05.2014 01:51, Dmitri Pal wrote:
On 05/05/2014 12:28 PM, Pavel Březina wrote:
On 05/05/2014 06:17 PM, Sumit Bose wrote:
On Mon, May 05, 2014 at 05:38:58PM +0200, Pavel Březina wrote:
On 05/05/2014 05:25 PM, Jakub Hrozek wrote:
On Mon, May 05, 2014 at 05:08:39PM +0200, Pavel Březina wrote: > On 05/05/2014 02:06 PM, Stef Walter wrote: >> On 01.05.2014 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. >> >> As noted on IRC, I don't think this should be a public sssd API. As >> internal code of the OpenLMI IFP provider it makes sense. >> >> But remember the public API *is* DBus itself. Callers of the sssd >> DBus >> API should use their local DBus facility in their language or >> toolkit to >> talk with sssd (ie: GDBus or python-dbus, or what have you). > >> I realize that sssd at present has no such local DBus facility in >> its >> toolkit (libdbus doesn't count, it's too low level) and that >> having such >> a client library internal to sssd makes sense. If it could be >> based on >> the codegen work that would be even better. >> >> But, back to the point: I would caution against making it a >> public API >> and having it become an fixture that people turn to when trying >> to talk >> with SSSD. > > Do we want the InfoPipe provider to be used at all? Sure, big > projects and skilled developers can stick with gdbus but why every > single project should write the same hundreds lines of code to do > the most common tasks? I think having such library will make using > the InfoPipe a less pain. > > Of course, this library does not completely hide the D-Bus for > method calls, it is a compromise between universality and the need > to completely reflect our D-Bus interface in this library. However, > it still hides D-Bus completely when dealing with properties. > > I want to keep the OpenLMI provider clean and bring as many things > on SSSD side as possible. This library allows me to do it. > > Sure, we don't have to make it a public API, even though it was the > original intention. I can make it an OpenLMI client or put it > completely into the OpenLMI provider tree.
Also some developers were a bit nervous when we proposed that they need to interact with DBus directly at the C level or were outright rejecting talking to the system bus directly.
It's a bit of Pandora box, though, we'd need to be very strict about this new interface in case somebody comes with an RFE..the DBus interface is /the/ interface...
I can amend the documentation to make it more clear.
The idea was that this library just provide some lookup calls for common stuff like list of trusted domains and some user attributes which goes beyond the POSIX interface on top of the D-BUS API to facilitates adoption. It's a convenience library and there is no plan to wrap the whole D-BUS API in this library.
Then at a very minimum we should call this library "libsimpleifpclient.so" or something like that... Calling it libsssd_dbus.so is really confusing.
Yep, that's not what it's doing.
Yes the intent was to make it public so that it can be easily called from a python, ruby or java app to get missing data when an admin is defining roles in the context of the application. So please make it targeted and public. There will be consumers of this API quite soon: Foreman, oVirt, Keystone and others...
This is backwards.
It's easy to call DBus from python, java and friends. There's no need to pass that interaction through the bottleneck (albeit simple bottleneck) that this library provides. At the current time this library is not thread-safe, not loop-integratable, not-abi-extensible etc. It would need to look significantly different to provide the foundation for what you provide there. And why? The callers you suggest already have solid ways to call DBus. It's sssd/tevent that doesn't.
I think what we should do is rather provide very simple example code snippets that show how to use the API from a couple of languages, and if we really think it helps maybe create a simple module that wraps these examples.
It will be less time than trying to maintain C bindings for sure.
Simo.
On Tue, May 06, 2014 at 08:30:42AM -0400, Dmitri Pal wrote:
On 05/06/2014 03:52 AM, Jakub Hrozek wrote:
On Tue, May 06, 2014 at 09:21:37AM +0200, Sumit Bose wrote:
It's easy to call DBus from python, java and friends. There's no need to pass that interaction through the bottleneck (albeit simple bottleneck) that this library provides. At the current time this library is not thread-safe, not loop-integratable, not-abi-extensible etc. It would need to look significantly different to provide the foundation for what you provide there. And why? The callers you suggest already have solid ways to call DBus. It's sssd/tevent that doesn't.
I agree with Stef here. Currently we have not planned bindings for those languages for the simple library. The intended users with the simple library are existing/legacy applications written in C which might be happier with a "classical" interface. Due to the nature of the languages I would expect that applications written in Ruby, Python or Java will find it more easy to use the DBUS API directly.
bye, Sumit
+1 this is what we've envisioned from the start. The bindings would go directly do the InfoPipe interface, that's also why we're spending so much time on making them work as one would expect from a full DBus API, with getsetters etc. But some users (like Nikos earlier on the list) just wanted a very /simple/ get-key-value-pair API and this library can be it. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
You lost me. Who is the consumer of the library then other than one apache module?
The apache module calls DBus directly. I think the confusion comes from the fact that the apache modules call /functions/ from the DBus API that we developed while we're fleshing out the objects.
Right now, Pavel would use the library and IIRC Sumit would as well.
Do we have a design page where it is clearly spelled out whom we are targeting with this effort?
On 05/06/2014 09:34 AM, Jakub Hrozek wrote:
On Tue, May 06, 2014 at 08:30:42AM -0400, Dmitri Pal wrote:
On 05/06/2014 03:52 AM, Jakub Hrozek wrote:
On Tue, May 06, 2014 at 09:21:37AM +0200, Sumit Bose wrote:
It's easy to call DBus from python, java and friends. There's no need to pass that interaction through the bottleneck (albeit simple bottleneck) that this library provides. At the current time this library is not thread-safe, not loop-integratable, not-abi-extensible etc. It would need to look significantly different to provide the foundation for what you provide there. And why? The callers you suggest already have solid ways to call DBus. It's sssd/tevent that doesn't.
I agree with Stef here. Currently we have not planned bindings for those languages for the simple library. The intended users with the simple library are existing/legacy applications written in C which might be happier with a "classical" interface. Due to the nature of the languages I would expect that applications written in Ruby, Python or Java will find it more easy to use the DBUS API directly.
bye, Sumit
+1 this is what we've envisioned from the start. The bindings would go directly do the InfoPipe interface, that's also why we're spending so much time on making them work as one would expect from a full DBus API, with getsetters etc. But some users (like Nikos earlier on the list) just wanted a very /simple/ get-key-value-pair API and this library can be it. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
You lost me. Who is the consumer of the library then other than one apache module?
The apache module calls DBus directly. I think the confusion comes from the fact that the apache modules call /functions/ from the DBus API that we developed while we're fleshing out the objects.
Right now, Pavel would use the library and IIRC Sumit would as well.
For what? OpenLMI? And for...?
Do we have a design page where it is clearly spelled out whom we are targeting with this effort?
https://fedorahosted.org/sssd/wiki/DesignDocs/DBusSimpleAPI _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Tue, May 06, 2014 at 11:16:12AM -0400, Dmitri Pal wrote:
On 05/06/2014 09:34 AM, Jakub Hrozek wrote:
On Tue, May 06, 2014 at 08:30:42AM -0400, Dmitri Pal wrote:
On 05/06/2014 03:52 AM, Jakub Hrozek wrote:
On Tue, May 06, 2014 at 09:21:37AM +0200, Sumit Bose wrote:
It's easy to call DBus from python, java and friends. There's no need to pass that interaction through the bottleneck (albeit simple bottleneck) that this library provides. At the current time this library is not thread-safe, not loop-integratable, not-abi-extensible etc. It would need to look significantly different to provide the foundation for what you provide there. And why? The callers you suggest already have solid ways to call DBus. It's sssd/tevent that doesn't.
I agree with Stef here. Currently we have not planned bindings for those languages for the simple library. The intended users with the simple library are existing/legacy applications written in C which might be happier with a "classical" interface. Due to the nature of the languages I would expect that applications written in Ruby, Python or Java will find it more easy to use the DBUS API directly.
bye, Sumit
+1 this is what we've envisioned from the start. The bindings would go directly do the InfoPipe interface, that's also why we're spending so much time on making them work as one would expect from a full DBus API, with getsetters etc. But some users (like Nikos earlier on the list) just wanted a very /simple/ get-key-value-pair API and this library can be it. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
You lost me. Who is the consumer of the library then other than one apache module?
The apache module calls DBus directly. I think the confusion comes from the fact that the apache modules call /functions/ from the DBus API that we developed while we're fleshing out the objects.
Right now, Pavel would use the library and IIRC Sumit would as well.
For what? OpenLMI? And for...?
libwbclient-sssd, the replacement for libwbclient which calls SSSD instead of winbind.
bye, Sumit
Do we have a design page where it is clearly spelled out whom we are targeting with this effort?
https://fedorahosted.org/sssd/wiki/DesignDocs/DBusSimpleAPI _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
-- Thank you, Dmitri Pal
Sr. Engineering Manager IdM portfolio Red Hat, Inc.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On 05/06/2014 11:34 AM, Sumit Bose wrote:
On Tue, May 06, 2014 at 11:16:12AM -0400, Dmitri Pal wrote:
On 05/06/2014 09:34 AM, Jakub Hrozek wrote:
On Tue, May 06, 2014 at 08:30:42AM -0400, Dmitri Pal wrote:
On 05/06/2014 03:52 AM, Jakub Hrozek wrote:
On Tue, May 06, 2014 at 09:21:37AM +0200, Sumit Bose wrote:
> It's easy to call DBus from python, java and friends. There's no need to > pass that interaction through the bottleneck (albeit simple bottleneck) > that this library provides. At the current time this library is not > thread-safe, not loop-integratable, not-abi-extensible etc. It would > need to look significantly different to provide the foundation for what > you provide there. And why? The callers you suggest already have solid > ways to call DBus. It's sssd/tevent that doesn't. I agree with Stef here. Currently we have not planned bindings for those languages for the simple library. The intended users with the simple library are existing/legacy applications written in C which might be happier with a "classical" interface. Due to the nature of the languages I would expect that applications written in Ruby, Python or Java will find it more easy to use the DBUS API directly.
bye, Sumit
+1 this is what we've envisioned from the start. The bindings would go directly do the InfoPipe interface, that's also why we're spending so much time on making them work as one would expect from a full DBus API, with getsetters etc. But some users (like Nikos earlier on the list) just wanted a very /simple/ get-key-value-pair API and this library can be it. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
You lost me. Who is the consumer of the library then other than one apache module?
The apache module calls DBus directly. I think the confusion comes from the fact that the apache modules call /functions/ from the DBus API that we developed while we're fleshing out the objects.
Right now, Pavel would use the library and IIRC Sumit would as well.
For what? OpenLMI? And for...?
libwbclient-sssd, the replacement for libwbclient which calls SSSD instead of winbind.
Then we can keep the library internal to SSSD for now since only SSSD related components plan to consume it so far.
bye, Sumit
Do we have a design page where it is clearly spelled out whom we are targeting with this effort?
https://fedorahosted.org/sssd/wiki/DesignDocs/DBusSimpleAPI _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
-- Thank you, Dmitri Pal
Sr. Engineering Manager IdM portfolio Red Hat, Inc.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Tue, May 06, 2014 at 06:34:53PM -0400, Dmitri Pal wrote:
On 05/06/2014 11:34 AM, Sumit Bose wrote:
On Tue, May 06, 2014 at 11:16:12AM -0400, Dmitri Pal wrote:
On 05/06/2014 09:34 AM, Jakub Hrozek wrote:
On Tue, May 06, 2014 at 08:30:42AM -0400, Dmitri Pal wrote:
On 05/06/2014 03:52 AM, Jakub Hrozek wrote:
On Tue, May 06, 2014 at 09:21:37AM +0200, Sumit Bose wrote: >>It's easy to call DBus from python, java and friends. There's no need to >>pass that interaction through the bottleneck (albeit simple bottleneck) >>that this library provides. At the current time this library is not >>thread-safe, not loop-integratable, not-abi-extensible etc. It would >>need to look significantly different to provide the foundation for what >>you provide there. And why? The callers you suggest already have solid >>ways to call DBus. It's sssd/tevent that doesn't. >I agree with Stef here. Currently we have not planned bindings for those >languages for the simple library. The intended users with the simple >library are existing/legacy applications written in C which might be >happier with a "classical" interface. Due to the nature of the languages >I would expect that applications written in Ruby, Python or Java will >find it more easy to use the DBUS API directly. > >bye, >Sumit +1 this is what we've envisioned from the start. The bindings would go directly do the InfoPipe interface, that's also why we're spending so much time on making them work as one would expect from a full DBus API, with getsetters etc. But some users (like Nikos earlier on the list) just wanted a very /simple/ get-key-value-pair API and this library can be it. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
You lost me. Who is the consumer of the library then other than one apache module?
The apache module calls DBus directly. I think the confusion comes from the fact that the apache modules call /functions/ from the DBus API that we developed while we're fleshing out the objects.
Right now, Pavel would use the library and IIRC Sumit would as well.
For what? OpenLMI? And for...?
libwbclient-sssd, the replacement for libwbclient which calls SSSD instead of winbind.
Then we can keep the library internal to SSSD for now since only SSSD related components plan to consume it so far.
In working on getting libwbclient-sssd to samba upstream because maintenance would be a lot easier this way.
bye, Sumit
bye, Sumit
Do we have a design page where it is clearly spelled out whom we are targeting with this effort?
https://fedorahosted.org/sssd/wiki/DesignDocs/DBusSimpleAPI _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
-- Thank you, Dmitri Pal
Sr. Engineering Manager IdM portfolio Red Hat, Inc.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
-- Thank you, Dmitri Pal
Sr. Engineering Manager IdM portfolio Red Hat, Inc.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On 05/07/2014 12:34 AM, Dmitri Pal wrote:
On 05/06/2014 11:34 AM, Sumit Bose wrote:
On Tue, May 06, 2014 at 11:16:12AM -0400, Dmitri Pal wrote:
On 05/06/2014 09:34 AM, Jakub Hrozek wrote:
On Tue, May 06, 2014 at 08:30:42AM -0400, Dmitri Pal wrote:
On 05/06/2014 03:52 AM, Jakub Hrozek wrote:
On Tue, May 06, 2014 at 09:21:37AM +0200, Sumit Bose wrote: >> It's easy to call DBus from python, java and friends. There's no >> need to >> pass that interaction through the bottleneck (albeit simple >> bottleneck) >> that this library provides. At the current time this library is not >> thread-safe, not loop-integratable, not-abi-extensible etc. It >> would >> need to look significantly different to provide the foundation >> for what >> you provide there. And why? The callers you suggest already have >> solid >> ways to call DBus. It's sssd/tevent that doesn't. > I agree with Stef here. Currently we have not planned bindings > for those > languages for the simple library. The intended users with the simple > library are existing/legacy applications written in C which might be > happier with a "classical" interface. Due to the nature of the > languages > I would expect that applications written in Ruby, Python or Java > will > find it more easy to use the DBUS API directly. > > bye, > Sumit +1 this is what we've envisioned from the start. The bindings would go directly do the InfoPipe interface, that's also why we're spending so much time on making them work as one would expect from a full DBus API, with getsetters etc. But some users (like Nikos earlier on the list) just wanted a very /simple/ get-key-value-pair API and this library can be it. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
You lost me. Who is the consumer of the library then other than one apache module?
The apache module calls DBus directly. I think the confusion comes from the fact that the apache modules call /functions/ from the DBus API that we developed while we're fleshing out the objects.
Right now, Pavel would use the library and IIRC Sumit would as well.
For what? OpenLMI? And for...?
libwbclient-sssd, the replacement for libwbclient which calls SSSD instead of winbind.
Then we can keep the library internal to SSSD for now since only SSSD related components plan to consume it so far.
But those component won't be part of SSSD upstream so keeping it internal will complicate things. We don't have to call it public but we should distribute the library in a separate package and header file in appropriate -devel package.
bye, Sumit
Do we have a design page where it is clearly spelled out whom we are targeting with this effort?
https://fedorahosted.org/sssd/wiki/DesignDocs/DBusSimpleAPI _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
-- Thank you, Dmitri Pal
Sr. Engineering Manager IdM portfolio Red Hat, Inc.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On 05/01/2014 05:39 PM, 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, I renamed the library to libsss_simpleifp.so and changed the thing that Stef wanted.
On 05/13/2014 11:18 AM, Pavel Březina wrote:
On 05/01/2014 05:39 PM, 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, I renamed the library to libsss_simpleifp.so and changed the thing that Stef wanted.
I forgot to commit last change, new patch set is attached.
On 05/13/2014 05:43 PM, Pavel Březina wrote:
On 05/13/2014 11:18 AM, Pavel Březina wrote:
On 05/01/2014 05:39 PM, 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, I renamed the library to libsss_simpleifp.so and changed the thing that Stef wanted.
I forgot to commit last change, new patch set is attached.
Sending new patchset that fixes few packaging issues found by Jan Šafránek from OpenLMI.
On 05/15/2014 09:09 PM, Pavel Březina wrote:
On 05/13/2014 05:43 PM, Pavel Březina wrote:
On 05/13/2014 11:18 AM, Pavel Březina wrote:
On 05/01/2014 05:39 PM, 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, I renamed the library to libsss_simpleifp.so and changed the thing that Stef wanted.
I forgot to commit last change, new patch set is attached.
Sending new patchset that fixes few packaging issues found by Jan Šafránek from OpenLMI.
And once again I was too fast. I forgot to commit an issue that documentation html since it was generated from missing file (the path pointed to the old location in sss_client).
Now should be everything all right.
On Thu, May 15, 2014 at 09:23:46PM +0200, Pavel Březina wrote:
On 05/15/2014 09:09 PM, Pavel Březina wrote:
On 05/13/2014 05:43 PM, Pavel Březina wrote:
On 05/13/2014 11:18 AM, Pavel Březina wrote:
On 05/01/2014 05:39 PM, 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, I renamed the library to libsss_simpleifp.so and changed the thing that Stef wanted.
I forgot to commit last change, new patch set is attached.
Sending new patchset that fixes few packaging issues found by Jan Šafránek from OpenLMI.
And once again I was too fast. I forgot to commit an issue that documentation html since it was generated from missing file (the path pointed to the old location in sss_client).
Now should be everything all right.
* sss_dbus: introduce API Did you consider using a different base for the errors, rather than 0 or do you think it's not needed because the error is not an int but an enum?
I think there needs to be a really verbose comment about the purpose of the library. We don't want the library to be used outside the simplest cases. As Stef already said earlier, the DBus interface is /the/ interface.
sss_dbus_object -- would it help for the 'interfaces' to be an array? After all, an object should support multiple interfaces.
Why is sss_dbus_bool typedef-ed as uint32_t and not the standard C bool? You're treating the types differently later on anyway.
I don't think the doxygen docs on sss_dbus_send_message should say what exactly the function does, just that it's a shorthand.
* sss_dbus: implement API The hunks that change src/lib/dbus/sss_dbus.exports and src/lib/dbus/sss_dbus.h belong to patch #1.
sss_dbus_send_message_ex - if you're checking all input parameters of public functions, you should also check _reply.
Why does sss_dbus_fetch_attr() return "***_attrs" ? Isn't the return value supposed to be a single attribute, not an array? There is a similar issue with sss_dbus_parse_attr().
You should always wrap multi-line macros with "do-while(0);" See: http://gcc.gnu.org/onlinedocs/cpp/Swallowing-the-Semicolon.html I know it's not sctrictly needed with the way you use the macros, but it's still best practice.
I don't like the macros in sss_dbus_parser.c, a macro that changes the flow of a program makes it quite hard to debug. Any chance the macro could just set an error code instead?
sss_dbus_parse_basic(): Instead of using the default handler, you can set ret to INVALID on initialization and then not use the default handler at all. The added bonus is that if you extend the type enum but forget to add the new type to the switch-case, you'll get a warning.
Why is there both sss_dbus_parse_single_attr() and sss_dbus_parse_attr()?
There is an extra semicolon after "case DBUS_TYPE_OBJECT_PATH:"
* build patch: in sssd.spec.in, you don't need to add BuildRequires: dbus-devel, we already require dbus-devel for core SSSD. I also prefer to have BuildRequires all at the top, because they apply to the SRPM, not the binary RPMs.
libsss_dbus-devel must require fully-versioned libsss_dbus.
src/lib/dbus/sss_dbus.pc.in: I think you can drop prefix and exec_prefix, right? Only includedir and libdir are used.
* unit test patch: Please squash the hunk that modifies src/lib/dbus/sss_dbus.c into an earlier patch.
The tests themselves look good to me, I'm glad we've finally acquired the habit of unit-testing the code if possible.
* sss_dbus: add support for string dictionary Again, the BuildRequires is already required by core sssd. You should also drop the explicit dhash Requires, RPM will just pick that up.
In the "done" handler of sss_dbus_parse_array(), can you either split the for so that each part is on its own line or all on one? This way seems unreadable to me. I also prefer: for (int i; ;;) { /* stuff */ } over: int i; for (i; ;;) { /* stuff */ } In the context of this done: handler, the scope of 'i' would be the same.
After applying this patch, valgrind shows a leak in the unit test: ==26658== 4 bytes in 1 blocks are definitely lost in loss record 1 of 8 ==26658== at 0x4C2745D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==26658== by 0x40B148: default_alloc (sss_dbus.c:32) ==26658== by 0x40AC29: sss_dbus_alloc_zero (sss_dbus_utils.c:29) ==26658== by 0x40AE07: sss_dbus_strdup (sss_dbus_utils.c:84) ==26658== by 0x40A79C: sss_dbus_parse_dict (sss_dbus_parser.c:151) ==26658== by 0x40A406: sss_dbus_parse_array (sss_dbus_parser.c:411) ==26658== by 0x4093D2: sss_dbus_parse_variant (sss_dbus_parser.c:472) ==26658== by 0x408D58: sss_dbus_parse_single_attr (sss_dbus_parser.c:512) ==26658== by 0x408C35: sss_dbus_parse_attr (sss_dbus_parser.c:546) ==26658== by 0x403C72: test_sss_dbus_parse_attr_string_dict (test_sss_dbus.c:661) ==26658== by 0x4E345B9: _run_test (in /usr/lib64/libcmocka.so.0.1.2) ==26658== by 0x4E34A29: _run_tests (in /usr/lib64/libcmocka.so.0.1.2)
* sss_dbus: add shortcuts for common use cases I wonder if the functions specific to an object could be kept private to the LMI code. Is the CIFS module going to use these? I see the point and I see the code can be handy, but I really think we'd be asked to provide similar wrappers for groups and all other object we're going to expose on the bus.
I think fetch_object_by_attr and fetch_object_by_name are fine, I'm not so sure about list_domains anymore. If you think these functions should stay, we need a good explanation in the docs on why we are supporting users and not groups.
There is a new leak after applying the patch: ==1047== 664 (184 direct, 480 indirect) bytes in 1 blocks are definitely lost in loss record 17 of 17 ==1047== at 0x4C291D4: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==1047== by 0x5B11D65: dbus_message_new_empty_header (dbus-message.c:1140) ==1047== by 0x5B12B21: dbus_message_new_method_call (dbus-message.c:1271) ==1047== by 0x40C2EF: sss_dbus_create_message (sss_dbus.c:176) ==1047== by 0x40C5B7: sss_dbus_create_prop_msg (sss_dbus.c:49) ==1047== by 0x40C4AE: sss_dbus_fetch_attr (sss_dbus.c:248) ==1047== by 0x4063BB: test_sss_dbus_fetch_attr (test_sss_dbus.c:1335) ==1047== by 0x4E345B9: _run_test (in /usr/lib64/libcmocka.so.0.1.2) ==1047== by 0x4E34A29: _run_tests (in /usr/lib64/libcmocka.so.0.1.2) ==1047== by 0x408924: main (test_sss_dbus.c:1927) ==1047==
* Add libsss_dbus.so to dlopen test ACK
* Rename libsss_dbus to libsss_simpleifp Looks OK, but why is it a separate patch, was it too hard to roll into the previous ones? Please note I'm fine with a separate patch, I'd just like to see the reason.
On 05/16/2014 02:04 PM, Jakub Hrozek wrote:
On Thu, May 15, 2014 at 09:23:46PM +0200, Pavel Březina wrote:
On 05/15/2014 09:09 PM, Pavel Březina wrote:
On 05/13/2014 05:43 PM, Pavel Březina wrote:
On 05/13/2014 11:18 AM, Pavel Březina wrote:
On 05/01/2014 05:39 PM, 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, I renamed the library to libsss_simpleifp.so and changed the thing that Stef wanted.
I forgot to commit last change, new patch set is attached.
Sending new patchset that fixes few packaging issues found by Jan Šafránek from OpenLMI.
And once again I was too fast. I forgot to commit an issue that documentation html since it was generated from missing file (the path pointed to the old location in sss_client).
Now should be everything all right.
- sss_dbus: introduce API
Did you consider using a different base for the errors, rather than 0 or do you think it's not needed because the error is not an int but an enum?
It is enough to have zero based enum here in my opinion. All functions return proper error code. But I can change it if you want.
I think there needs to be a really verbose comment about the purpose of the library. We don't want the library to be used outside the simplest cases. As Stef already said earlier, the DBus interface is /the/ interface.
OK. I'll try to craft something.
sss_dbus_object -- would it help for the 'interfaces' to be an array? After all, an object should support multiple interfaces.
How would we handle interfaces that does not have distinct properties and method names?
The main purpose of this struct was to have all information about an object on one place with simple access to name and object path.
However I would be fine to drop it, since I don't actually use it in OpenLMI... it is used merely in the common functions.
Why is sss_dbus_bool typedef-ed as uint32_t and not the standard C bool? You're treating the types differently later on anyway.
I think it should be more an alias to dbus_bool_t (I didn't need to include dbus.h before so I just used appropriate type for it).
But I think I can use bool from stdbool.h there, since we switched to C99 anyway.
I don't think the doxygen docs on sss_dbus_send_message should say what exactly the function does, just that it's a shorthand.
- sss_dbus: implement API
The hunks that change src/lib/dbus/sss_dbus.exports and src/lib/dbus/sss_dbus.h belong to patch #1.
sss_dbus_send_message_ex - if you're checking all input parameters of public functions, you should also check _reply.
Sometimes the caller may not need to be interested in reply - mainly for methods that don't return any value (just dbus error).
Why does sss_dbus_fetch_attr() return "***_attrs" ? Isn't the return value supposed to be a single attribute, not an array? There is a similar issue with sss_dbus_parse_attr().
This was first done that way, because was expanding "extra attributes" instead of having them in a hash table. Therefore asking for one attribute may resulted in having a set of attribute.
I kept it this way, even though it is not strictly needed now, because I wanted to kept the structure opaque and you need to ask to convert it to a specific data type.
There would have been more getter functions, that would work with single attribute.
I think having it unified is a benefit here - you can simply switch from working with one attribute to all attribute.
You should always wrap multi-line macros with "do-while(0);" See: http://gcc.gnu.org/onlinedocs/cpp/Swallowing-the-Semicolon.html I know it's not sctrictly needed with the way you use the macros, but it's still best practice.
I don't like the macros in sss_dbus_parser.c, a macro that changes the flow of a program makes it quite hard to debug. Any chance the macro could just set an error code instead?
Given how the macro is used I think this is an acceptable exception.
But if you strongly disagree, I will think about it.
sss_dbus_parse_basic(): Instead of using the default handler, you can set ret to INVALID on initialization and then not use the default handler at all. The added bonus is that if you extend the type enum but forget to add the new type to the switch-case, you'll get a warning.
Why is there both sss_dbus_parse_single_attr() and sss_dbus_parse_attr()?
There is an extra semicolon after "case DBUS_TYPE_OBJECT_PATH:"
- build patch:
in sssd.spec.in, you don't need to add BuildRequires: dbus-devel, we already require dbus-devel for core SSSD. I also prefer to have BuildRequires all at the top, because they apply to the SRPM, not the binary RPMs.
libsss_dbus-devel must require fully-versioned libsss_dbus.
src/lib/dbus/sss_dbus.pc.in: I think you can drop prefix and exec_prefix, right? Only includedir and libdir are used.
- unit test patch:
Please squash the hunk that modifies src/lib/dbus/sss_dbus.c into an earlier patch.
The tests themselves look good to me, I'm glad we've finally acquired the habit of unit-testing the code if possible.
- sss_dbus: add support for string dictionary
Again, the BuildRequires is already required by core sssd. You should also drop the explicit dhash Requires, RPM will just pick that up.
In the "done" handler of sss_dbus_parse_array(), can you either split the for so that each part is on its own line or all on one? This way seems unreadable to me. I also prefer: for (int i; ;;) { /* stuff */ } over: int i; for (i; ;;) { /* stuff */ } In the context of this done: handler, the scope of 'i' would be the same.
After applying this patch, valgrind shows a leak in the unit test: ==26658== 4 bytes in 1 blocks are definitely lost in loss record 1 of 8 ==26658== at 0x4C2745D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==26658== by 0x40B148: default_alloc (sss_dbus.c:32) ==26658== by 0x40AC29: sss_dbus_alloc_zero (sss_dbus_utils.c:29) ==26658== by 0x40AE07: sss_dbus_strdup (sss_dbus_utils.c:84) ==26658== by 0x40A79C: sss_dbus_parse_dict (sss_dbus_parser.c:151) ==26658== by 0x40A406: sss_dbus_parse_array (sss_dbus_parser.c:411) ==26658== by 0x4093D2: sss_dbus_parse_variant (sss_dbus_parser.c:472) ==26658== by 0x408D58: sss_dbus_parse_single_attr (sss_dbus_parser.c:512) ==26658== by 0x408C35: sss_dbus_parse_attr (sss_dbus_parser.c:546) ==26658== by 0x403C72: test_sss_dbus_parse_attr_string_dict (test_sss_dbus.c:661) ==26658== by 0x4E345B9: _run_test (in /usr/lib64/libcmocka.so.0.1.2) ==26658== by 0x4E34A29: _run_tests (in /usr/lib64/libcmocka.so.0.1.2)
- sss_dbus: add shortcuts for common use cases
I wonder if the functions specific to an object could be kept private to the LMI code. Is the CIFS module going to use these? I see the point and I see the code can be handy, but I really think we'd be asked to provide similar wrappers for groups and all other object we're going to expose on the bus.
I think fetch_object_by_attr and fetch_object_by_name are fine, I'm not so sure about list_domains anymore. If you think these functions should stay, we need a good explanation in the docs on why we are supporting users and not groups.
Actually I don't use those functions at all so I'm fine with removing them. I didn't want them to be there for this precise reason.
Sumit asked for them.
There is a new leak after applying the patch: ==1047== 664 (184 direct, 480 indirect) bytes in 1 blocks are definitely lost in loss record 17 of 17 ==1047== at 0x4C291D4: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==1047== by 0x5B11D65: dbus_message_new_empty_header (dbus-message.c:1140) ==1047== by 0x5B12B21: dbus_message_new_method_call (dbus-message.c:1271) ==1047== by 0x40C2EF: sss_dbus_create_message (sss_dbus.c:176) ==1047== by 0x40C5B7: sss_dbus_create_prop_msg (sss_dbus.c:49) ==1047== by 0x40C4AE: sss_dbus_fetch_attr (sss_dbus.c:248) ==1047== by 0x4063BB: test_sss_dbus_fetch_attr (test_sss_dbus.c:1335) ==1047== by 0x4E345B9: _run_test (in /usr/lib64/libcmocka.so.0.1.2) ==1047== by 0x4E34A29: _run_tests (in /usr/lib64/libcmocka.so.0.1.2) ==1047== by 0x408924: main (test_sss_dbus.c:1927) ==1047==
- Add libsss_dbus.so to dlopen test
ACK
- Rename libsss_dbus to libsss_simpleifp
Looks OK, but why is it a separate patch, was it too hard to roll into the previous ones? Please note I'm fine with a separate patch, I'd just like to see the reason.
I think it would be possible to squash it, I didn't want to do it in a case we change the opinion back during the review :-)
On Fri, May 16, 2014 at 02:54:55PM +0200, Pavel Březina wrote:
On 05/16/2014 02:04 PM, Jakub Hrozek wrote:
On Thu, May 15, 2014 at 09:23:46PM +0200, Pavel Březina wrote:
On 05/15/2014 09:09 PM, Pavel Březina wrote:
On 05/13/2014 05:43 PM, Pavel Březina wrote:
On 05/13/2014 11:18 AM, Pavel Březina wrote:
On 05/01/2014 05:39 PM, 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, I renamed the library to libsss_simpleifp.so and changed the thing that Stef wanted.
I forgot to commit last change, new patch set is attached.
Sending new patchset that fixes few packaging issues found by Jan Šafránek from OpenLMI.
And once again I was too fast. I forgot to commit an issue that documentation html since it was generated from missing file (the path pointed to the old location in sss_client).
Now should be everything all right.
- sss_dbus: introduce API
Did you consider using a different base for the errors, rather than 0 or do you think it's not needed because the error is not an int but an enum?
It is enough to have zero based enum here in my opinion. All functions return proper error code. But I can change it if you want.
I think it's fine. It might be nice to add a strerror function in a later patch, though.
I think there needs to be a really verbose comment about the purpose of the library. We don't want the library to be used outside the simplest cases. As Stef already said earlier, the DBus interface is /the/ interface.
OK. I'll try to craft something.
sss_dbus_object -- would it help for the 'interfaces' to be an array? After all, an object should support multiple interfaces.
How would we handle interfaces that does not have distinct properties and method names?
The main purpose of this struct was to have all information about an object on one place with simple access to name and object path.
However I would be fine to drop it, since I don't actually use it in OpenLMI... it is used merely in the common functions.
Ah, then it's fine. I think I misunderstood the field in the structure.
Why is sss_dbus_bool typedef-ed as uint32_t and not the standard C bool? You're treating the types differently later on anyway.
I think it should be more an alias to dbus_bool_t (I didn't need to include dbus.h before so I just used appropriate type for it).
But I think I can use bool from stdbool.h there, since we switched to C99 anyway.
I think the standard C bool would make sense. Also, does this typedef need to be part of the public header? Can the bool be an opaque type?
I don't think the doxygen docs on sss_dbus_send_message should say what exactly the function does, just that it's a shorthand.
- sss_dbus: implement API
The hunks that change src/lib/dbus/sss_dbus.exports and src/lib/dbus/sss_dbus.h belong to patch #1.
sss_dbus_send_message_ex - if you're checking all input parameters of public functions, you should also check _reply.
Sometimes the caller may not need to be interested in reply - mainly for methods that don't return any value (just dbus error).
Yeah, I realized that after I wrote this comment, just forgot to remove the comment from the e-mail..sorry
Why does sss_dbus_fetch_attr() return "***_attrs" ? Isn't the return value supposed to be a single attribute, not an array? There is a similar issue with sss_dbus_parse_attr().
This was first done that way, because was expanding "extra attributes" instead of having them in a hash table. Therefore asking for one attribute may resulted in having a set of attribute.
I kept it this way, even though it is not strictly needed now, because I wanted to kept the structure opaque and you need to ask to convert it to a specific data type.
There would have been more getter functions, that would work with single attribute.
I think having it unified is a benefit here - you can simply switch from working with one attribute to all attribute.
OK.
You should always wrap multi-line macros with "do-while(0);" See: http://gcc.gnu.org/onlinedocs/cpp/Swallowing-the-Semicolon.html I know it's not sctrictly needed with the way you use the macros, but it's still best practice.
I don't like the macros in sss_dbus_parser.c, a macro that changes the flow of a program makes it quite hard to debug. Any chance the macro could just set an error code instead?
Given how the macro is used I think this is an acceptable exception.
But if you strongly disagree, I will think about it.
Please add the do-while.
sss_dbus_parse_basic(): Instead of using the default handler, you can set ret to INVALID on initialization and then not use the default handler at all. The added bonus is that if you extend the type enum but forget to add the new type to the switch-case, you'll get a warning.
Why is there both sss_dbus_parse_single_attr() and sss_dbus_parse_attr()?
There is an extra semicolon after "case DBUS_TYPE_OBJECT_PATH:"
- build patch:
in sssd.spec.in, you don't need to add BuildRequires: dbus-devel, we already require dbus-devel for core SSSD. I also prefer to have BuildRequires all at the top, because they apply to the SRPM, not the binary RPMs.
libsss_dbus-devel must require fully-versioned libsss_dbus.
src/lib/dbus/sss_dbus.pc.in: I think you can drop prefix and exec_prefix, right? Only includedir and libdir are used.
- unit test patch:
Please squash the hunk that modifies src/lib/dbus/sss_dbus.c into an earlier patch.
The tests themselves look good to me, I'm glad we've finally acquired the habit of unit-testing the code if possible.
- sss_dbus: add support for string dictionary
Again, the BuildRequires is already required by core sssd. You should also drop the explicit dhash Requires, RPM will just pick that up.
In the "done" handler of sss_dbus_parse_array(), can you either split the for so that each part is on its own line or all on one? This way seems unreadable to me. I also prefer: for (int i; ;;) { /* stuff */ } over: int i; for (i; ;;) { /* stuff */ } In the context of this done: handler, the scope of 'i' would be the same.
After applying this patch, valgrind shows a leak in the unit test: ==26658== 4 bytes in 1 blocks are definitely lost in loss record 1 of 8 ==26658== at 0x4C2745D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==26658== by 0x40B148: default_alloc (sss_dbus.c:32) ==26658== by 0x40AC29: sss_dbus_alloc_zero (sss_dbus_utils.c:29) ==26658== by 0x40AE07: sss_dbus_strdup (sss_dbus_utils.c:84) ==26658== by 0x40A79C: sss_dbus_parse_dict (sss_dbus_parser.c:151) ==26658== by 0x40A406: sss_dbus_parse_array (sss_dbus_parser.c:411) ==26658== by 0x4093D2: sss_dbus_parse_variant (sss_dbus_parser.c:472) ==26658== by 0x408D58: sss_dbus_parse_single_attr (sss_dbus_parser.c:512) ==26658== by 0x408C35: sss_dbus_parse_attr (sss_dbus_parser.c:546) ==26658== by 0x403C72: test_sss_dbus_parse_attr_string_dict (test_sss_dbus.c:661) ==26658== by 0x4E345B9: _run_test (in /usr/lib64/libcmocka.so.0.1.2) ==26658== by 0x4E34A29: _run_tests (in /usr/lib64/libcmocka.so.0.1.2)
- sss_dbus: add shortcuts for common use cases
I wonder if the functions specific to an object could be kept private to the LMI code. Is the CIFS module going to use these? I see the point and I see the code can be handy, but I really think we'd be asked to provide similar wrappers for groups and all other object we're going to expose on the bus.
I think fetch_object_by_attr and fetch_object_by_name are fine, I'm not so sure about list_domains anymore. If you think these functions should stay, we need a good explanation in the docs on why we are supporting users and not groups.
Actually I don't use those functions at all so I'm fine with removing them. I didn't want them to be there for this precise reason.
Sumit asked for them.
Sumit, since Pavel doesn't use the object and user shorthands, would you be OK to keep them in your code? Do you agree with not 'leaking' the per-object functions to the public interface and keep it as simple as possible or do you see it would cause problems for you?
There is a new leak after applying the patch: ==1047== 664 (184 direct, 480 indirect) bytes in 1 blocks are definitely lost in loss record 17 of 17 ==1047== at 0x4C291D4: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==1047== by 0x5B11D65: dbus_message_new_empty_header (dbus-message.c:1140) ==1047== by 0x5B12B21: dbus_message_new_method_call (dbus-message.c:1271) ==1047== by 0x40C2EF: sss_dbus_create_message (sss_dbus.c:176) ==1047== by 0x40C5B7: sss_dbus_create_prop_msg (sss_dbus.c:49) ==1047== by 0x40C4AE: sss_dbus_fetch_attr (sss_dbus.c:248) ==1047== by 0x4063BB: test_sss_dbus_fetch_attr (test_sss_dbus.c:1335) ==1047== by 0x4E345B9: _run_test (in /usr/lib64/libcmocka.so.0.1.2) ==1047== by 0x4E34A29: _run_tests (in /usr/lib64/libcmocka.so.0.1.2) ==1047== by 0x408924: main (test_sss_dbus.c:1927) ==1047==
- Add libsss_dbus.so to dlopen test
ACK
- Rename libsss_dbus to libsss_simpleifp
Looks OK, but why is it a separate patch, was it too hard to roll into the previous ones? Please note I'm fine with a separate patch, I'd just like to see the reason.
I think it would be possible to squash it, I didn't want to do it in a case we change the opinion back during the review :-)
If it's easy, then please squash it, if not, I'm fine with a separate patch.
On Fri, May 16, 2014 at 03:32:53PM +0200, Jakub Hrozek wrote:
On Fri, May 16, 2014 at 02:54:55PM +0200, Pavel Březina wrote:
On 05/16/2014 02:04 PM, Jakub Hrozek wrote:
....
- sss_dbus: add shortcuts for common use cases
I wonder if the functions specific to an object could be kept private to the LMI code. Is the CIFS module going to use these? I see the point and I see the code can be handy, but I really think we'd be asked to provide similar wrappers for groups and all other object we're going to expose on the bus.
I think fetch_object_by_attr and fetch_object_by_name are fine, I'm not so sure about list_domains anymore. If you think these functions should stay, we need a good explanation in the docs on why we are supporting users and not groups.
Actually I don't use those functions at all so I'm fine with removing them. I didn't want them to be there for this precise reason.
Sumit asked for them.
Sumit, since Pavel doesn't use the object and user shorthands, would you be OK to keep them in your code? Do you agree with not 'leaking' the per-object functions to the public interface and keep it as simple as possible or do you see it would cause problems for you?
My idea was that this library hides everything D-BUS related from the caller so that e.g. he does not has to care about D-BUS method names and types.
I see e.g. sss_dbus_fetch_user_by_name() basically as a way to avoid to look up the method name, "UserByName" in this case, and the D-BUS type for the argument, DBUS_TYPE_STRING here. The later would also require to have D-BUS header files installed.
If we drop this calls we should add references to the docs where method names and expected parameters can be found and add enum sss_dbus_attr_type to the public header file and translate them back into the D-BUS types when needed.
Btw, currently using sss_dbus.h requires that D-BUS headers are installed because of DBusMessage. I would suggest to use a opaque struct which only contains a DBusMessage member to remove this dependency.
There is a new leak after applying the patch: ==1047== 664 (184 direct, 480 indirect) bytes in 1 blocks are definitely lost in loss record 17 of 17 ==1047== at 0x4C291D4: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==1047== by 0x5B11D65: dbus_message_new_empty_header (dbus-message.c:1140) ==1047== by 0x5B12B21: dbus_message_new_method_call (dbus-message.c:1271) ==1047== by 0x40C2EF: sss_dbus_create_message (sss_dbus.c:176) ==1047== by 0x40C5B7: sss_dbus_create_prop_msg (sss_dbus.c:49) ==1047== by 0x40C4AE: sss_dbus_fetch_attr (sss_dbus.c:248) ==1047== by 0x4063BB: test_sss_dbus_fetch_attr (test_sss_dbus.c:1335) ==1047== by 0x4E345B9: _run_test (in /usr/lib64/libcmocka.so.0.1.2) ==1047== by 0x4E34A29: _run_tests (in /usr/lib64/libcmocka.so.0.1.2) ==1047== by 0x408924: main (test_sss_dbus.c:1927) ==1047==
- Add libsss_dbus.so to dlopen test
ACK
- Rename libsss_dbus to libsss_simpleifp
Looks OK, but why is it a separate patch, was it too hard to roll into the previous ones? Please note I'm fine with a separate patch, I'd just like to see the reason.
I think it would be possible to squash it, I didn't want to do it in a case we change the opinion back during the review :-)
If it's easy, then please squash it, if not, I'm fine with a separate patch.
I think it would be nice to rename the header file (sss_dbus.h -> sss_sifp.h or sss_simpleifp.h) and the general prefix (sss_dbus_* -> sss_sifp_*) as well. If we agree that this would make sense I'm willing to extend the current rename patch accordingly. But I think in this case it would be easier with a separate patch.
bye, Sumit
On 05/16/2014 03:32 PM, Jakub Hrozek wrote:
On Fri, May 16, 2014 at 02:54:55PM +0200, Pavel Březina wrote:
On 05/16/2014 02:04 PM, Jakub Hrozek wrote:
On Thu, May 15, 2014 at 09:23:46PM +0200, Pavel Březina wrote:
On 05/15/2014 09:09 PM, Pavel Březina wrote:
On 05/13/2014 05:43 PM, Pavel Březina wrote:
On 05/13/2014 11:18 AM, Pavel Březina wrote: > On 05/01/2014 05:39 PM, 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, > I renamed the library to libsss_simpleifp.so and changed the thing that > Stef wanted.
I forgot to commit last change, new patch set is attached.
Sending new patchset that fixes few packaging issues found by Jan Šafránek from OpenLMI.
And once again I was too fast. I forgot to commit an issue that documentation html since it was generated from missing file (the path pointed to the old location in sss_client).
Now should be everything all right.
- sss_dbus: introduce API
Did you consider using a different base for the errors, rather than 0 or do you think it's not needed because the error is not an int but an enum?
It is enough to have zero based enum here in my opinion. All functions return proper error code. But I can change it if you want.
I think it's fine. It might be nice to add a strerror function in a later patch, though.
I think there needs to be a really verbose comment about the purpose of the library. We don't want the library to be used outside the simplest cases. As Stef already said earlier, the DBus interface is /the/ interface.
OK. I'll try to craft something.
sss_dbus_object -- would it help for the 'interfaces' to be an array? After all, an object should support multiple interfaces.
How would we handle interfaces that does not have distinct properties and method names?
The main purpose of this struct was to have all information about an object on one place with simple access to name and object path.
However I would be fine to drop it, since I don't actually use it in OpenLMI... it is used merely in the common functions.
Ah, then it's fine. I think I misunderstood the field in the structure.
Why is sss_dbus_bool typedef-ed as uint32_t and not the standard C bool? You're treating the types differently later on anyway.
I think it should be more an alias to dbus_bool_t (I didn't need to include dbus.h before so I just used appropriate type for it).
But I think I can use bool from stdbool.h there, since we switched to C99 anyway.
I think the standard C bool would make sense. Also, does this typedef need to be part of the public header? Can the bool be an opaque type?
Since I switched to std bool then the typedef became useless so I removed it completely.
I don't think the doxygen docs on sss_dbus_send_message should say what exactly the function does, just that it's a shorthand.
- sss_dbus: implement API
The hunks that change src/lib/dbus/sss_dbus.exports and src/lib/dbus/sss_dbus.h belong to patch #1.
sss_dbus_send_message_ex - if you're checking all input parameters of public functions, you should also check _reply.
Sometimes the caller may not need to be interested in reply - mainly for methods that don't return any value (just dbus error).
Yeah, I realized that after I wrote this comment, just forgot to remove the comment from the e-mail..sorry
Why does sss_dbus_fetch_attr() return "***_attrs" ? Isn't the return value supposed to be a single attribute, not an array? There is a similar issue with sss_dbus_parse_attr().
This was first done that way, because was expanding "extra attributes" instead of having them in a hash table. Therefore asking for one attribute may resulted in having a set of attribute.
I kept it this way, even though it is not strictly needed now, because I wanted to kept the structure opaque and you need to ask to convert it to a specific data type.
There would have been more getter functions, that would work with single attribute.
I think having it unified is a benefit here - you can simply switch from working with one attribute to all attribute.
OK.
You should always wrap multi-line macros with "do-while(0);" See: http://gcc.gnu.org/onlinedocs/cpp/Swallowing-the-Semicolon.html I know it's not sctrictly needed with the way you use the macros, but it's still best practice.
I don't like the macros in sss_dbus_parser.c, a macro that changes the flow of a program makes it quite hard to debug. Any chance the macro could just set an error code instead?
Given how the macro is used I think this is an acceptable exception.
But if you strongly disagree, I will think about it.
Please add the do-while.
sss_dbus_parse_basic(): Instead of using the default handler, you can set ret to INVALID on initialization and then not use the default handler at all. The added bonus is that if you extend the type enum but forget to add the new type to the switch-case, you'll get a warning.
Those are DBUS types that are #defined, so I'd stick with current code here instead of having ret initialized.
Why is there both sss_dbus_parse_single_attr() and sss_dbus_parse_attr()?
The parser code is the same for single attribute and list of attributes (from dict_entry). I haven't came up with a better name for it, since it is named as what it does.
There is an extra semicolon after "case DBUS_TYPE_OBJECT_PATH:"
It is required by the language grammar, since a label cannot be followed by declaration. I could move de declaration to the beginning of the function but I wanted to keep it there for clarity.
- build patch:
in sssd.spec.in, you don't need to add BuildRequires: dbus-devel, we already require dbus-devel for core SSSD. I also prefer to have BuildRequires all at the top, because they apply to the SRPM, not the binary RPMs.
libsss_dbus-devel must require fully-versioned libsss_dbus.
src/lib/dbus/sss_dbus.pc.in: I think you can drop prefix and exec_prefix, right? Only includedir and libdir are used.
- unit test patch:
Please squash the hunk that modifies src/lib/dbus/sss_dbus.c into an earlier patch.
The tests themselves look good to me, I'm glad we've finally acquired the habit of unit-testing the code if possible.
- sss_dbus: add support for string dictionary
Again, the BuildRequires is already required by core sssd. You should also drop the explicit dhash Requires, RPM will just pick that up.
In the "done" handler of sss_dbus_parse_array(), can you either split the for so that each part is on its own line or all on one? This way seems unreadable to me. I also prefer: for (int i; ;;) { /* stuff */ } over: int i; for (i; ;;) { /* stuff */ } In the context of this done: handler, the scope of 'i' would be the same.
After applying this patch, valgrind shows a leak in the unit test: ==26658== 4 bytes in 1 blocks are definitely lost in loss record 1 of 8 ==26658== at 0x4C2745D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==26658== by 0x40B148: default_alloc (sss_dbus.c:32) ==26658== by 0x40AC29: sss_dbus_alloc_zero (sss_dbus_utils.c:29) ==26658== by 0x40AE07: sss_dbus_strdup (sss_dbus_utils.c:84) ==26658== by 0x40A79C: sss_dbus_parse_dict (sss_dbus_parser.c:151) ==26658== by 0x40A406: sss_dbus_parse_array (sss_dbus_parser.c:411) ==26658== by 0x4093D2: sss_dbus_parse_variant (sss_dbus_parser.c:472) ==26658== by 0x408D58: sss_dbus_parse_single_attr (sss_dbus_parser.c:512) ==26658== by 0x408C35: sss_dbus_parse_attr (sss_dbus_parser.c:546) ==26658== by 0x403C72: test_sss_dbus_parse_attr_string_dict (test_sss_dbus.c:661) ==26658== by 0x4E345B9: _run_test (in /usr/lib64/libcmocka.so.0.1.2) ==26658== by 0x4E34A29: _run_tests (in /usr/lib64/libcmocka.so.0.1.2)
- sss_dbus: add shortcuts for common use cases
I wonder if the functions specific to an object could be kept private to the LMI code. Is the CIFS module going to use these? I see the point and I see the code can be handy, but I really think we'd be asked to provide similar wrappers for groups and all other object we're going to expose on the bus.
I think fetch_object_by_attr and fetch_object_by_name are fine, I'm not so sure about list_domains anymore. If you think these functions should stay, we need a good explanation in the docs on why we are supporting users and not groups.
Actually I don't use those functions at all so I'm fine with removing them. I didn't want them to be there for this precise reason.
Sumit asked for them.
Sumit, since Pavel doesn't use the object and user shorthands, would you be OK to keep them in your code? Do you agree with not 'leaking' the per-object functions to the public interface and keep it as simple as possible or do you see it would cause problems for you?
There is a new leak after applying the patch: ==1047== 664 (184 direct, 480 indirect) bytes in 1 blocks are definitely lost in loss record 17 of 17 ==1047== at 0x4C291D4: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==1047== by 0x5B11D65: dbus_message_new_empty_header (dbus-message.c:1140) ==1047== by 0x5B12B21: dbus_message_new_method_call (dbus-message.c:1271) ==1047== by 0x40C2EF: sss_dbus_create_message (sss_dbus.c:176) ==1047== by 0x40C5B7: sss_dbus_create_prop_msg (sss_dbus.c:49) ==1047== by 0x40C4AE: sss_dbus_fetch_attr (sss_dbus.c:248) ==1047== by 0x4063BB: test_sss_dbus_fetch_attr (test_sss_dbus.c:1335) ==1047== by 0x4E345B9: _run_test (in /usr/lib64/libcmocka.so.0.1.2) ==1047== by 0x4E34A29: _run_tests (in /usr/lib64/libcmocka.so.0.1.2) ==1047== by 0x408924: main (test_sss_dbus.c:1927) ==1047==
Hmm, the dbus message is correctly unrefed in sss_dbus_fetch_attr(), this leak should not occur. Is it possible that it is due to the fact that the message is actually not sent over the dbus?
- Add libsss_dbus.so to dlopen test
ACK
- Rename libsss_dbus to libsss_simpleifp
Looks OK, but why is it a separate patch, was it too hard to roll into the previous ones? Please note I'm fine with a separate patch, I'd just like to see the reason.
I think it would be possible to squash it, I didn't want to do it in a case we change the opinion back during the review :-)
If it's easy, then please squash it, if not, I'm fine with a separate patch.
Squash.
All concerns should be fixed (but the memory leak in test). I also changed the prefix to sss_sifp as Sumit asked (sending as separate patch).
On (19/05/14 22:40), Pavel Březina wrote:
On 05/16/2014 03:32 PM, Jakub Hrozek wrote:
On Fri, May 16, 2014 at 02:54:55PM +0200, Pavel Březina wrote:
On 05/16/2014 02:04 PM, Jakub Hrozek wrote:
On Thu, May 15, 2014 at 09:23:46PM +0200, Pavel Březina wrote:
On 05/15/2014 09:09 PM, Pavel Březina wrote:
On 05/13/2014 05:43 PM, Pavel Březina wrote: >On 05/13/2014 11:18 AM, Pavel Březina wrote: >>On 05/01/2014 05:39 PM, 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, >>I renamed the library to libsss_simpleifp.so and changed the thing that >>Stef wanted. > >I forgot to commit last change, new patch set is attached.
Sending new patchset that fixes few packaging issues found by Jan Šafránek from OpenLMI.
And once again I was too fast. I forgot to commit an issue that documentation html since it was generated from missing file (the path pointed to the old location in sss_client).
Now should be everything all right.
- sss_dbus: introduce API
Did you consider using a different base for the errors, rather than 0 or do you think it's not needed because the error is not an int but an enum?
It is enough to have zero based enum here in my opinion. All functions return proper error code. But I can change it if you want.
I think it's fine. It might be nice to add a strerror function in a later patch, though.
I think there needs to be a really verbose comment about the purpose of the library. We don't want the library to be used outside the simplest cases. As Stef already said earlier, the DBus interface is /the/ interface.
OK. I'll try to craft something.
sss_dbus_object -- would it help for the 'interfaces' to be an array? After all, an object should support multiple interfaces.
How would we handle interfaces that does not have distinct properties and method names?
The main purpose of this struct was to have all information about an object on one place with simple access to name and object path.
However I would be fine to drop it, since I don't actually use it in OpenLMI... it is used merely in the common functions.
Ah, then it's fine. I think I misunderstood the field in the structure.
Why is sss_dbus_bool typedef-ed as uint32_t and not the standard C bool? You're treating the types differently later on anyway.
I think it should be more an alias to dbus_bool_t (I didn't need to include dbus.h before so I just used appropriate type for it).
But I think I can use bool from stdbool.h there, since we switched to C99 anyway.
I think the standard C bool would make sense. Also, does this typedef need to be part of the public header? Can the bool be an opaque type?
Since I switched to std bool then the typedef became useless so I removed it completely.
I don't think the doxygen docs on sss_dbus_send_message should say what exactly the function does, just that it's a shorthand.
- sss_dbus: implement API
The hunks that change src/lib/dbus/sss_dbus.exports and src/lib/dbus/sss_dbus.h belong to patch #1.
sss_dbus_send_message_ex - if you're checking all input parameters of public functions, you should also check _reply.
Sometimes the caller may not need to be interested in reply - mainly for methods that don't return any value (just dbus error).
Yeah, I realized that after I wrote this comment, just forgot to remove the comment from the e-mail..sorry
Why does sss_dbus_fetch_attr() return "***_attrs" ? Isn't the return value supposed to be a single attribute, not an array? There is a similar issue with sss_dbus_parse_attr().
This was first done that way, because was expanding "extra attributes" instead of having them in a hash table. Therefore asking for one attribute may resulted in having a set of attribute.
I kept it this way, even though it is not strictly needed now, because I wanted to kept the structure opaque and you need to ask to convert it to a specific data type.
There would have been more getter functions, that would work with single attribute.
I think having it unified is a benefit here - you can simply switch from working with one attribute to all attribute.
OK.
You should always wrap multi-line macros with "do-while(0);" See: http://gcc.gnu.org/onlinedocs/cpp/Swallowing-the-Semicolon.html I know it's not sctrictly needed with the way you use the macros, but it's still best practice.
I don't like the macros in sss_dbus_parser.c, a macro that changes the flow of a program makes it quite hard to debug. Any chance the macro could just set an error code instead?
Given how the macro is used I think this is an acceptable exception.
But if you strongly disagree, I will think about it.
Please add the do-while.
sss_dbus_parse_basic(): Instead of using the default handler, you can set ret to INVALID on initialization and then not use the default handler at all. The added bonus is that if you extend the type enum but forget to add the new type to the switch-case, you'll get a warning.
Those are DBUS types that are #defined, so I'd stick with current code here instead of having ret initialized.
Why is there both sss_dbus_parse_single_attr() and sss_dbus_parse_attr()?
The parser code is the same for single attribute and list of attributes (from dict_entry). I haven't came up with a better name for it, since it is named as what it does.
There is an extra semicolon after "case DBUS_TYPE_OBJECT_PATH:"
It is required by the language grammar, since a label cannot be followed by declaration. I could move de declaration to the beginning of the function but I wanted to keep it there for clarity.
- build patch:
in sssd.spec.in, you don't need to add BuildRequires: dbus-devel, we already require dbus-devel for core SSSD. I also prefer to have BuildRequires all at the top, because they apply to the SRPM, not the binary RPMs.
libsss_dbus-devel must require fully-versioned libsss_dbus.
src/lib/dbus/sss_dbus.pc.in: I think you can drop prefix and exec_prefix, right? Only includedir and libdir are used.
- unit test patch:
Please squash the hunk that modifies src/lib/dbus/sss_dbus.c into an earlier patch.
The tests themselves look good to me, I'm glad we've finally acquired the habit of unit-testing the code if possible.
- sss_dbus: add support for string dictionary
Again, the BuildRequires is already required by core sssd. You should also drop the explicit dhash Requires, RPM will just pick that up.
In the "done" handler of sss_dbus_parse_array(), can you either split the for so that each part is on its own line or all on one? This way seems unreadable to me. I also prefer: for (int i; ;;) { /* stuff */ } over: int i; for (i; ;;) { /* stuff */ } In the context of this done: handler, the scope of 'i' would be the same.
After applying this patch, valgrind shows a leak in the unit test: ==26658== 4 bytes in 1 blocks are definitely lost in loss record 1 of 8 ==26658== at 0x4C2745D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==26658== by 0x40B148: default_alloc (sss_dbus.c:32) ==26658== by 0x40AC29: sss_dbus_alloc_zero (sss_dbus_utils.c:29) ==26658== by 0x40AE07: sss_dbus_strdup (sss_dbus_utils.c:84) ==26658== by 0x40A79C: sss_dbus_parse_dict (sss_dbus_parser.c:151) ==26658== by 0x40A406: sss_dbus_parse_array (sss_dbus_parser.c:411) ==26658== by 0x4093D2: sss_dbus_parse_variant (sss_dbus_parser.c:472) ==26658== by 0x408D58: sss_dbus_parse_single_attr (sss_dbus_parser.c:512) ==26658== by 0x408C35: sss_dbus_parse_attr (sss_dbus_parser.c:546) ==26658== by 0x403C72: test_sss_dbus_parse_attr_string_dict (test_sss_dbus.c:661) ==26658== by 0x4E345B9: _run_test (in /usr/lib64/libcmocka.so.0.1.2) ==26658== by 0x4E34A29: _run_tests (in /usr/lib64/libcmocka.so.0.1.2)
- sss_dbus: add shortcuts for common use cases
I wonder if the functions specific to an object could be kept private to the LMI code. Is the CIFS module going to use these? I see the point and I see the code can be handy, but I really think we'd be asked to provide similar wrappers for groups and all other object we're going to expose on the bus.
I think fetch_object_by_attr and fetch_object_by_name are fine, I'm not so sure about list_domains anymore. If you think these functions should stay, we need a good explanation in the docs on why we are supporting users and not groups.
Actually I don't use those functions at all so I'm fine with removing them. I didn't want them to be there for this precise reason.
Sumit asked for them.
Sumit, since Pavel doesn't use the object and user shorthands, would you be OK to keep them in your code? Do you agree with not 'leaking' the per-object functions to the public interface and keep it as simple as possible or do you see it would cause problems for you?
There is a new leak after applying the patch: ==1047== 664 (184 direct, 480 indirect) bytes in 1 blocks are definitely lost in loss record 17 of 17 ==1047== at 0x4C291D4: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==1047== by 0x5B11D65: dbus_message_new_empty_header (dbus-message.c:1140) ==1047== by 0x5B12B21: dbus_message_new_method_call (dbus-message.c:1271) ==1047== by 0x40C2EF: sss_dbus_create_message (sss_dbus.c:176) ==1047== by 0x40C5B7: sss_dbus_create_prop_msg (sss_dbus.c:49) ==1047== by 0x40C4AE: sss_dbus_fetch_attr (sss_dbus.c:248) ==1047== by 0x4063BB: test_sss_dbus_fetch_attr (test_sss_dbus.c:1335) ==1047== by 0x4E345B9: _run_test (in /usr/lib64/libcmocka.so.0.1.2) ==1047== by 0x4E34A29: _run_tests (in /usr/lib64/libcmocka.so.0.1.2) ==1047== by 0x408924: main (test_sss_dbus.c:1927) ==1047==
Hmm, the dbus message is correctly unrefed in sss_dbus_fetch_attr(), this leak should not occur. Is it possible that it is due to the fact that the message is actually not sent over the dbus?
- Add libsss_dbus.so to dlopen test
ACK
- Rename libsss_dbus to libsss_simpleifp
Looks OK, but why is it a separate patch, was it too hard to roll into the previous ones? Please note I'm fine with a separate patch, I'd just like to see the reason.
I think it would be possible to squash it, I didn't want to do it in a case we change the opinion back during the review :-)
If it's easy, then please squash it, if not, I'm fine with a separate patch.
Squash.
All concerns should be fixed (but the memory leak in test). I also changed the prefix to sss_sifp as Sumit asked (sending as separate patch).
I have a few comments about exported functions in sss_simpleifp.exports
1. The function sss_sifp_send_message_exis in public header file sss_sifp.h, but it *is not* exported in sss_simpleifp.exports
sh-4.2$ git grep sss_sifp_send_message_ex src/lib/sifp/sss_sifp.c:sss_sifp_send_message_ex(sss_sifp_ctx *ctx, src/lib/sifp/sss_sifp.h: * sss_sifp_send_message_ex(ctx, msg, 5000, _reply); src/lib/sifp/sss_sifp.h:sss_sifp_send_message_ex(sss_sifp_ctx *ctx,
sh-4.2$ objdump -T .libs/libsss_simpleifp.so | grep SSS_SIMPLEIFP | grep sss_sifp_send_message 0000000000001e90 g DF .text 0000000000000010 SSS_SIMPLEIFP_0.0 sss_sifp_send_message
2. The function sss_sifp_get_iface_for_object is in private header file, but it *is* exported in sss_simpleifp.exports
sh-4.2$ objdump -T .libs/libsss_simpleifp.so | grep SSS_SIMPLEIFP | grep sss_sifp_get_iface_for_object 0000000000004760 g DF .text 000000000000006b SSS_SIMPLEIFP_0.0 sss_sifp_get_iface_for_object
sh-4.2$ git grep sss_sifp_get_iface_for_object src/lib/sifp/sss_sifp_common.c: interface = sss_sifp_get_iface_for_object(object_path); src/lib/sifp/sss_sifp_private.h:sss_sifp_get_iface_for_object(const char *object_path); src/lib/sifp/sss_sifp_utils.c:sss_sifp_get_iface_for_object(const char *object_path) src/lib/sifp/sss_simpleifp.exports: sss_sifp_get_iface_for_object;
LS
On Mon, May 19, 2014 at 10:40:40PM +0200, Pavel Březina wrote:
On 05/16/2014 03:32 PM, Jakub Hrozek wrote:
On Fri, May 16, 2014 at 02:54:55PM +0200, Pavel Březina wrote:
On 05/16/2014 02:04 PM, Jakub Hrozek wrote:
On Thu, May 15, 2014 at 09:23:46PM +0200, Pavel Březina wrote:
On 05/15/2014 09:09 PM, Pavel Březina wrote:
...
All concerns should be fixed (but the memory leak in test). I also changed the prefix to sss_sifp as Sumit asked (sending as separate patch).
Thank you, imo it more looks much more consistent.
Please find attached a small patch which should illustrate my other suggestion to remove the build time DBus dependency for the user of the library. Doc fixes and a free function for the new struct are still missing, the patch should just illustrate the idea. If you think this makes sense, I can add the missing parts if you like.
With this the dbus-1 requirement in the pc file can be removed as well because the runtime dependency for DBus will be solved by package management. Btw, I think there is an issue in the current pc file, both requirements should be in a single line.
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Thu, May 22, 2014 at 11:07:51AM +0200, Sumit Bose wrote:
On Mon, May 19, 2014 at 10:40:40PM +0200, Pavel Březina wrote:
On 05/16/2014 03:32 PM, Jakub Hrozek wrote:
On Fri, May 16, 2014 at 02:54:55PM +0200, Pavel Březina wrote:
On 05/16/2014 02:04 PM, Jakub Hrozek wrote:
On Thu, May 15, 2014 at 09:23:46PM +0200, Pavel Březina wrote:
On 05/15/2014 09:09 PM, Pavel Březina wrote:
...
All concerns should be fixed (but the memory leak in test). I also changed the prefix to sss_sifp as Sumit asked (sending as separate patch).
Thank you, imo it more looks much more consistent.
Please find attached a small patch which should illustrate my other suggestion to remove the build time DBus dependency for the user of the library. Doc fixes and a free function for the new struct are still missing, the patch should just illustrate the idea. If you think this makes sense, I can add the missing parts if you like.
With this the dbus-1 requirement in the pc file can be removed as well because the runtime dependency for DBus will be solved by package management. Btw, I think there is an issue in the current pc file, both requirements should be in a single line.
and now with the patch ...
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Thu, May 22, 2014 at 11:07:51AM +0200, Sumit Bose wrote:
On Mon, May 19, 2014 at 10:40:40PM +0200, Pavel Březina wrote:
On 05/16/2014 03:32 PM, Jakub Hrozek wrote:
On Fri, May 16, 2014 at 02:54:55PM +0200, Pavel Březina wrote:
On 05/16/2014 02:04 PM, Jakub Hrozek wrote:
On Thu, May 15, 2014 at 09:23:46PM +0200, Pavel Březina wrote:
On 05/15/2014 09:09 PM, Pavel Březina wrote:
...
All concerns should be fixed (but the memory leak in test). I also changed the prefix to sss_sifp as Sumit asked (sending as separate patch).
Thank you, imo it more looks much more consistent.
Please find attached a small patch which should illustrate my other suggestion to remove the build time DBus dependency for the user of the library. Doc fixes and a free function for the new struct are still missing, the patch should just illustrate the idea. If you think this makes sense, I can add the missing parts if you like.
With this the dbus-1 requirement in the pc file can be removed as well because the runtime dependency for DBus will be solved by package management. Btw, I think there is an issue in the current pc file, both requirements should be in a single line.
bye, Sumit
You forgot to attach the patch..
On Thu, 2014-05-22 at 11:17 +0200, Sumit Bose wrote:
On Thu, May 22, 2014 at 11:07:51AM +0200, Sumit Bose wrote:
On Mon, May 19, 2014 at 10:40:40PM +0200, Pavel Březina wrote:
On 05/16/2014 03:32 PM, Jakub Hrozek wrote:
On Fri, May 16, 2014 at 02:54:55PM +0200, Pavel Březina wrote:
On 05/16/2014 02:04 PM, Jakub Hrozek wrote:
On Thu, May 15, 2014 at 09:23:46PM +0200, Pavel Březina wrote: >On 05/15/2014 09:09 PM, Pavel Březina wrote:
...
All concerns should be fixed (but the memory leak in test). I also changed the prefix to sss_sifp as Sumit asked (sending as separate patch).
Thank you, imo it more looks much more consistent.
Please find attached a small patch which should illustrate my other suggestion to remove the build time DBus dependency for the user of the library. Doc fixes and a free function for the new struct are still missing, the patch should just illustrate the idea. If you think this makes sense, I can add the missing parts if you like.
With this the dbus-1 requirement in the pc file can be removed as well because the runtime dependency for DBus will be solved by package management. Btw, I think there is an issue in the current pc file, both requirements should be in a single line.
and now with the patch ...
I am concerned with all the abstraction around the allocation functions.
I see that by default malloc() is used, and with the use of malloc I can already see a ton of memory leaks throughout the code, look for example at sss_sifp_create_ifp_msg() in your patch, I think it will leak a struct sss_sifp_dbus_message at every invocation.
Why this complexity and pluggability of allocators ?
Simo.
On Thu, May 22, 2014 at 09:02:45AM -0400, Simo Sorce wrote:
On Thu, 2014-05-22 at 11:17 +0200, Sumit Bose wrote:
On Thu, May 22, 2014 at 11:07:51AM +0200, Sumit Bose wrote:
On Mon, May 19, 2014 at 10:40:40PM +0200, Pavel Březina wrote:
On 05/16/2014 03:32 PM, Jakub Hrozek wrote:
On Fri, May 16, 2014 at 02:54:55PM +0200, Pavel Březina wrote:
On 05/16/2014 02:04 PM, Jakub Hrozek wrote: >On Thu, May 15, 2014 at 09:23:46PM +0200, Pavel Březina wrote: >>On 05/15/2014 09:09 PM, Pavel Březina wrote:
...
All concerns should be fixed (but the memory leak in test). I also changed the prefix to sss_sifp as Sumit asked (sending as separate patch).
Thank you, imo it more looks much more consistent.
Please find attached a small patch which should illustrate my other suggestion to remove the build time DBus dependency for the user of the library. Doc fixes and a free function for the new struct are still missing, the patch should just illustrate the idea. If you think this makes sense, I can add the missing parts if you like.
With this the dbus-1 requirement in the pc file can be removed as well because the runtime dependency for DBus will be solved by package management. Btw, I think there is an issue in the current pc file, both requirements should be in a single line.
and now with the patch ...
I am concerned with all the abstraction around the allocation functions.
I see that by default malloc() is used, and with the use of malloc I can already see a ton of memory leaks throughout the code, look for example at sss_sifp_create_ifp_msg() in your patch, I think it will leak a struct sss_sifp_dbus_message at every invocation.
as said, the patch was meant to illustrate the idea, it certainly needs fixing.
Why this complexity and pluggability of allocators ?
I think it dates back when you asked if dhash can have a pluggable allocator so that talloc can be used directly. After that most the SSSD libraries for external use had this.
Do you think this should change?
bye, Sumit
Simo.
-- Simo Sorce * Red Hat, Inc * New York
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Thu, 2014-05-22 at 15:22 +0200, Sumit Bose wrote:
On Thu, May 22, 2014 at 09:02:45AM -0400, Simo Sorce wrote:
On Thu, 2014-05-22 at 11:17 +0200, Sumit Bose wrote:
On Thu, May 22, 2014 at 11:07:51AM +0200, Sumit Bose wrote:
On Mon, May 19, 2014 at 10:40:40PM +0200, Pavel Březina wrote:
On 05/16/2014 03:32 PM, Jakub Hrozek wrote:
On Fri, May 16, 2014 at 02:54:55PM +0200, Pavel Březina wrote: >On 05/16/2014 02:04 PM, Jakub Hrozek wrote: >>On Thu, May 15, 2014 at 09:23:46PM +0200, Pavel Březina wrote: >>>On 05/15/2014 09:09 PM, Pavel Březina wrote:
...
All concerns should be fixed (but the memory leak in test). I also changed the prefix to sss_sifp as Sumit asked (sending as separate patch).
Thank you, imo it more looks much more consistent.
Please find attached a small patch which should illustrate my other suggestion to remove the build time DBus dependency for the user of the library. Doc fixes and a free function for the new struct are still missing, the patch should just illustrate the idea. If you think this makes sense, I can add the missing parts if you like.
With this the dbus-1 requirement in the pc file can be removed as well because the runtime dependency for DBus will be solved by package management. Btw, I think there is an issue in the current pc file, both requirements should be in a single line.
and now with the patch ...
I am concerned with all the abstraction around the allocation functions.
I see that by default malloc() is used, and with the use of malloc I can already see a ton of memory leaks throughout the code, look for example at sss_sifp_create_ifp_msg() in your patch, I think it will leak a struct sss_sifp_dbus_message at every invocation.
as said, the patch was meant to illustrate the idea, it certainly needs fixing.
Why this complexity and pluggability of allocators ?
I think it dates back when you asked if dhash can have a pluggable allocator so that talloc can be used directly. After that most the SSSD libraries for external use had this.
Do you think this should change?
It made sense for dhash as it was a very small, contained library with a simple interface, and it made a lot of sense to be able to use it outside of sssd.
This is a sssd specific interface, so it may be ok to just use talloc.
The only 'potential' reason would be if we need to allow for thread safety on the client side (unfortunately talloc is not completely thread safe for the 'NULL' context), but we can always put in work to fix talloc :)
It is always a trade-off in the end, I just wanted to point out the code I've sen has issues though, and that concerned me.
Simo.
On 05/22/2014 03:47 PM, Simo Sorce wrote:
On Thu, 2014-05-22 at 15:22 +0200, Sumit Bose wrote:
On Thu, May 22, 2014 at 09:02:45AM -0400, Simo Sorce wrote:
On Thu, 2014-05-22 at 11:17 +0200, Sumit Bose wrote:
On Thu, May 22, 2014 at 11:07:51AM +0200, Sumit Bose wrote:
On Mon, May 19, 2014 at 10:40:40PM +0200, Pavel Březina wrote:
On 05/16/2014 03:32 PM, Jakub Hrozek wrote: > On Fri, May 16, 2014 at 02:54:55PM +0200, Pavel Březina wrote: >> On 05/16/2014 02:04 PM, Jakub Hrozek wrote: >>> On Thu, May 15, 2014 at 09:23:46PM +0200, Pavel Březina wrote: >>>> On 05/15/2014 09:09 PM, Pavel Březina wrote:
...
All concerns should be fixed (but the memory leak in test). I also changed the prefix to sss_sifp as Sumit asked (sending as separate patch).
Thank you, imo it more looks much more consistent.
Please find attached a small patch which should illustrate my other suggestion to remove the build time DBus dependency for the user of the library. Doc fixes and a free function for the new struct are still missing, the patch should just illustrate the idea. If you think this makes sense, I can add the missing parts if you like.
With this the dbus-1 requirement in the pc file can be removed as well because the runtime dependency for DBus will be solved by package management. Btw, I think there is an issue in the current pc file, both requirements should be in a single line.
and now with the patch ...
I am concerned with all the abstraction around the allocation functions.
I see that by default malloc() is used, and with the use of malloc I can already see a ton of memory leaks throughout the code, look for example at sss_sifp_create_ifp_msg() in your patch, I think it will leak a struct sss_sifp_dbus_message at every invocation.
as said, the patch was meant to illustrate the idea, it certainly needs fixing.
Why this complexity and pluggability of allocators ?
I think it dates back when you asked if dhash can have a pluggable allocator so that talloc can be used directly. After that most the SSSD libraries for external use had this.
Do you think this should change?
It made sense for dhash as it was a very small, contained library with a simple interface, and it made a lot of sense to be able to use it outside of sssd.
It's going to be used also in OpenLMI, so I don't think enforcing the allocator is a good think.
This is a sssd specific interface, so it may be ok to just use talloc.
The only 'potential' reason would be if we need to allow for thread safety on the client side (unfortunately talloc is not completely thread safe for the 'NULL' context), but we can always put in work to fix talloc :)
It is always a trade-off in the end, I just wanted to point out the code I've sen has issues though, and that concerned me.
Simo.
On 05/22/2014 11:17 AM, Sumit Bose wrote:
On Thu, May 22, 2014 at 11:07:51AM +0200, Sumit Bose wrote:
On Mon, May 19, 2014 at 10:40:40PM +0200, Pavel Březina wrote:
On 05/16/2014 03:32 PM, Jakub Hrozek wrote:
On Fri, May 16, 2014 at 02:54:55PM +0200, Pavel Březina wrote:
On 05/16/2014 02:04 PM, Jakub Hrozek wrote:
On Thu, May 15, 2014 at 09:23:46PM +0200, Pavel Březina wrote: > On 05/15/2014 09:09 PM, Pavel Březina wrote:
...
All concerns should be fixed (but the memory leak in test). I also changed the prefix to sss_sifp as Sumit asked (sending as separate patch).
Thank you, imo it more looks much more consistent.
Please find attached a small patch which should illustrate my other suggestion to remove the build time DBus dependency for the user of the library. Doc fixes and a free function for the new struct are still missing, the patch should just illustrate the idea. If you think this makes sense, I can add the missing parts if you like.
With this the dbus-1 requirement in the pc file can be removed as well because the runtime dependency for DBus will be solved by package management. Btw, I think there is an issue in the current pc file, both requirements should be in a single line.
and now with the patch ...
Well... the way it is now, this encapsulation makes the public sss_sifp_create_message() and sss_sifp_send_message() useless since you cannot append parameters to the message.
If you really dislike the dependency that much, we can just remove these functions. I like them since they made the code a little simpler, even when you append the arguments through dbus function. But I can go without them.
Or we need also to provide equivalent to dbus_message_append_args(). For example we can go one more step forward and also pass the dbus arguments in the sss_sifp_create_message(), like:
sss_sifp_create_message(ctx, path, SSS_DBUS_IFACE_COMPONENTS, "ChangeDebugLevel", DBUS_TYPE_UINT32, &level, DBUS_TYPE_INVALID);
But having it like this would really require conversion from DBUS_TYPE into SSS_SIFP_ATTR_TYPE which I wanted to avoid (in the beginning I didn't plan to use the custom types at all). It would make the library more than what I intended from the beginning. However it may not be a bad thing after all...
Your call?
On 05/22/2014 09:00 PM, Pavel Březina wrote:
On 05/22/2014 11:17 AM, Sumit Bose wrote:
On Thu, May 22, 2014 at 11:07:51AM +0200, Sumit Bose wrote:
On Mon, May 19, 2014 at 10:40:40PM +0200, Pavel Březina wrote:
On 05/16/2014 03:32 PM, Jakub Hrozek wrote:
On Fri, May 16, 2014 at 02:54:55PM +0200, Pavel Březina wrote:
On 05/16/2014 02:04 PM, Jakub Hrozek wrote: > On Thu, May 15, 2014 at 09:23:46PM +0200, Pavel Březina wrote: >> On 05/15/2014 09:09 PM, Pavel Březina wrote:
...
All concerns should be fixed (but the memory leak in test). I also changed the prefix to sss_sifp as Sumit asked (sending as separate patch).
Thank you, imo it more looks much more consistent.
Please find attached a small patch which should illustrate my other suggestion to remove the build time DBus dependency for the user of the library. Doc fixes and a free function for the new struct are still missing, the patch should just illustrate the idea. If you think this makes sense, I can add the missing parts if you like.
With this the dbus-1 requirement in the pc file can be removed as well because the runtime dependency for DBus will be solved by package management. Btw, I think there is an issue in the current pc file, both requirements should be in a single line.
and now with the patch ...
Well... the way it is now, this encapsulation makes the public sss_sifp_create_message() and sss_sifp_send_message() useless since you cannot append parameters to the message.
If you really dislike the dependency that much, we can just remove these functions. I like them since they made the code a little simpler, even when you append the arguments through dbus function. But I can go without them.
Or we need also to provide equivalent to dbus_message_append_args(). For example we can go one more step forward and also pass the dbus arguments in the sss_sifp_create_message(), like:
sss_sifp_create_message(ctx, path, SSS_DBUS_IFACE_COMPONENTS, "ChangeDebugLevel", DBUS_TYPE_UINT32, &level, DBUS_TYPE_INVALID);
But having it like this would really require conversion from DBUS_TYPE into SSS_SIFP_ATTR_TYPE which I wanted to avoid (in the beginning I didn't plan to use the custom types at all). It would make the library more than what I intended from the beginning. However it may not be a bad thing after all...
But then again a method may return a reply so we would have to wrap also dbus_message_get_args at least for basic types a that brings us deeper and deeper and sifp will at some point have to become a full dbus interface, which was not the intention.
Keeping those function and the dbus dependency makes the code using sifp cleaner even if you need a little bit extended functionality, since you can call any method through dbus but you don't haven to initialize the connection, handle reply even if it is not needed, etc.
So you can keep using sifp to simply fetch and parse object attributes and to call List* and Find* methods but you still can call any other supported method and reuse the sifp dbus connection.
On Thu, May 22, 2014 at 09:00:49PM +0200, Pavel Březina wrote:
On 05/22/2014 11:17 AM, Sumit Bose wrote:
On Thu, May 22, 2014 at 11:07:51AM +0200, Sumit Bose wrote:
On Mon, May 19, 2014 at 10:40:40PM +0200, Pavel Březina wrote:
On 05/16/2014 03:32 PM, Jakub Hrozek wrote:
On Fri, May 16, 2014 at 02:54:55PM +0200, Pavel Březina wrote:
On 05/16/2014 02:04 PM, Jakub Hrozek wrote: >On Thu, May 15, 2014 at 09:23:46PM +0200, Pavel Březina wrote: >>On 05/15/2014 09:09 PM, Pavel Březina wrote:
...
All concerns should be fixed (but the memory leak in test). I also changed the prefix to sss_sifp as Sumit asked (sending as separate patch).
Thank you, imo it more looks much more consistent.
Please find attached a small patch which should illustrate my other suggestion to remove the build time DBus dependency for the user of the library. Doc fixes and a free function for the new struct are still missing, the patch should just illustrate the idea. If you think this makes sense, I can add the missing parts if you like.
With this the dbus-1 requirement in the pc file can be removed as well because the runtime dependency for DBus will be solved by package management. Btw, I think there is an issue in the current pc file, both requirements should be in a single line.
and now with the patch ...
Well... the way it is now, this encapsulation makes the public sss_sifp_create_message() and sss_sifp_send_message() useless since you cannot append parameters to the message.
ah, ok, I see, so the purpose was to make DBusMessage available to the caller.
If you really dislike the dependency that much, we can just remove these functions. I like them since they made the code a little simpler, even when you append the arguments through dbus function. But I can go without them.
I have to admit I would like to remove them, but maybe have have different use cases in mind. I'm looking at case like:
err = sss_sifp_init(&ctx); if (err != SSS_SIFP_OK) { return 2; }
err = sss_sifp_fetch_user_by_name(ctx, username, &user); if (err != SSS_SIFP_OK) { return 3; }
err = sss_sifp_find_attr_as_string(user->attrs, attr, &str); if (err != SSS_SIFP_OK) { return 4; }
printf("[%s]=[%s]\n", attr, str);
sss_sifp_free_object(ctx, &user); sss_sifp_free(&ctx);
Maybe we can split the header file like libcurl does so that the caller can decided if he prefers sss_sifp_easy.h where dbus includes file are not required or the more flexible sss_sifp.h ?
bye, Sumit
Or we need also to provide equivalent to dbus_message_append_args(). For example we can go one more step forward and also pass the dbus arguments in the sss_sifp_create_message(), like:
sss_sifp_create_message(ctx, path, SSS_DBUS_IFACE_COMPONENTS, "ChangeDebugLevel", DBUS_TYPE_UINT32, &level, DBUS_TYPE_INVALID);
But having it like this would really require conversion from DBUS_TYPE into SSS_SIFP_ATTR_TYPE which I wanted to avoid (in the beginning I didn't plan to use the custom types at all). It would make the library more than what I intended from the beginning. However it may not be a bad thing after all...
Your call?
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On 05/23/2014 07:06 PM, Sumit Bose wrote:
On Thu, May 22, 2014 at 09:00:49PM +0200, Pavel Březina wrote:
On 05/22/2014 11:17 AM, Sumit Bose wrote:
On Thu, May 22, 2014 at 11:07:51AM +0200, Sumit Bose wrote:
On Mon, May 19, 2014 at 10:40:40PM +0200, Pavel Březina wrote:
On 05/16/2014 03:32 PM, Jakub Hrozek wrote:
On Fri, May 16, 2014 at 02:54:55PM +0200, Pavel Březina wrote: > On 05/16/2014 02:04 PM, Jakub Hrozek wrote: >> On Thu, May 15, 2014 at 09:23:46PM +0200, Pavel Březina wrote: >>> On 05/15/2014 09:09 PM, Pavel Březina wrote:
...
All concerns should be fixed (but the memory leak in test). I also changed the prefix to sss_sifp as Sumit asked (sending as separate patch).
Thank you, imo it more looks much more consistent.
Please find attached a small patch which should illustrate my other suggestion to remove the build time DBus dependency for the user of the library. Doc fixes and a free function for the new struct are still missing, the patch should just illustrate the idea. If you think this makes sense, I can add the missing parts if you like.
With this the dbus-1 requirement in the pc file can be removed as well because the runtime dependency for DBus will be solved by package management. Btw, I think there is an issue in the current pc file, both requirements should be in a single line.
and now with the patch ...
Well... the way it is now, this encapsulation makes the public sss_sifp_create_message() and sss_sifp_send_message() useless since you cannot append parameters to the message.
ah, ok, I see, so the purpose was to make DBusMessage available to the caller.
If you really dislike the dependency that much, we can just remove these functions. I like them since they made the code a little simpler, even when you append the arguments through dbus function. But I can go without them.
I have to admit I would like to remove them, but maybe have have different use cases in mind. I'm looking at case like:
err = sss_sifp_init(&ctx); if (err != SSS_SIFP_OK) { return 2; } err = sss_sifp_fetch_user_by_name(ctx, username, &user); if (err != SSS_SIFP_OK) { return 3; } err = sss_sifp_find_attr_as_string(user->attrs, attr, &str); if (err != SSS_SIFP_OK) { return 4; } printf("[%s]=[%s]\n", attr, str); sss_sifp_free_object(ctx, &user); sss_sifp_free(&ctx);Maybe we can split the header file like libcurl does so that the caller can decided if he prefers sss_sifp_easy.h where dbus includes file are not required or the more flexible sss_sifp.h ?
Ok, we can do that. How about moving the functions that requires dbus headers into sss_sifp_dbus.h?
bye, Sumit
Or we need also to provide equivalent to dbus_message_append_args(). For example we can go one more step forward and also pass the dbus arguments in the sss_sifp_create_message(), like:
sss_sifp_create_message(ctx, path, SSS_DBUS_IFACE_COMPONENTS, "ChangeDebugLevel", DBUS_TYPE_UINT32, &level, DBUS_TYPE_INVALID);
But having it like this would really require conversion from DBUS_TYPE into SSS_SIFP_ATTR_TYPE which I wanted to avoid (in the beginning I didn't plan to use the custom types at all). It would make the library more than what I intended from the beginning. However it may not be a bad thing after all...
Your call?
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Fri, May 23, 2014 at 07:09:23PM +0200, Pavel Březina wrote:
On 05/23/2014 07:06 PM, Sumit Bose wrote:
On Thu, May 22, 2014 at 09:00:49PM +0200, Pavel Březina wrote:
On 05/22/2014 11:17 AM, Sumit Bose wrote:
On Thu, May 22, 2014 at 11:07:51AM +0200, Sumit Bose wrote:
On Mon, May 19, 2014 at 10:40:40PM +0200, Pavel Březina wrote:
On 05/16/2014 03:32 PM, Jakub Hrozek wrote:
Maybe we can split the header file like libcurl does so that the caller can decided if he prefers sss_sifp_easy.h where dbus includes file are not required or the more flexible sss_sifp.h ?
Ok, we can do that. How about moving the functions that requires dbus headers into sss_sifp_dbus.h?
I'm fine with the name. Do you have time to do this, or shall I prepare a patch on top of yours?
bye, Sumit
On 05/23/2014 09:13 PM, Sumit Bose wrote:
On Fri, May 23, 2014 at 07:09:23PM +0200, Pavel Březina wrote:
On 05/23/2014 07:06 PM, Sumit Bose wrote:
On Thu, May 22, 2014 at 09:00:49PM +0200, Pavel Březina wrote:
On 05/22/2014 11:17 AM, Sumit Bose wrote:
On Thu, May 22, 2014 at 11:07:51AM +0200, Sumit Bose wrote:
On Mon, May 19, 2014 at 10:40:40PM +0200, Pavel Březina wrote: > On 05/16/2014 03:32 PM, Jakub Hrozek wrote:
Maybe we can split the header file like libcurl does so that the caller can decided if he prefers sss_sifp_easy.h where dbus includes file are not required or the more flexible sss_sifp.h ?
Ok, we can do that. How about moving the functions that requires dbus headers into sss_sifp_dbus.h?
I'm fine with the name. Do you have time to do this, or shall I prepare a patch on top of yours?
I'll do it.
On 05/23/2014 09:13 PM, Sumit Bose wrote:
On Fri, May 23, 2014 at 07:09:23PM +0200, Pavel Březina wrote:
On 05/23/2014 07:06 PM, Sumit Bose wrote:
On Thu, May 22, 2014 at 09:00:49PM +0200, Pavel Březina wrote:
On 05/22/2014 11:17 AM, Sumit Bose wrote:
On Thu, May 22, 2014 at 11:07:51AM +0200, Sumit Bose wrote:
On Mon, May 19, 2014 at 10:40:40PM +0200, Pavel Březina wrote: > On 05/16/2014 03:32 PM, Jakub Hrozek wrote:
Maybe we can split the header file like libcurl does so that the caller can decided if he prefers sss_sifp_easy.h where dbus includes file are not required or the more flexible sss_sifp.h ?
Ok, we can do that. How about moving the functions that requires dbus headers into sss_sifp_dbus.h?
I'm fine with the name. Do you have time to do this, or shall I prepare a patch on top of yours?
bye, Sumit
Hi, new patches are attached.
On Sun, May 25, 2014 at 09:52:26PM +0200, Pavel Březina wrote:
On 05/23/2014 09:13 PM, Sumit Bose wrote:
On Fri, May 23, 2014 at 07:09:23PM +0200, Pavel Březina wrote:
On 05/23/2014 07:06 PM, Sumit Bose wrote:
On Thu, May 22, 2014 at 09:00:49PM +0200, Pavel Březina wrote:
On 05/22/2014 11:17 AM, Sumit Bose wrote:
On Thu, May 22, 2014 at 11:07:51AM +0200, Sumit Bose wrote: >On Mon, May 19, 2014 at 10:40:40PM +0200, Pavel Březina wrote: >>On 05/16/2014 03:32 PM, Jakub Hrozek wrote:
Maybe we can split the header file like libcurl does so that the caller can decided if he prefers sss_sifp_easy.h where dbus includes file are not required or the more flexible sss_sifp.h ?
Ok, we can do that. How about moving the functions that requires dbus headers into sss_sifp_dbus.h?
I'm fine with the name. Do you have time to do this, or shall I prepare a patch on top of yours?
bye, Sumit
Hi, new patches are attached.
Thank you.
Please apply the following change to avoid a compiler warning:
diff --git a/src/tests/cmocka/test_sss_sifp.c b/src/tests/cmocka/test_sss_sifp.c index d22f7af..190dd76 100644 --- a/src/tests/cmocka/test_sss_sifp.c +++ b/src/tests/cmocka/test_sss_sifp.c @@ -48,7 +48,7 @@ __wrap_dbus_connection_send_with_reply_and_block(DBusConnection *connection, return NULL; }
- return sss_mock_type(DBusMessage *); + return sss_mock_ptr_type(DBusMessage *); }
In sss_sifp.h you can remove '#include <dbus/dbus.h>' and the declarations of sss_sifp_create_message(), sss_sifp_send_message(), sss_sifp_send_message_ex(), sss_sifp_invoke_list() and sss_sifp_invoke_find() because they are now available in sss_sifp_dbus.h.
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Sun, May 25, 2014 at 09:52:26PM +0200, Pavel Březina wrote:
On 05/23/2014 09:13 PM, Sumit Bose wrote:
On Fri, May 23, 2014 at 07:09:23PM +0200, Pavel Březina wrote:
On 05/23/2014 07:06 PM, Sumit Bose wrote:
On Thu, May 22, 2014 at 09:00:49PM +0200, Pavel Březina wrote:
On 05/22/2014 11:17 AM, Sumit Bose wrote:
On Thu, May 22, 2014 at 11:07:51AM +0200, Sumit Bose wrote: >On Mon, May 19, 2014 at 10:40:40PM +0200, Pavel Březina wrote: >>On 05/16/2014 03:32 PM, Jakub Hrozek wrote:
Maybe we can split the header file like libcurl does so that the caller can decided if he prefers sss_sifp_easy.h where dbus includes file are not required or the more flexible sss_sifp.h ?
Ok, we can do that. How about moving the functions that requires dbus headers into sss_sifp_dbus.h?
I'm fine with the name. Do you have time to do this, or shall I prepare a patch on top of yours?
bye, Sumit
Hi, new patches are attached.
I went through the patchset again because it changed a bit from the initial review and in general I think it's too much of code to be reviewed by a single person.
I only have some nitpicks, apart from what Sumit said: sss_sifp_send_message_ex - after dbus_connection_send_with_reply_and_block is called, should we also check reply for being non-NULL? Also, if dbus_error is set but also reply is set, we should unref reply.
In sss_sifp_alloc_zero() you have two spaces: memset(addr, '\0', size * num); ^^
But more importantly, as valgrind reveals, there are some memory leaks, at least in the tests. I don't want to block including the library for the beta because of these leaks, but please file a ticket to clean them up post-beta.
On 05/27/2014 03:44 PM, Jakub Hrozek wrote:
On Sun, May 25, 2014 at 09:52:26PM +0200, Pavel Březina wrote:
On 05/23/2014 09:13 PM, Sumit Bose wrote:
On Fri, May 23, 2014 at 07:09:23PM +0200, Pavel Březina wrote:
On 05/23/2014 07:06 PM, Sumit Bose wrote:
On Thu, May 22, 2014 at 09:00:49PM +0200, Pavel Březina wrote:
On 05/22/2014 11:17 AM, Sumit Bose wrote: > On Thu, May 22, 2014 at 11:07:51AM +0200, Sumit Bose wrote: >> On Mon, May 19, 2014 at 10:40:40PM +0200, Pavel Březina wrote: >>> On 05/16/2014 03:32 PM, Jakub Hrozek wrote:
Maybe we can split the header file like libcurl does so that the caller can decided if he prefers sss_sifp_easy.h where dbus includes file are not required or the more flexible sss_sifp.h ?
Ok, we can do that. How about moving the functions that requires dbus headers into sss_sifp_dbus.h?
I'm fine with the name. Do you have time to do this, or shall I prepare a patch on top of yours?
bye, Sumit
Hi, new patches are attached.
I went through the patchset again because it changed a bit from the initial review and in general I think it's too much of code to be reviewed by a single person.
I only have some nitpicks, apart from what Sumit said: sss_sifp_send_message_ex - after dbus_connection_send_with_reply_and_block is called, should we also check reply for being non-NULL? Also, if dbus_error is set but also reply is set, we should unref reply.
No and no. Reply is null if and only if an error is set. At least that's what is supposed to be I haven't check the dbus code.
http://dbus.freedesktop.org/doc/api/html/group__DBusConnection.html#ga8d6431...
In sss_sifp_alloc_zero() you have two spaces: memset(addr, '\0', size * num); ^^
But more importantly, as valgrind reveals, there are some memory leaks, at least in the tests. I don't want to block including the library for the beta because of these leaks, but please file a ticket to clean them up post-beta.
The only leak I see is:
==24809== 664 (184 direct, 480 indirect) bytes in 1 blocks are definitely lost in loss record 16 of 16 ==24809== at 0x4A0881C: malloc (vg_replace_malloc.c:270) ==24809== by 0x3386A17BB5: ??? (in /usr/lib64/libdbus-1.so.3.5.6) ==24809== by 0x3386A189D2: dbus_message_new_method_call (in /usr/lib64/libdbus-1.so.3.5.6) ==24809== by 0x40B7D8: sss_sifp_create_message (sss_sifp_dbus.c:63) ==24809== by 0x40BD75: sss_sifp_create_prop_msg (sss_sifp.c:44) ==24809== by 0x40C0B6: sss_sifp_fetch_attr (sss_sifp.c:160) ==24809== by 0x406065: test_sss_sifp_fetch_attr (test_sss_sifp.c:1336) ==24809== by 0x4C30767: _run_test (cmocka.c:1723) ==24809== by 0x4C30BF8: _run_tests (cmocka.c:1839) ==24809== by 0x40837C: main (test_sss_sifp.c:1928)
Unfortunately I don't know what's wrong here. It refers to a memory leak from a dbus message, however the message is (unless I am blind) unrefed. So the leak is not supposed to be there.
New patches are attached.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Tue, May 27, 2014 at 10:49:28PM +0200, Pavel Březina wrote:
On 05/27/2014 03:44 PM, Jakub Hrozek wrote:
On Sun, May 25, 2014 at 09:52:26PM +0200, Pavel Březina wrote:
On 05/23/2014 09:13 PM, Sumit Bose wrote:
On Fri, May 23, 2014 at 07:09:23PM +0200, Pavel Březina wrote:
On 05/23/2014 07:06 PM, Sumit Bose wrote:
On Thu, May 22, 2014 at 09:00:49PM +0200, Pavel Březina wrote: >On 05/22/2014 11:17 AM, Sumit Bose wrote: >>On Thu, May 22, 2014 at 11:07:51AM +0200, Sumit Bose wrote: >>>On Mon, May 19, 2014 at 10:40:40PM +0200, Pavel Březina wrote: >>>>On 05/16/2014 03:32 PM, Jakub Hrozek wrote:
Maybe we can split the header file like libcurl does so that the caller can decided if he prefers sss_sifp_easy.h where dbus includes file are not required or the more flexible sss_sifp.h ?
Ok, we can do that. How about moving the functions that requires dbus headers into sss_sifp_dbus.h?
I'm fine with the name. Do you have time to do this, or shall I prepare a patch on top of yours?
bye, Sumit
Hi, new patches are attached.
I went through the patchset again because it changed a bit from the initial review and in general I think it's too much of code to be reviewed by a single person.
I only have some nitpicks, apart from what Sumit said: sss_sifp_send_message_ex - after dbus_connection_send_with_reply_and_block is called, should we also check reply for being non-NULL? Also, if dbus_error is set but also reply is set, we should unref reply.
No and no. Reply is null if and only if an error is set. At least that's what is supposed to be I haven't check the dbus code.
http://dbus.freedesktop.org/doc/api/html/group__DBusConnection.html#ga8d6431...
I found DBus documentation to be inaccurate in the past as well. For instance, some of the functions that work with typed varargs were documented to only return false on OOM, but they also return false on type mismatch. So I learned to be paranoid.
In sss_sifp_alloc_zero() you have two spaces: memset(addr, '\0', size * num); ^^
But more importantly, as valgrind reveals, there are some memory leaks, at least in the tests. I don't want to block including the library for the beta because of these leaks, but please file a ticket to clean them up post-beta.
The only leak I see is:
==24809== 664 (184 direct, 480 indirect) bytes in 1 blocks are definitely lost in loss record 16 of 16 ==24809== at 0x4A0881C: malloc (vg_replace_malloc.c:270) ==24809== by 0x3386A17BB5: ??? (in /usr/lib64/libdbus-1.so.3.5.6) ==24809== by 0x3386A189D2: dbus_message_new_method_call (in /usr/lib64/libdbus-1.so.3.5.6) ==24809== by 0x40B7D8: sss_sifp_create_message (sss_sifp_dbus.c:63) ==24809== by 0x40BD75: sss_sifp_create_prop_msg (sss_sifp.c:44) ==24809== by 0x40C0B6: sss_sifp_fetch_attr (sss_sifp.c:160) ==24809== by 0x406065: test_sss_sifp_fetch_attr (test_sss_sifp.c:1336) ==24809== by 0x4C30767: _run_test (cmocka.c:1723) ==24809== by 0x4C30BF8: _run_tests (cmocka.c:1839) ==24809== by 0x40837C: main (test_sss_sifp.c:1928)
Unfortunately I don't know what's wrong here. It refers to a memory leak from a dbus message, however the message is (unless I am blind) unrefed. So the leak is not supposed to be there.
I couldn't find the reason either but valgrind is rarely wrong. So please file a ticket for 1.12.0 stabilisation.
New patches are attached.
They are looking good to me and I can confirm that Sumit's suggestions were fixed. ACK from me, it would be nice to see if Sumit is happy with the latest iteration as well.
I'll run the patches through Coverity in the meantime.
On Wed, May 28, 2014 at 05:09:20PM +0200, Jakub Hrozek wrote:
On Tue, May 27, 2014 at 10:49:28PM +0200, Pavel Březina wrote:
On 05/27/2014 03:44 PM, Jakub Hrozek wrote:
On Sun, May 25, 2014 at 09:52:26PM +0200, Pavel Březina wrote:
On 05/23/2014 09:13 PM, Sumit Bose wrote:
On Fri, May 23, 2014 at 07:09:23PM +0200, Pavel Březina wrote:
On 05/23/2014 07:06 PM, Sumit Bose wrote: >On Thu, May 22, 2014 at 09:00:49PM +0200, Pavel Březina wrote: >>On 05/22/2014 11:17 AM, Sumit Bose wrote: >>>On Thu, May 22, 2014 at 11:07:51AM +0200, Sumit Bose wrote: >>>>On Mon, May 19, 2014 at 10:40:40PM +0200, Pavel Březina wrote: >>>>>On 05/16/2014 03:32 PM, Jakub Hrozek wrote: > > >Maybe we can split the header file like libcurl does so that the caller can >decided if he prefers sss_sifp_easy.h where dbus includes file are not required >or the more flexible sss_sifp.h ?
Ok, we can do that. How about moving the functions that requires dbus headers into sss_sifp_dbus.h?
I'm fine with the name. Do you have time to do this, or shall I prepare a patch on top of yours?
bye, Sumit
Hi, new patches are attached.
I went through the patchset again because it changed a bit from the initial review and in general I think it's too much of code to be reviewed by a single person.
I only have some nitpicks, apart from what Sumit said: sss_sifp_send_message_ex - after dbus_connection_send_with_reply_and_block is called, should we also check reply for being non-NULL? Also, if dbus_error is set but also reply is set, we should unref reply.
No and no. Reply is null if and only if an error is set. At least that's what is supposed to be I haven't check the dbus code.
http://dbus.freedesktop.org/doc/api/html/group__DBusConnection.html#ga8d6431...
I found DBus documentation to be inaccurate in the past as well. For instance, some of the functions that work with typed varargs were documented to only return false on OOM, but they also return false on type mismatch. So I learned to be paranoid.
In sss_sifp_alloc_zero() you have two spaces: memset(addr, '\0', size * num); ^^
But more importantly, as valgrind reveals, there are some memory leaks, at least in the tests. I don't want to block including the library for the beta because of these leaks, but please file a ticket to clean them up post-beta.
The only leak I see is:
==24809== 664 (184 direct, 480 indirect) bytes in 1 blocks are definitely lost in loss record 16 of 16 ==24809== at 0x4A0881C: malloc (vg_replace_malloc.c:270) ==24809== by 0x3386A17BB5: ??? (in /usr/lib64/libdbus-1.so.3.5.6) ==24809== by 0x3386A189D2: dbus_message_new_method_call (in /usr/lib64/libdbus-1.so.3.5.6) ==24809== by 0x40B7D8: sss_sifp_create_message (sss_sifp_dbus.c:63) ==24809== by 0x40BD75: sss_sifp_create_prop_msg (sss_sifp.c:44) ==24809== by 0x40C0B6: sss_sifp_fetch_attr (sss_sifp.c:160) ==24809== by 0x406065: test_sss_sifp_fetch_attr (test_sss_sifp.c:1336) ==24809== by 0x4C30767: _run_test (cmocka.c:1723) ==24809== by 0x4C30BF8: _run_tests (cmocka.c:1839) ==24809== by 0x40837C: main (test_sss_sifp.c:1928)
Unfortunately I don't know what's wrong here. It refers to a memory leak from a dbus message, however the message is (unless I am blind) unrefed. So the leak is not supposed to be there.
I couldn't find the reason either but valgrind is rarely wrong. So please file a ticket for 1.12.0 stabilisation.
New patches are attached.
They are looking good to me and I can confirm that Sumit's suggestions were fixed. ACK from me, it would be nice to see if Sumit is happy with the latest iteration as well.
yes, they look good, so ACK form my side as well.
I just wonder how we can handle '-version-info 0:0:0' during the beta phase. Do we have to change it for the release if we do changes to the library or are we allowed to say that changes may happen during beta without updating version-info?
bye, Sumit
I'll run the patches through Coverity in the meantime. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (28/05/14 18:29), Sumit Bose wrote:
On Wed, May 28, 2014 at 05:09:20PM +0200, Jakub Hrozek wrote:
On Tue, May 27, 2014 at 10:49:28PM +0200, Pavel Březina wrote:
On 05/27/2014 03:44 PM, Jakub Hrozek wrote:
On Sun, May 25, 2014 at 09:52:26PM +0200, Pavel Březina wrote:
On 05/23/2014 09:13 PM, Sumit Bose wrote:
On Fri, May 23, 2014 at 07:09:23PM +0200, Pavel Březina wrote: >On 05/23/2014 07:06 PM, Sumit Bose wrote: >>On Thu, May 22, 2014 at 09:00:49PM +0200, Pavel Březina wrote: >>>On 05/22/2014 11:17 AM, Sumit Bose wrote: >>>>On Thu, May 22, 2014 at 11:07:51AM +0200, Sumit Bose wrote: >>>>>On Mon, May 19, 2014 at 10:40:40PM +0200, Pavel Březina wrote: >>>>>>On 05/16/2014 03:32 PM, Jakub Hrozek wrote: >> >> >>Maybe we can split the header file like libcurl does so that the caller can >>decided if he prefers sss_sifp_easy.h where dbus includes file are not required >>or the more flexible sss_sifp.h ? > >Ok, we can do that. How about moving the functions that requires dbus >headers into sss_sifp_dbus.h? >
I'm fine with the name. Do you have time to do this, or shall I prepare a patch on top of yours?
bye, Sumit
Hi, new patches are attached.
I went through the patchset again because it changed a bit from the initial review and in general I think it's too much of code to be reviewed by a single person.
I only have some nitpicks, apart from what Sumit said: sss_sifp_send_message_ex - after dbus_connection_send_with_reply_and_block is called, should we also check reply for being non-NULL? Also, if dbus_error is set but also reply is set, we should unref reply.
No and no. Reply is null if and only if an error is set. At least that's what is supposed to be I haven't check the dbus code.
http://dbus.freedesktop.org/doc/api/html/group__DBusConnection.html#ga8d6431...
I found DBus documentation to be inaccurate in the past as well. For instance, some of the functions that work with typed varargs were documented to only return false on OOM, but they also return false on type mismatch. So I learned to be paranoid.
In sss_sifp_alloc_zero() you have two spaces: memset(addr, '\0', size * num); ^^
But more importantly, as valgrind reveals, there are some memory leaks, at least in the tests. I don't want to block including the library for the beta because of these leaks, but please file a ticket to clean them up post-beta.
The only leak I see is:
==24809== 664 (184 direct, 480 indirect) bytes in 1 blocks are definitely lost in loss record 16 of 16 ==24809== at 0x4A0881C: malloc (vg_replace_malloc.c:270) ==24809== by 0x3386A17BB5: ??? (in /usr/lib64/libdbus-1.so.3.5.6) ==24809== by 0x3386A189D2: dbus_message_new_method_call (in /usr/lib64/libdbus-1.so.3.5.6) ==24809== by 0x40B7D8: sss_sifp_create_message (sss_sifp_dbus.c:63) ==24809== by 0x40BD75: sss_sifp_create_prop_msg (sss_sifp.c:44) ==24809== by 0x40C0B6: sss_sifp_fetch_attr (sss_sifp.c:160) ==24809== by 0x406065: test_sss_sifp_fetch_attr (test_sss_sifp.c:1336) ==24809== by 0x4C30767: _run_test (cmocka.c:1723) ==24809== by 0x4C30BF8: _run_tests (cmocka.c:1839) ==24809== by 0x40837C: main (test_sss_sifp.c:1928)
Unfortunately I don't know what's wrong here. It refers to a memory leak from a dbus message, however the message is (unless I am blind) unrefed. So the leak is not supposed to be there.
I couldn't find the reason either but valgrind is rarely wrong. So please file a ticket for 1.12.0 stabilisation.
New patches are attached.
They are looking good to me and I can confirm that Sumit's suggestions were fixed. ACK from me, it would be nice to see if Sumit is happy with the latest iteration as well.
yes, they look good, so ACK form my side as well.
I just wonder how we can handle '-version-info 0:0:0' during the beta phase. Do we have to change it for the release if we do changes to the library or are we allowed to say that changes may happen during beta without updating version-info?
Numbers are cheap :-) It will be beter to increment numbers.
LS
On Wed, May 28, 2014 at 06:29:42PM +0200, Sumit Bose wrote:
They are looking good to me and I can confirm that Sumit's suggestions were fixed. ACK from me, it would be nice to see if Sumit is happy with the latest iteration as well.
yes, they look good, so ACK form my side as well.
Pushed all to master: 7a6a7db9a535d6cceb61c080594c67feccbcea12 0bb98b7700b1b61f5b0a20b93279d5c2c391007f efa6c1f75c4c18bcc148d6e7efd429c2d56499ad f43c6a9ae2aea13b7a83fd932139f9352efbfcad a7e27c11866a48742bb70564b88e15bf15e9367d 91cf6f4c6069d6aff01aab171825e83a1a669e2f 931b39232b82be374256d8532d86903fe1d87bf2
On 05/28/2014 05:09 PM, Jakub Hrozek wrote:
On Tue, May 27, 2014 at 10:49:28PM +0200, Pavel Březina wrote:
On 05/27/2014 03:44 PM, Jakub Hrozek wrote:
On Sun, May 25, 2014 at 09:52:26PM +0200, Pavel Březina wrote:
On 05/23/2014 09:13 PM, Sumit Bose wrote:
On Fri, May 23, 2014 at 07:09:23PM +0200, Pavel Březina wrote:
On 05/23/2014 07:06 PM, Sumit Bose wrote: > On Thu, May 22, 2014 at 09:00:49PM +0200, Pavel Březina wrote: >> On 05/22/2014 11:17 AM, Sumit Bose wrote: >>> On Thu, May 22, 2014 at 11:07:51AM +0200, Sumit Bose wrote: >>>> On Mon, May 19, 2014 at 10:40:40PM +0200, Pavel Březina wrote: >>>>> On 05/16/2014 03:32 PM, Jakub Hrozek wrote: > > > Maybe we can split the header file like libcurl does so that the caller can > decided if he prefers sss_sifp_easy.h where dbus includes file are not required > or the more flexible sss_sifp.h ?
Ok, we can do that. How about moving the functions that requires dbus headers into sss_sifp_dbus.h?
I'm fine with the name. Do you have time to do this, or shall I prepare a patch on top of yours?
bye, Sumit
Hi, new patches are attached.
I went through the patchset again because it changed a bit from the initial review and in general I think it's too much of code to be reviewed by a single person.
I only have some nitpicks, apart from what Sumit said: sss_sifp_send_message_ex - after dbus_connection_send_with_reply_and_block is called, should we also check reply for being non-NULL? Also, if dbus_error is set but also reply is set, we should unref reply.
No and no. Reply is null if and only if an error is set. At least that's what is supposed to be I haven't check the dbus code.
http://dbus.freedesktop.org/doc/api/html/group__DBusConnection.html#ga8d6431...
I found DBus documentation to be inaccurate in the past as well. For instance, some of the functions that work with typed varargs were documented to only return false on OOM, but they also return false on type mismatch. So I learned to be paranoid.
In sss_sifp_alloc_zero() you have two spaces: memset(addr, '\0', size * num); ^^
But more importantly, as valgrind reveals, there are some memory leaks, at least in the tests. I don't want to block including the library for the beta because of these leaks, but please file a ticket to clean them up post-beta.
The only leak I see is:
==24809== 664 (184 direct, 480 indirect) bytes in 1 blocks are definitely lost in loss record 16 of 16 ==24809== at 0x4A0881C: malloc (vg_replace_malloc.c:270) ==24809== by 0x3386A17BB5: ??? (in /usr/lib64/libdbus-1.so.3.5.6) ==24809== by 0x3386A189D2: dbus_message_new_method_call (in /usr/lib64/libdbus-1.so.3.5.6) ==24809== by 0x40B7D8: sss_sifp_create_message (sss_sifp_dbus.c:63) ==24809== by 0x40BD75: sss_sifp_create_prop_msg (sss_sifp.c:44) ==24809== by 0x40C0B6: sss_sifp_fetch_attr (sss_sifp.c:160) ==24809== by 0x406065: test_sss_sifp_fetch_attr (test_sss_sifp.c:1336) ==24809== by 0x4C30767: _run_test (cmocka.c:1723) ==24809== by 0x4C30BF8: _run_tests (cmocka.c:1839) ==24809== by 0x40837C: main (test_sss_sifp.c:1928)
Unfortunately I don't know what's wrong here. It refers to a memory leak from a dbus message, however the message is (unless I am blind) unrefed. So the leak is not supposed to be there.
I couldn't find the reason either but valgrind is rarely wrong. So please file a ticket for 1.12.0 stabilisation.
https://fedorahosted.org/sssd/ticket/2342
I think it may be only test related, since the message is never send there. Maybe it hits some bug in dbus.
New patches are attached.
They are looking good to me and I can confirm that Sumit's suggestions were fixed. ACK from me, it would be nice to see if Sumit is happy with the latest iteration as well.
I'll run the patches through Coverity in the meantime. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel@lists.fedorahosted.org
-
Dmitri Pal -
Jakub Hrozek -
Lukas Slebodnik -
Pavel Březina -
Simo Sorce -
Stef Walter -
Sumit Bose