[RFC][Patch] DBus Service C library

[Havoc Pennington]

Hi,

Shahar Frank wrote:
 > So, I decided to
> implement simple and more "traditional" C oriented Server library. The
> library is not yet finished but I decided to contribute it hoping that I
> can get some valuable feedback, advices, comments, idea, etc. 

It's great to see work in this area. Did you see:
http://lists.freedesktop.org/archives/dbus/2007-July/008159.html

I think there are some ideas there you could incorporate.

The code there is a slightly more elaborated version of
http://svn.mugshot.org/local-export-daemon/trunk/src/hippo-dbus-helper.h

example usage is:
http://svn.mugshot.org/local-export-daemon/trunk/src/session-api.c

> I made an effort to do things in the "dbus way", but as I learned dbus
> low level only while writing this library, I missed many mechanisms and
> I have made quiet a few implementation mistakes. For example I
> implemented a new logging system, didn't use dbus portable function
> wrappers, and didn't use dbus internal abstract data structures, such as
> dbus link lists. I really want to have your opinions on these subjects
> (or any other). 

If you use the internal dbus utilities, we essentially have to make this 
a new feature of libdbus (in the same shared object). That is IMO 
probably the right thing to do.

During a testing/experimentation phase we could keep it separate, though 
it will mean some weird build system setup.

I think there's a lot of work left here to get a stable API.

> If you feel it may interest others, I will happy to move the project to
> your repository and work on it from there.

I'm not sure yet we'd definitely want it in the main dbus package, 
though it seems possible. The first step here would be to get an account 
on freedesktop.org in the dbus group, and then you could at least set up 
your own git repository if you want, and put things in CVS later if we 
decide that makes sense. To get an account you have to file a bug on 
bugs.freedesktop.org, the wiki probably has instructions on there somewhere.

Some code comments, probably premature:

