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(a)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(a)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(a)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?