[SSSD] [PATCHES] sbus: support multiple interfaces on single object path

Jakub Hrozek jhrozek at redhat.com
Mon Jan 19 21:33:24 UTC 2015


On Fri, Jan 16, 2015 at 02:25:39PM +0100, Pavel Březina wrote:
> On 01/16/2015 02:06 PM, Jakub Hrozek wrote:
> >>+#define CHECK_SIGNATURE(req, error, label, exp) do { \
> >>+    const char *__sig; \
> >>+    __sig = dbus_message_get_signature(req->message); \
> >>+    if (strcmp(__sig, exp) != 0) { \
> >>+        error = sbus_error_new(req, DBUS_ERROR_INVALID_ARGS, \
> >>+               "Invalid arguments: expected \"%s\", got \"%s\"", exp, __sig); \
> >>+        goto label; \
> >
> >Another flow-changing macro...I'm fine with wrappering this in a
> >function, but please don't jump from macros..
> 
> It's funny that you don't like it, since I learned it from you :-)

Damn it, past Jakub :-)

In general, macros that change the flow are really hard to debug and
what's even worse is that it might not be obvious that the macro can
also jump.

> 
> The whole point of those macros is to move error checking out of the
> function. If you put the code inside a function you still have to test the
> result in the caller so it doesn't make any sense.
> 
> I can get rid of CHECK_SIGNATURE since it is used on three places only
> anyway. But WRITE_DATA makes the code more readable. As you said yourself,
> fprintf is not really expected to fail. This way it is possible to focus on
> the flow of the generator itself instead of seeing four lines that are not
> supposed to be executed ever after each fprintf.
> 
> This is IMHO much better:
> WRITE_DATA(file, ret, done, FMT_METHOD_NOARG, method->name);
> WRITE_DATA(file, ret, done, FMT_METHOD, method->name);
> WRITE_DATA(file, ret, done, FMT_METHOD_CLOSE);

OK, in this single occurence, let's allow the jump in the macro, but
please rename the macro so that it's clear that it can change the flow
of the program. Maybe something like "WRITE_OR_FAIL".



More information about the sssd-devel mailing list