[SSSD] [PATCH] First batch of infrastructure patches for review

Jakub Hrozek jhrozek at redhat.com
Mon Jan 13 21:43:43 UTC 2014


On Fri, Jan 10, 2014 at 12:26:59PM +0100, Stef Walter wrote:
> As was discussed yesterday, I'm working on the sssd dbus infrastructure,
> so we can have rich access to sssd information via Dbus. Jakub has been
> working on implementing methods that expose that.
> 
> These are infrastructure patches, that take the basic DBus support that
> sssd has (ie: sbus) and start to mature it so that we can handle things
> like properties, NNN-instances of objects, automatic introspection,
> object manager, automatic unpacking of args for handlers, and so on.
> 
> As I mentioned earlier, some of these concepts may seem foreign, for
> example using DBus interface definitions to drive things.
> 
> However this has many benefits as we get into implementing more dbus stuff:
> 
>  * Removes boiler-plate all over the place.
>  * Lets the compiler check dbus interface implementations and usage.
>  * Makes it possible to do complex stuff (PropertiesChanged or
>    ObjectManager) that would otherwise be insane.
> 
> I have tried to balance things and not go crazy with changing style all
> over the place and keep to the sssd way of doing things most of the time.
> 
> Yes, this does involve a codegen, much like flex or bison. It has real
> concrete benefits. While it is possible to handcraft the necessary meta
> data structs (see patch 0001), you lose compile-time checking and some
> robustness.
> 
> The codegen generates as little code as possible, and we have tests to
> verify that it is correct (thanks Sumit for that idea).
> 
> It's important to note that an interface, and an implemented instance of
> that interface are two different things.
> 
> There's a lot more to do. Next up will be implementing automatic
> introspection generation, and property access.
> 
> If you prefer to access this as a branch, see:
> 
> https://github.com/stefwalter/sssd/tree/dbus-infra
> 
> Cheers,
> 
> Stef

Hi Stef,

even though I don't like code generation normally, I think in this case
the generated code is clean and readable. I think the benefits clearly
overweight the fear of 'generated code', especially considering the more
advanced parsers and getters/setters that are coming up in the future.

The code itself looks very good to me as well, I was just wondering about
several details when I read the code, most of which are about making sure
the resulting codebase is at least as easy to read and navigate as the
old one. For example, I wonder if there would be any benefit in naming the
resulting *.[ch] files so that it's easy to recognize they've been generated?

See some more comments (mostly just nitpicks) inline:

> Subject: [PATCH 1/6] sbus: Add meta data structures and code generator

[snip detailed and useful commit message] 

> +# -----------------------------------------------------------------------------
> +# Objects
> +
> +class DBusXmlException(Exception):
> +    line = 0
> +    file = None
> +
> +    # Lets us print problems like a compiler would
> +    def __str__(self):
> +        message = Exception.__str__(self)
> +        if self.file and self.line:
> +            return "%s:%d: %s" % (self.file, self.line, message)
> +        elif self.file:
> +            return "%s: %s" % (self.file, message)
> +        else:
> +            return message
> +
> +class Base:
> +    def __init__(self, name):
> +        if not name:
> +            raise DBusXmlException('No name on element')
> +        self.name = name
> +        self.annotations = { }
> +    def c_name(self):
> +        return self.annotations.get("org.freedesktop.DBus.GLib.CSymbol", self.name)
> +
> +class Arg(Base):
> +    def __init__(self, method, name, signature):
> +        Base.__init__(self, name)
> +        self.method = method
> +        self.signature = signature
> +
> +class Method(Base):
> +    def __init__(self, iface, name):
> +        Base.__init__(self, name)
> +        self.iface = iface
> +        self.in_args = []
> +        self.out_args = []
> +    def fq_c_name(self):
> +        return "%s_%s" % (self.iface.c_name(), self.c_name())
> +    def use_raw_handler(self):
> +        anno = 'org.freedesktop.sssd.RawHandler'
> +        return self.annotations.get(anno, self.iface.annotations.get(anno)) == 'true'

What is the RawHandler good for? I don't see it used anywhere..

[snip]

> +
> +def source_interface(iface):
> +    print "/* interface info for %s */" % iface.name
> +    print "const struct sbus_interface_meta %s_meta = {" % iface.c_name();
> +    print "    \"%s\", /* name */" % iface.name
> +    if iface.methods:
> +        print "    %s__methods, " % iface.c_name()

This print inserts a trailing whitespace into the generated code. Not a
big deal, but my editor lights up whenever I open a generated file :-)

