[SSSD] [PATCH] libsss_dbus

Pavel Březina pbrezina at redhat.com
Mon May 5 15:08:39 UTC 2014


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.




More information about the sssd-devel mailing list