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