> +    else:
> +        print "    NULL, /* no methods */"
> +    if iface.signals:
> +        print "    %s__signals, " % iface.c_name()

Same trailing whitespace is generated here.

> +    else:
> +        print "    NULL, /* no signals */"
> +    if iface.properties:
> +        print "    %s__properties" % iface.c_name()
> +    else:
> +        print "    NULL, /* no propetries */"
> +    print "};"
> +    print
> +

> +++ b/src/sbus/sssd_dbus_meta.c

[snip]

> +const struct sbus_method_meta *
> +sbus_meta_find_method (const struct sbus_interface_meta *interface,
> +                       const char *method_name)

The leading space before the opening bracket is not common in the SSSD
source code. Would you agree with squashing the attached mini-patch that
removes the space?

The code itself looks fine to me.

> +{
> +    const struct sbus_method_meta *method;
> +
> +    for (method = interface->methods; method && method->name; method++) {
> +        if (strcmp (method_name, method->name) == 0)
> +            return method;
> +    }
> +
> +    return NULL;
> +}
> +

> index 0000000..8878d9b
> --- /dev/null
> +++ b/src/tests/sbus_codegen-tests.c
> @@ -0,0 +1,166 @@
> +/*
> +   SSSD
> +
> +   Reference counting tests.

Copy-n-paste bug :-)

The test itself looks fine to me.

> +
> +   Authors:
> +        Stef Walter <stefw at redhat.com>
> +
> +   Copyright (C) Red Hat, Inc 2014
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +*/

> Subject: [PATCH 2/6] sbus: Add sbus_vtable and update codegen to support it
> 
> Each interface is a vtable structure derived from
> sbus_vtable, in the sense that it has an sbus_vtable
> struct as its first argument. This lets us upcast the
> interface vtable structure to an sbus_vtable and dispatch
> to it dynamically and cleanly.
> 
> The interface metadata contains information about which
> vtable offset in the interface metadata should be dispatched
> to for a given function. This is a common scheme, not only
> among dbus implementations, but also compiled languages.
> 
> Currently all the vtable functions are of type
> sbus_msg_handler_fn. These are the handlers we are familiar
> with and perform raw processing of the message. Later commits
> will introduce type safe handlers, that levelage compile checking
> and automatic argument packing/unpacking.
> 
> Although this may seem contrived now, the remainder of the
> dbus infrastructure work will build on this, including
> ofd.Properties, ofd.ObjectManager, ofd.Introspect, compiler
> checked type safe unpacking/packing, etc.
> 
> The codegen now generates vtable structures for each interface
> along-side the metadata, and fills them in appropriately.
> 
> It is obviously still possible to hand-craft such vtables and
> metadata if needed for a special case.
> 
> Once again examples output can be found at:
> 
>   $(builddir)/src/tests/sbus_codegen-defs.h
> 
> You'll likely need to 'make clean' after applying this patch
> as this modifies the codegen.

ACK to the approach especially considering the future work!

I'm thinking about one aspect that might be just my personal preference
so I'd like to hear other opinions. I'm completely addicted to
cscope/ctags so I wonder how to make life easier for developers now that
there are source files outside srcdir. Perhaps it would be enough to
export $SOURCEDIRS (see man cscope) set to builddir in the bashrc_sssd script?

I really don't have any comments about the code itself.

> Subject: [PATCH 3/6] nss: Stop using one DBus interface with totally different
>  methods
> 
> This is an incorrect use of DBus, where we use a single interface
> name with completely different sets of methods.
> 
> Easily fixed.
> 
> Once the vtable stuff is in use then this would be automatically
> detected and fail to build.

ACK

> From 4edab640ba714cd42b33feecfc63d7737d1a5140 Mon Sep 17 00:00:00 2001
> From: Stef Walter <stefw at redhat.com>
> Date: Fri, 10 Jan 2014 08:58:12 +0100
> Subject: [PATCH 4/6] sbus: Rework sbus to use interface metadata and vtables
> 

In my opinion these changes make it less clear which C function is called
in response to which sbus method. I think it's mostly a matter of adding
a comment that would say to go inspect the "meta" structure for
mappings. Alternatively, we could add the method names as a comment for
each line with a function call, but this defeats the intent of having
all the metadata on one place, currently the generated file.

