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.