> +struct dbus_type {
> +	char c;
> +	int size;
> +	int basic;
> +} dbus_types[] = {

There should be functions type_get_alignment() type_is_basic() etc. in 
the dbus code already, avoiding this array.

> +static char *log_level_str[] = {

As you say, should use the stuff in libdbus already for logging.

> +		free(service->err);
> +	service->err = strdup(msg);

You'd want to use dbus_free, _dbus_strdup, etc.

> +	for (ee = pp + DBUS_PARAM_MAX; pp < ee && *pp; pp++)
> +		if (!strcmp(name, (*pp)->name))
> +			return *pp;

libdbus code would use 2-space indent and strcmp == 0 rather than !strcmp

> +static int dbus_type_size(char const *sig)

the libdbus style would be

return_type
function_name()

on separate lines. Also, a static or internal function would start with 
_, like _dbus_type_get_size()

I think there's a get_alignment() already that pretty much does this.

> +	DBusParam *p = dbus_method_lookup_param(method, in, name);

libdbus coding style would not have a function call and variable decl on 
the same line.

> +	int sz;

libdbus style would not use abbreviations.

> +		memcpy(value, p->data, sz);

memcpy, strcpy, etc. are not allowed in libdbus style - DBusString is 
always used, see HACKING file.

> +	if (!(param = calloc(1, sizeof (*param))))

dbus_new0 is the calloc equivalent

> +	param->name = strdup(name);
> +	param->signature = strdup(signature);

Note here out-of-memory must be checked. (and use _dbus_strdup)

> +	if (!param || !param->name)
> +		return "NULL/unassigned param";

generally would use a DBusError rather than returning an error string

> +	if (in)		// strncat may write size + 1 char !!!
> +		strncat(method->insig, param->signature,
> sizeof(method->insig) -1);

DBusString rather than strncat


I'll stop with the style comments since you get the idea, and look at 
the API a bit:

> +   This Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License

(LGPL won't work btw, we were going to just do MIT/X11 for the dbus 
package in the future, but LGPL is incompatible with the current 
licensing if this is in the same package, I believe)

> +#ifndef _DBUS_SERVER_H
> +#define _DBUS_SERVER_H

Note there is already a DBusServer object and dbus-server.h

If you keep this a separate library, it should have a distinct namespace 
other than "dbus_"

If we fold it into libdbus, possibly it should have another namespace 
anyway, though I'm not sure I can think of one it might clarify things 
for developers to know that part of the API is "fundamental/low-level" 
and part is just convenience overlay / plain C binding.

> +#include <unistd.h>	// DBUS_PANIC.exit

System headers shouldn't be in library headers, since you can never 
remove them once added and apps may rely on them, and they can cause 
weird side effects for apps.

> +
> +enum {
> +	DBUS_SERVICE_LOG_NONE,
> +	DBUS_SERVICE_LOG_FATAL,
> +	DBUS_SERVICE_LOG_ERROR,
> +	DBUS_SERVICE_LOG_WARN,
> +	DBUS_SERVICE_LOG_INFO,
> +	DBUS_SERVICE_LOG_DEBUG,
> +	DBUS_SERVICE_LOG_VERBOSE,
> +
> +	DBUS_SERVICE_LOG_LAST_
> +};

The log facility should not be in public API.

> +typedef struct DBusService DBusService;

Most likely this should not be called DBusService. A service is a 
process that can be launched from a .service file. The library should 
probably not deal with that specifically much, instead usually we are 
interested in bus names. (Services can be launched by name, but not all 
names are owned by services.)

> +typedef char *(dbus_method_handler)(DBusMethod *method, DBusMessage
> *msg, void *user);

This would be DBusMethodHandler in the current API's style.

> +#define DBUS_PARAM_MAX 5
> +#define DBUS_SIG_STR_MAX 32

It would be better to avoid these maxes, I would say just have people 
spec the params as a single signature string. But otherwise, have a 
"const DBusParam*" in the method struct and require declaring the param 
array separately.

> +
> +enum {
> +	DBUS_METHOD_NORMAL = 0,
> +	DBUS_METHOD_SIGNAL = 1 << 0,
> +	DBUS_METHOD_SIGNAL_RECIEVER = 1 << 1,
> +};

I would not consider all of these methods, maybe
  DBUS_OBJECT_MEMBER_METHOD,
  DBUS_OBJECT_MEMBER_SIGNAL

A signal receiver to me is a different kind of thing, I can't figure out 
how to get it in the same enum.

The enum needs a typedef, like typedef enum {} DBusObjectMember;

> +struct DBusMethod {
> +	char *name;			// method member name

const char*

> +	DBusParam *inparam[DBUS_PARAM_MAX];	// in arguments
> +	DBusParam *outparam[DBUS_PARAM_MAX];	// return paramenters
> +
> +	char insig[DBUS_SIG_STR_MAX];	// method arguments signature
> +	char outsig[DBUS_SIG_STR_MAX];	// method return signature

These can just be const char*

> +	unsigned flags;			// type flags
> +	dbus_method_handler *handler;	// user handler function
> +	DBusInterface *interface;	// back pointer
> +	DBusSignal *signal;		// back pointer for signal
> methods
> +	void *user;			// user pointer
> +	struct DBusMethod *next;	// used by interface's methods
> list
> +};

I think you are stuffing the API and interesting-to-user-of-library 
stuff and internal-to-library stuff into one struct.

I would suggest having one struct that is used by the library user to 
declare their stuff in a static array, and a separate (private, not in 
the header) struct that is used for library-internal data structures.

I'm not sure you need most of these Signal/Interface/Param/etc. types in 
the public API - maybe look at how I did it in the stuff I linked to at 
the start of the mail.

> +char *dbus_service_emit_signal(DBusService *service, DBusMethod
> *emitter);

Signal emission in dbus is conceptually from an object, rather than from 
a bus name.

> +char *dbus_service_connect(DBusService * service, int bus_session);

enum typedef should be used, like DBusBusType, rather than int

> +static inline char *dbus_method_arg(DBusMethod *method, char *name,
> void *value)
> +{
> +	return dbus_method_get_param(method, name, 1, value);
> +}

inline isn't portable (unless we "#define inline" so it vanishes on some 
platforms), but I think the larger issue here is that an inline or macro 
ruins the ability to change the implementation in the future without 
rebuilding all apps. So I'd just make these normal library functions.

> +	if (method->flags & DBUS_METHOD_SIGNAL_RECIEVER)

I before E except after C ;-)

Anyway, this is a promising and high-value problem to tackle, nice work.

Havoc