On 02/11/2015 08:23 PM, Jakub Hrozek wrote:
On Wed, Jan 21, 2015 at 02:08:54PM +0100, Pavel Březina wrote:
> 2/3 of previous sbus changes are acked so I'm sending few more patches from
> my tree.
>
> Commit messages should be sufficient to justify the changes. Simply put, I
> wanted to standardize and shorten names of generated and hard coded
> variables and constants so the code stays simple with longer interface
> names.
> From 610ffa5ee147f383ad2f876b6659bb9b92628808 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
> Date: Thu, 15 Jan 2015 14:23:19 +0100
> Subject: [PATCH 1/7] IFP: move interface definitions from ifpsrv.c into
> separate file
>
> Number of IFP interfaces will grown up rapidly in the future. It is
> not convenient to keep it inside ifpsrv.c.
One little nitpick
> +errno_t ifp_register_sbus_interface(struct sbus_connection *conn, void *pvt)
Can you s/pvt/handler_data/ to be consistent with the changes we did in
sbus?
Done.
> From f5efe4b4ac3903d9c31bffe14c305eb2a23d56cf Mon Sep 17
00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
> Date: Thu, 15 Jan 2015 14:38:39 +0100
> Subject: [PATCH 2/7] IFP: unify generated interfaces names
>
> Number of interfaces will grow. It is mandatory to unify names of
> generated structures and methods to simplify coding and debugging.
>
> The C name is created from D-Bus lowercased interface name using
> the following rewrite rules:
> org.freedesktop.sssd.infopipe -> iface_ifp
> . -> _
>
> Example:
> org.freedesktop.sssd.infopipe.Domains -> iface_ifp_domains
Again, nitpick:
> #include <dbus/dbus.h>
>
> #include "sbus/sssd_dbus.h"
> -#include "monitor/monitor_interfaces.h"
> -#include "confdb/confdb.h"
> -#include "responder/ifp/ifp_private.h"
> +#include "responder/ifp/ifp_iface_generated.h"
> #include "responder/ifp/ifp_domains.h"
> #include "responder/ifp/ifp_components.h"
> -#include "responder/common/responder_sbus.h"
The include changes seem unrelated, or are they? (except adding
responder/ifp/ifp_iface_generated.h maybe)
This belongs to the first patch since it removes headers that are no
longer needed when the code was moved. I moved it to the first patch.
> From e816d0414b49d43cfe58920793f9106c567ab6aa Mon Sep 17
00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
> Date: Fri, 16 Jan 2015 13:36:12 +0100
> Subject: [PATCH 4/7] IFP: simplify object path constant names
>
> The number of interfaces will grow and in order to keep names of
> object path constant consistent it means that also their names
> will grow. The new naming schema is:
>
> /org/freedesktop/sssd/infopipe -> IFP_PATH_
> / -> _
> everything uppercase
>
> Example:
> /org/freedesktop/sssd/infopipe/Components/Responders
> -> IFP_PATH_COMPONENTS_RESPONDERS
>
> If the path contains a _TREE suffix, it represents the whole
> subtree. For example:
> IFP_PATH_DOMAINS is /org/freedesktop/sssd/infopipe/Domains/*
Ack to the code, but didn't you mean to say IFP_PATH_DOMAINS_TREE in the
commit message? (I like the _TREE change btw)
You are correct sir.
New patches are attached, thank you for the review.