On 13.01.2014 22:43, Jakub Hrozek wrote:
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.
...
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?
Sure. I've made a 'make codegen' target, and the generated files have
-generated.[ch] suffixes. They're now checked in.
They still automatically regenerate if either the code generator or the
the XML file change. Initially I tried to do without this, but it was
really fragile having to remember to manually run a target like 'make
codegen' after every change, even for me working on this.
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..
Ah, this is leakage from the marshalling patches (not yet posted).
Removed from this commit.
> +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 :-)
Fixed. Especially now that we're committing the code, this is important.
> + else:
> + print " NULL, /* no methods */"
> + if iface.signals:
> + print " %s__signals, " % iface.c_name()
Same trailing whitespace is generated here.
Fixed.
> +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.
Done.
> --- /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.
Fixed.
> 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.
The function names are usually very similar to the DBus method names,
and I added comments for all those that were set to NULL.
The compiler will also be checking here once we have type safe handlers.
It'll often be a compiler error to assign the wrong handler to the wrong
vtable member (as long as the signatures don't match).
However, if you want we can use the C99 struct initialization by name
feature here. Are we fine with using that?
So it would look like:
struct mon_srv_iface monitor_methods = {
{ &mon_srv_iface_meta, 0 },
.getMonitorVersion = get_monitor_version,
.RegisterService = client_registration,
};
Then it would be realy compile safe, *and* readable.
The attached patches don't do this. Please confirm this before I go
ahead and do all the busy work of changing over :)
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?
Yes. I mainly removed it because the autogenerated constant names are
already longer, since they share their derivation with the struct names.
You end up with constants that look like this:
DATA_PROVIDER_IFACE_METHOD_REGISTERSERVICE
Are you sure you want that?
FWIW: 'DATA_PROVIDER_IFACE' comes from here:
<annotation value="data_provider_iface"
name="org.freedesktop.DBus.GLib.CSymbol"/>
and 'REGISTERSERVICE' comes from here:
<method name="RegisterService">
Anyway, I can add _METHOD_ back. But can you confirm before I go through
everywhere and make the change?
Alternatively if you'd rather not use these generated constants, I'm
fine with that too. It really is a minor aspect of this work.
Updated patches attached.
Cheers,
Stef