[SSSD] [PATCH] libsss_dbus

Stef Walter stefw at redhat.com
Mon May 5 12:06:40 UTC 2014


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



More information about the sssd-devel mailing list