> -struct sbus_method monitor_methods[] = {
> -    { MON_SRV_METHOD_VERSION, get_monitor_version },
> -    { MON_SRV_METHOD_REGISTER, client_registration },
> -    { NULL, NULL }
> +struct mon_srv_iface monitor_methods = {
> +    { &mon_srv_iface_meta, 0 },
> +    get_monitor_version,
> +    client_registration,
>  };
>  
>  struct sbus_interface monitor_server_interface = {
> -    MON_SRV_INTERFACE,
>      MON_SRV_PATH,
> -    SBUS_DEFAULT_VTABLE,
> -    monitor_methods,
> +    &monitor_methods.vtable,
>      NULL
>  };

But that's really the only comment I have. I haven't tested all sbus
methods either, but the basic stuff seems to work. I applied the patches
in my local tree and I'm running git HEAD + these patches at the moment
to catch any issues there might be.

> From 4ff481788da1a164976010b8a50ec794c3b7975c Mon Sep 17 00:00:00 2001
> From: Stef Walter <stefw at redhat.com>
> Date: Fri, 10 Jan 2014 11:08:16 +0100
> Subject: [PATCH 5/6] sbus: Generate constants from interface definitions
> Subject: [PATCH 6/6] sbus: Use constants to make dbus calls

ACK to the idea.

One question -- the old version had _METHOD_ embedded in the constant
which made it easy to see the constant describes a method.

Did you consider it redundant info when doing the change?
-------------- next part --------------
>From d08f72441cdb2278283230f431c87a23f2969109 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Mon, 13 Jan 2014 16:38:16 +0100
Subject: [PATCH] Fixup - coding style

---
 src/sbus/sssd_dbus_meta.c      | 12 ++++++------
 src/tests/sbus_codegen-tests.c |  8 ++++----
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/sbus/sssd_dbus_meta.c b/src/sbus/sssd_dbus_meta.c
index 3776c687d8a2e16de94626aa6746cdcc63954fd3..54dd975c34f18e6df09b8ea1042d66668fd6648d 100644
--- a/src/sbus/sssd_dbus_meta.c
+++ b/src/sbus/sssd_dbus_meta.c
@@ -22,8 +22,8 @@
 #include "sbus/sssd_dbus_meta.h"
 
 const struct sbus_method_meta *
-sbus_meta_find_method (const struct sbus_interface_meta *interface,
-                       const char *method_name)
+sbus_meta_find_method(const struct sbus_interface_meta *interface,
+                      const char *method_name)
 {
     const struct sbus_method_meta *method;
 
@@ -36,8 +36,8 @@ sbus_meta_find_method (const struct sbus_interface_meta *interface,
 }
 
 const struct sbus_signal_meta *
-sbus_meta_find_signal (const struct sbus_interface_meta *interface,
-                       const char *signal_name)
+sbus_meta_find_signal(const struct sbus_interface_meta *interface,
+                      const char *signal_name)
 {
     const struct sbus_signal_meta *signal;
 
@@ -50,8 +50,8 @@ sbus_meta_find_signal (const struct sbus_interface_meta *interface,
 }
 
 const struct sbus_property_meta *
-sbus_meta_find_property (const struct sbus_interface_meta *interface,
-                         const char *property_name)
+sbus_meta_find_property(const struct sbus_interface_meta *interface,
+                        const char *property_name)
 {
     const struct sbus_property_meta *property;
 
diff --git a/src/tests/sbus_codegen-tests.c b/src/tests/sbus_codegen-tests.c
index 8878d9bab35765ffbd13fa808de0420aba1d8256..0c70d6718a9f0bdc75f5d25e863beb855f6dc216 100644
--- a/src/tests/sbus_codegen-tests.c
+++ b/src/tests/sbus_codegen-tests.c
@@ -32,8 +32,8 @@
 #include "tests/sbus_codegen-defs.h"
 
 static const struct sbus_arg_meta *
-find_arg (const struct sbus_arg_meta *args,
-          const char *name)
+find_arg(const struct sbus_arg_meta *args,
+         const char *name)
 {
     const struct sbus_arg_meta *arg;
     for (arg = args; arg->name != NULL; arg++) {
@@ -144,8 +144,8 @@ int main(int argc, const char *argv[])
     };
 
     pc = poptGetContext(argv[0], argc, argv, long_options, 0);
-    while((opt = poptGetNextOpt(pc)) != -1) {
-        switch(opt) {
+    while ((opt = poptGetNextOpt(pc)) != -1) {
+        switch (opt) {
         default:
             fprintf(stderr, "\nInvalid option %s: %s\n\n",
                     poptBadOption(pc, 0), poptStrerror(opt));
-- 
1.8.4.2



More information about the sssd-devel mailing list