[PATCH] DATA_PROVIDER: BE_REQ as string in log message

[Petr Cech]

Hi,

I investigated the situation around the log message, which mentioned 
Lukas. I prepared this patch. The result is that the original message
> [sssd[be[cygnus.dev]]] [be_get_account_info] (0x0200): Got request

for [0x1001][1][name=celestian]
changed to
> [sssd[be[cygnus.dev]]] [be_get_account_info] (0x0200): Got request

for [0x1001][FAST BE_REQ_USER][1][name=celestian]

A)
I would like to ask if mark 'FAST' is useful, or if I should remove it.

B)
While writing a patch Lukas noticed another similar logging messages
> [sssd[pam]] [sss_dp_get_account_msg] (0x0400): Creating request for

[LDAP][3][1][name=mof_user6]

I investigated it. This is the same thing -- BE_REQ_*, but it is no 
longer in the provider, but in the responder. Can you please advise me 
where I could the function 'be_req2str' write?

The first message is coming from
> src/providers/data_provider_be.c --> be_get_account_info,

the second is from
> src/responder/common/responder_dp --> sss_dp_get_account_msg

Thanks.

Petr

[Pavel Reichl]

On 07/15/2015 08:53 AM, Petr Cech wrote:
> Hi,
>
> I investigated the situation around the log message, which mentioned 
> Lukas. I prepared this patch. The result is that the original message
> > [sssd[be[cygnus.dev]]] [be_get_account_info] (0x0200): Got request
>
> for [0x1001][1][name=celestian]
> changed to
> > [sssd[be[cygnus.dev]]] [be_get_account_info] (0x0200): Got request
>
> for [0x1001][FAST BE_REQ_USER][1][name=celestian]
>
> A)
> I would like to ask if mark 'FAST' is useful, or if I should remove it.

I think it can't hurt and might be useful, so IMO leave it.
> B)
> While writing a patch Lukas noticed another similar logging messages
> > [sssd[pam]] [sss_dp_get_account_msg] (0x0400): Creating request for
>
> [LDAP][3][1][name=mof_user6]
>
> I investigated it. This is the same thing -- BE_REQ_*, but it is no 
> longer in the provider, but in the responder. Can you please advise me 
> where I could the function 'be_req2str' write?

I think you should move it to separate file, as you don't want to share 
more code then necessary.
There are 2 possibilities where to have this file
a) in src/providers  - responders already link with some modules from 
this folder, so I suppose it's viable

SSSD_RESPONDER_OBJ = \
     ...
     src/providers/data_provider_iface_generated.c \
     src/providers/data_provider_iface_generated.h

b) move it to src/util which is a natural place for shared code

In my opinion a) is better because code is closely coupled with back end 
and is not general enough to be part of utils.

Sorry for not giving definitive answers. :-)
> +const char* be_req2str(dbus_uint32_t req_type)
> +{

// I was thinking whether you should use some
static char output[??];

//and fill it with "FAST" in a single condition and thus simplifying 
subsequent case

if (req_type & BE_REQ_FAST0)
    sprintf(output, "FAST");
> switch (req_type & BE_REQ_TYPE_MASK) {
> case BE_REQ_USER:
>    return req_type & BE_REQ_FAST ? "FAST BE_REQ_USER" : "BE_REQ_USER";
> case BE_REQ_GROUP:
>    return req_type & BE_REQ_FAST ? "FAST BE_REQ_GROUP" : "BE_REQ_GROUP";
> case BE_REQ_INITGROUPS:
>    return req_type & BE_REQ_FAST ? "FAST BE_REQ_INITGROUPS" : "BE_REQ_INITGROUPS";
> case BE_REQ_NETGROUP:
>    return req_type & BE_REQ_FAST ? "FAST BE_REQ_NETGROUP" : "BE_REQ_NETGROUP";
> case BE_REQ_SERVICES:
>    return req_type & BE_REQ_FAST ? "FAST BE_REQ_SERVICES" : "BE_REQ_SERVICES";
> case BE_REQ_SUDO_FULL:
>    return req_type & BE_REQ_FAST ? "FAST BE_REQ_SUDO_FULL" : "BE_REQ_SUDO_FULL";
> case BE_REQ_SUDO_RULES:
>    return req_type & BE_REQ_FAST ? "FAST BE_REQ_SUDO_RULES" : "BE_REQ_SUDO_RULES";
> case BE_REQ_AUTOFS:
>    return req_type & BE_REQ_FAST ? "FAST BE_REQ_AUTOFS" : "BE_REQ_AUTOFS";
> case BE_REQ_HOST:
>    return req_type & BE_REQ_FAST ? "FAST BE_REQ_HOST" : "BE_REQ_HOST";
> case BE_REQ_BY_SECID:
>    return req_type & BE_REQ_FAST ? "FAST BE_REQ_BY_SECID" : "BE_REQ_BY_SECID";
> case BE_REQ_USER_AND_GROUP:
>    return req_type & BE_REQ_FAST ? "FAST BE_REQ_USER_AND_GROUP" : "BE_REQ_USER_AND_GROUP";
> case BE_REQ_BY_UUID:
>    return req_type & BE_REQ_FAST ? "FAST BE_REQ_BY_UUID" : "BE_REQ_BY_UUID";
> case BE_REQ_BY_CERT:
>    return req_type & BE_REQ_FAST ? "FAST BE_REQ_BY_CERT" : "BE_REQ_BY_CERT";
> }
> return "UNKNOWN_REQ";
>
> +}
> The first message is coming from
> > src/providers/data_provider_be.c --> be_get_account_info,
>
> the second is from
> > src/responder/common/responder_dp --> sss_dp_get_account_msg
>
> Thanks.
>
> Petr
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Wed, Aug 05, 2015 at 10:03:30AM +0200, Pavel Reichl wrote:
> On 07/15/2015 08:53 AM, Petr Cech wrote:
> > Hi,
> >
> > I investigated the situation around the log message, which mentioned
> > Lukas. I prepared this patch. The result is that the original message
> > > [sssd[be[cygnus.dev]]] [be_get_account_info] (0x0200): Got request for
> >
> > [0x1001][1][name=celestian]
> > changed to
> > > [sssd[be[cygnus.dev]]] [be_get_account_info] (0x0200): Got request for
> >
> > [0x1001][FAST BE_REQ_USER][1][name=celestian]
> >
> > A)
> > I would like to ask if mark 'FAST' is useful, or if I should remove it.
>
> I think it can't hurt and might be useful, so IMO leave it.

Yes, IIRC FAST flag is always set for identity lookups and never set for
PAM responder lookups. I think it tells the responder whether to attempt
to reconnect if offline.

This might be useful data for the Internal document.
> > B)
> > While writing a patch Lukas noticed another similar logging messages
> > > [sssd[pam]] [sss_dp_get_account_msg] (0x0400): Creating request for
> >
> > [LDAP][3][1][name=mof_user6]
> >
> > I investigated it. This is the same thing -- BE_REQ_*, but it is no longer
> > in the provider, but in the responder. Can you please advise me where I
> > could the function 'be_req2str' write?
>
> I think you should move it to separate file, as you don't want to share more
> code then necessary.
> There are 2 possibilities where to have this file
> a) in src/providers  - responders already link with some modules from this
> folder, so I suppose it's viable

Yes, this is a bit of hierarchy violation, but in the end we need to
share the constants somehow. I think it's fine to keep the definite
version in the providers/dp_* hierarchy, because that's where the
interface is defined, the responder is a consumer.
> SSSD_RESPONDER_OBJ = \
>     ...
>     src/providers/data_provider_iface_generated.c \
>     src/providers/data_provider_iface_generated.h
>
> b) move it to src/util which is a natural place for shared code
>
> In my opinion a) is better because code is closely coupled with back end and
> is not general enough to be part of utils.

I agree. In future, especially if we document the interface and promote
3rd parties to write responders and providers, we might want to put the
interface into its own directory, but for now it's fine.
> Sorry for not giving definitive answers. :-)
> > +const char* be_req2str(dbus_uint32_t req_type)
> > +{
>
> // I was thinking whether you should use some
> static char output[??];
>
> //and fill it with "FAST" in a single condition and thus simplifying
> subsequent case
>
> if (req_type & BE_REQ_FAST0)
>    sprintf(output, "FAST");

I think this would be fine if we formatted the output, but we only
return const strings..
> > switch (req_type & BE_REQ_TYPE_MASK) {
> > case BE_REQ_USER:
> >    return req_type & BE_REQ_FAST ? "FAST BE_REQ_USER" : "BE_REQ_USER";
> > case BE_REQ_GROUP:
> >    return req_type & BE_REQ_FAST ? "FAST BE_REQ_GROUP" : "BE_REQ_GROUP";
> > case BE_REQ_INITGROUPS:
> >    return req_type & BE_REQ_FAST ? "FAST BE_REQ_INITGROUPS" : "BE_REQ_INITGROUPS";
> > case BE_REQ_NETGROUP:
> >    return req_type & BE_REQ_FAST ? "FAST BE_REQ_NETGROUP" : "BE_REQ_NETGROUP";
> > case BE_REQ_SERVICES:
> >    return req_type & BE_REQ_FAST ? "FAST BE_REQ_SERVICES" : "BE_REQ_SERVICES";
> > case BE_REQ_SUDO_FULL:
> >    return req_type & BE_REQ_FAST ? "FAST BE_REQ_SUDO_FULL" : "BE_REQ_SUDO_FULL";
> > case BE_REQ_SUDO_RULES:
> >    return req_type & BE_REQ_FAST ? "FAST BE_REQ_SUDO_RULES" : "BE_REQ_SUDO_RULES";
> > case BE_REQ_AUTOFS:
> >    return req_type & BE_REQ_FAST ? "FAST BE_REQ_AUTOFS" : "BE_REQ_AUTOFS";
> > case BE_REQ_HOST:
> >    return req_type & BE_REQ_FAST ? "FAST BE_REQ_HOST" : "BE_REQ_HOST";
> > case BE_REQ_BY_SECID:
> >    return req_type & BE_REQ_FAST ? "FAST BE_REQ_BY_SECID" : "BE_REQ_BY_SECID";
> > case BE_REQ_USER_AND_GROUP:
> >    return req_type & BE_REQ_FAST ? "FAST BE_REQ_USER_AND_GROUP" : "BE_REQ_USER_AND_GROUP";
> > case BE_REQ_BY_UUID:
> >    return req_type & BE_REQ_FAST ? "FAST BE_REQ_BY_UUID" : "BE_REQ_BY_UUID";
> > case BE_REQ_BY_CERT:
> >    return req_type & BE_REQ_FAST ? "FAST BE_REQ_BY_CERT" : "BE_REQ_BY_CERT";
> > }
> > return "UNKNOWN_REQ";
> >
> > +}
> > The first message is coming from
> > > src/providers/data_provider_be.c --> be_get_account_info,
> >
> > the second is from
> > > src/responder/common/responder_dp --> sss_dp_get_account_msg
> >
> > Thanks.
> >
> > Petr
> >
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Petr Cech]

On 08/05/2015 11:23 AM, Jakub Hrozek wrote:
> > > B)
> > > > > While writing a patch Lukas noticed another similar logging messages
> > > > > > >[sssd[pam]] [sss_dp_get_account_msg] (0x0400): Creating request for
> > > > >
> > > > > [LDAP][3][1][name=mof_user6]
> > > > >
> > > > > I investigated it. This is the same thing -- BE_REQ_*, but it is no longer
> > > > > in the provider, but in the responder. Can you please advise me where I
> > > > > could the function 'be_req2str' write?
> > >
> > > I think you should move it to separate file, as you don't want to share more
> > > code then necessary.
> > > There are 2 possibilities where to have this file
> > > a) in src/providers  - responders already link with some modules from this
> > > folder, so I suppose it's viable
>
> Yes, this is a bit of hierarchy violation, but in the end we need to
> share the constants somehow. I think it's fine to keep the definite
> version in the providers/dp_* hierarchy, because that's where the
> interface is defined, the responder is a consumer.

There is new patch attached.

I think, that constants and const2str() functions should be in one 
place. I tried to suggest how we might share our constants. That's why I 
created a new header file in which we could move all the constants in 
the future.

I am open to discussion. I look forward to your views.

Petr

[Pavel Reichl]

On 08/18/2015 01:19 PM, Petr Cech wrote:
> On 08/05/2015 11:23 AM, Jakub Hrozek wrote:
> > > > B)
> > > > > > While writing a patch Lukas noticed another similar logging
> > > >
> > > > messages
> > > > > > >>[sssd[pam]] [sss_dp_get_account_msg] (0x0400): Creating
> > > > >
> > > > > request for
> > > > > > [LDAP][3][1][name=mof_user6]
> > > > > >
> > > > > > I investigated it. This is the same thing -- BE_REQ_*, but it is
> > > >
> > > > no longer
> > > > > > in the provider, but in the responder. Can you please advise me
> > > >
> > > > where I
> > > > > > could the function 'be_req2str' write?
> > > >
> > > > I think you should move it to separate file, as you don't want to
> > >
> > > share more
> > > > code then necessary.
> > > > There are 2 possibilities where to have this file
> > > > a) in src/providers  - responders already link with some modules
> > >
> > > from this
> > > > folder, so I suppose it's viable
> >
> > Yes, this is a bit of hierarchy violation, but in the end we need to
> > share the constants somehow. I think it's fine to keep the definite
> > version in the providers/dp_* hierarchy, because that's where the
> > interface is defined, the responder is a consumer.
>
> There is new patch attached.
>
> I think, that constants and const2str() functions should be in one 
> place. I tried to suggest how we might share our constants. That's why 
> I created a new header file in which we could move all the constants 
> in the future.
>
> I am open to discussion. I look forward to your views.
>
> Petr

Hello Petr,

make dict check fails:
> /workspace/ci/label/rhel7/ci-build-debug/sssd-1.13.1/_inst/share/locale" -g3 -O2 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -c ../src/providers/ldap/ldap_id.c  -fPIC -DPIC -o src/providers/ldap/.libs/libsss_ldap_common_la-ldap_id.o
> ../src/providers/ldap/ldap_id.c:33:41: fatal error: providers/data_provider_req.h: No such file or directory
>   #include "providers/data_provider_req.h"

you can fix this by something like:
> Makefile.am
> @@ -584,6 +584,7 @@ dist_noinst_HEADERS = \
>      src/confdb/confdb_private.h \
>      src/confdb/confdb_setup.h \
>      src/providers/data_provider.h \
> src/providers/data_provider_req.h \

I think that data_provider_req.h should be included in data_provider.h, 
you could save a lot of changes in source files that require both of them.

[Petr Cech]

On 08/21/2015 01:08 PM, Pavel Reichl wrote:
> Hello Petr,
>
> make dict check fails:
> > /workspace/ci/label/rhel7/ci-build-debug/sssd-1.13.1/_inst/share/locale" 
> > -g3 -O2 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE 
> > -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE 
> > -D_LARGEFILE64_SOURCE -c ../src/providers/ldap/ldap_id.c  -fPIC -DPIC 
> > -o src/providers/ldap/.libs/libsss_ldap_common_la-ldap_id.o
> > ../src/providers/ldap/ldap_id.c:33:41: fatal error: 
> > providers/data_provider_req.h: No such file or directory
> >   #include "providers/data_provider_req.h"
>
> you can fix this by something like:
> > Makefile.am
> > @@ -584,6 +584,7 @@ dist_noinst_HEADERS = \
> >      src/confdb/confdb_private.h \
> >      src/confdb/confdb_setup.h \
> >      src/providers/data_provider.h \
> > src/providers/data_provider_req.h \
>
> I think that data_provider_req.h should be included in 
> data_provider.h, you could save a lot of changes in source files that 
> require both of them.

Thanks.
There is repaired patch attached.
Petr

[Pavel Reichl]

On 08/21/2015 03:40 PM, Petr Cech wrote:
> On 08/21/2015 01:08 PM, Pavel Reichl wrote:
> > Hello Petr,
> >
> > make dict check fails:
> > > /workspace/ci/label/rhel7/ci-build-debug/sssd-1.13.1/_inst/share/locale" 
> > > -g3 -O2 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE 
> > > -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE 
> > > -D_LARGEFILE64_SOURCE -c ../src/providers/ldap/ldap_id.c  -fPIC 
> > > -DPIC -o src/providers/ldap/.libs/libsss_ldap_common_la-ldap_id.o
> > > ../src/providers/ldap/ldap_id.c:33:41: fatal error: 
> > > providers/data_provider_req.h: No such file or directory
> > >   #include "providers/data_provider_req.h"
> >
> > you can fix this by something like:
> > > Makefile.am
> > > @@ -584,6 +584,7 @@ dist_noinst_HEADERS = \
> > >      src/confdb/confdb_private.h \
> > >      src/confdb/confdb_setup.h \
> > >      src/providers/data_provider.h \
> > > src/providers/data_provider_req.h \
> >
> > I think that data_provider_req.h should be included in 
> > data_provider.h, you could save a lot of changes in source files that 
> > require both of them.
>
> Thanks.
> There is repaired patch attached.
> Petr

Petr can you change data_provider_req.c to include

"providers/data_provider_req.h"

instead of

"providers/data_provider.h" ?

I originally thought that you will be able to include solely 
data_provider_req.h from responder_dp.c but I see that data_provider.h 
is required. But I still don't mind introducing data_provider_req.h.

If you change this little nitpick I think I can ACK the patch.

[Petr Cech]

On 08/21/2015 05:10 PM, Pavel Reichl wrote:
> Petr can you change data_provider_req.c to include
>
> "providers/data_provider_req.h"
>
> instead of
>
> "providers/data_provider.h" ?
>
> I originally thought that you will be able to include solely 
> data_provider_req.h from responder_dp.c but I see that data_provider.h 
> is required. But I still don't mind introducing data_provider_req.h.
>
> If you change this little nitpick I think I can ACK the patch.

Pavel,
you're right, that's mine main opinion to this issue. I am sorry, I need 
more focus and coffe.
There is fixed (not repaired) patch.
Petr

[Pavel Reichl]

On 08/21/2015 05:49 PM, Petr Cech wrote:
> On 08/21/2015 05:10 PM, Pavel Reichl wrote:
> > Petr can you change data_provider_req.c to include
> >
> > "providers/data_provider_req.h"
> >
> > instead of
> >
> > "providers/data_provider.h" ?
> >
> > I originally thought that you will be able to include solely 
> > data_provider_req.h from responder_dp.c but I see that 
> > data_provider.h is required. But I still don't mind introducing 
> > data_provider_req.h.
> >
> > If you change this little nitpick I think I can ACK the patch.
>
> Pavel,
> you're right, that's mine main opinion to this issue. I am sorry, I 
> need more focus and coffe.
> There is fixed (not repaired) patch.
> Petr
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

ACK

[Pavel Reichl]

[snip]
> > On 08/21/2015 05:10 PM, Pavel Reichl wrote:
>
> ACK

Just adding link to passed ci (with exception of test 
test_ldap_id_cleanup which is false positive (and already fixed and 
acked on list)):
http://sssd-ci.duckdns.org/logs/job/23/73/summary.html

[Lukas Slebodnik]

On (21/08/15 17:49), Petr Cech wrote:
> On 08/21/2015 05:10 PM, Pavel Reichl wrote:
> > Petr can you change data_provider_req.c to include
> >
> > "providers/data_provider_req.h"
> >
> > instead of
> >
> > "providers/data_provider.h" ?
> >
> > I originally thought that you will be able to include solely
> > data_provider_req.h from responder_dp.c but I see that data_provider.h is
> > required. But I still don't mind introducing data_provider_req.h.
> >
> > If you change this little nitpick I think I can ACK the patch.
>
> Pavel,
> you're right, that's mine main opinion to this issue. I am sorry, I need more
> focus and coffe.
> There is fixed (not repaired) patch.
> Petr
> From aebda5def026d7a0fc40c4034ef18ba97ada5f36 Mon Sep 17 00:00:00 2001
> From: Petr Cech 
> Date: Tue, 18 Aug 2015 06:59:31 -0400
> Subject: [PATCH] DATA_PROVIDER: BE_REQ as string in log message
>
> Add be_req2str() for translation BE_REQ to string.
> So we will have
> || Got request for [0x1001][FAST BE_REQ_USER][1][name=celestian]
> instead of
> || Got request for [0x1001][1][name=celestian]
>
> Function be_req2str() is used in data provider and in responder too.
> So this patch create new header file data_provider_req.h which
> delivers function be_req2str() and definitions of BE_REQ_*.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2708
>
> Makefile.am                         |  6 +++-
> src/providers/data_provider.h       | 17 +---------
> src/providers/data_provider_be.c    |  3 +-
> src/providers/data_provider_req.c   | 68 +++++++++++++++++++++++++++++++++++++
> src/providers/data_provider_req.h   | 51 ++++++++++++++++++++++++++++
> src/responder/common/responder_dp.c |  4 +--
> 6 files changed, 129 insertions(+), 20 deletions(-)
> create mode 100644 src/providers/data_provider_req.c
> create mode 100644 src/providers/data_provider_req.h
>
> diff --git a/Makefile.am b/Makefile.am
> index f153ab0adf390880672a1681b386ea26426465cb..94920b29d7aab44085e401f8ada8555ab69fed6a 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -446,7 +446,8 @@ SSSD_RESPONDER_OBJ = \
>     src/monitor/monitor_iface_generated.c \
>     src/monitor/monitor_iface_generated.h \
>     src/providers/data_provider_iface_generated.c \
> src/providers/data_provider_iface_generated.h
> src/providers/data_provider_iface_generated.h \
> src/providers/data_provider_req.c
>
> SSSD_TOOLS_OBJ = \
>     src/tools/sss_sync_ops.c \
> @@ -583,6 +584,7 @@ dist_noinst_HEADERS = \
>     src/confdb/confdb_private.h \
>     src/confdb/confdb_setup.h \
>     src/providers/data_provider.h \
> src/providers/data_provider_req.h \
> src/providers/dp_backend.h \
> src/providers/dp_dyndns.h \
> src/providers/dp_ptask_private.h \
>
> @@ -1193,6 +1195,7 @@ endif
>
> sssd_be_SOURCES = \
>     src/providers/data_provider_be.c \
> src/providers/data_provider_req.c \
> src/providers/data_provider_fo.c \
> src/providers/data_provider_opts.c \
> src/providers/data_provider_callbacks.c \
>
> @@ -1646,6 +1649,7 @@ simple_access_tests_SOURCES = \
>     src/providers/simple/simple_access.c \
>     src/providers/simple/simple_access_check.c \
>     src/providers/data_provider_be.c \
> src/providers/data_provider_req.c \
> src/providers/data_provider_fo.c \
> src/providers/data_provider_opts.c \
> src/providers/data_provider_callbacks.c \
>
> diff --git a/src/providers/data_provider.h b/src/providers/data_provider.h
> index 510c63ce41c99314ec8fcf11fffb2e66082e8951..39051b90c3aad96f62dcbb86a20bcfd8c954879b 100644
> --- a/src/providers/data_provider.h
> +++ b/src/providers/data_provider.h
> @@ -43,6 +43,7 @@
> #include "sbus/sbus_client.h"
> #include "sss_client/sss_cli.h"
> #include "util/authtok.h"
> +#include "providers/data_provider_req.h"
> #include "providers/data_provider_iface_generated.h"
>
> #define DATA_PROVIDER_VERSION 0x0001
> @@ -131,22 +132,6 @@
> #define BE_FILTER_CERT 6
> #define BE_FILTER_WILDCARD 7
>
> -#define BE_REQ_USER          0x0001
> -#define BE_REQ_GROUP         0x0002
> -#define BE_REQ_INITGROUPS    0x0003
> -#define BE_REQ_NETGROUP      0x0004
> -#define BE_REQ_SERVICES      0x0005
> -#define BE_REQ_SUDO_FULL     0x0006
> -#define BE_REQ_SUDO_RULES    0x0007
> -#define BE_REQ_AUTOFS        0x0009
> -#define BE_REQ_HOST          0x0010
> -#define BE_REQ_BY_SECID      0x0011
> -#define BE_REQ_USER_AND_GROUP 0x0012
> -#define BE_REQ_BY_UUID       0x0013
> -#define BE_REQ_BY_CERT       0x0014
> -#define BE_REQ_TYPE_MASK     0x00FF
> -#define BE_REQ_FAST          0x1000
>
> #define DP_SEC_ID "secid"
> #define DP_CERT "cert"
> /* sizeof() counts the trailing \0 so we must substract 1 for the string
> diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c
> index d147630248f0a24f5a632760b55b9284a6928e40..d71a69cb8e2997975828236998ec0b0e3f353f07 100644
> --- a/src/providers/data_provider_be.c
> +++ b/src/providers/data_provider_be.c
> @@ -1104,7 +1104,8 @@ static int be_get_account_info(struct sbus_request *dbus_req, void *user_data)
>         return EOK; /* handled */
> DEBUG(SSSDBG_FUNC_DATA,
>      "Got request for [%#x][%d][%s]\n", type, attr_type, filter);
>      "Got request for [%#x][%s][%d][%s]\n", type, be_req2str(type),
>      attr_type, filter);
>
> /* If we are offline and fast reply was requested
> return offline immediately
>
> diff --git a/src/providers/data_provider_req.c b/src/providers/data_provider_req.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..2a7e4fed2f19f75bd2504b915e6a8c75a1482614
> --- /dev/null
> +++ b/src/providers/data_provider_req.c
> @@ -0,0 +1,68 @@
> +/*
> SSSD
> Data Provider -- backend request
> Copyright (C) Petr Cech  2015
> 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/.
>
> +*/
>
> +#include "providers/data_provider_req.h"
>
> +const char *be_req2str(dbus_uint32_t req_type)
> +{
> switch (req_type & BE_REQ_TYPE_MASK) {
> case BE_REQ_USER:
>    return req_type & BE_REQ_FAST ?
>           "FAST BE_REQ_USER" : "BE_REQ_USER";

Petr,
what do you think about small simplification.

#include "providers/data_provider_req.h"

+#define be_req_to_str(req_type, be_req_t) \
+        ((req_type) & BE_REQ_FAST) \
+        ? "FAST " #be_req_t \
+        : #be_req_t
+
 const char *be_req2str(dbus_uint32_t req_type)
 {
     switch (req_type & BE_REQ_TYPE_MASK) {
     case BE_REQ_USER:
-        return req_type & BE_REQ_FAST ?
-               "FAST BE_REQ_USER" : "BE_REQ_USER";
+        return be_req_to_str(req_type, BE_REQ_USER);
     case BE_REQ_GROUP:
         return req_type & BE_REQ_FAST ?
                "FAST BE_REQ_GROUP" : "BE_REQ_GROUP";

The result of preprocessor is almost the same
and code look little bit simpler.

const char *be_req2str(dbus_uint32_t req_type)
{
    switch (req_type & 0x00FF) {
    case 0x0001:
        return ((req_type) & 0x1000) ? "FAST " "BE_REQ_USER" : "BE_REQ_USER";
    case 0x0002:
        return req_type & 0x1000 ?
               "FAST BE_REQ_GROUP" : "BE_REQ_GROUP";
    case 0x0003:
        return req_type & 0x1000 ?
               "FAST BE_REQ_INITGROUPS" : "BE_REQ_INITGROUPS";

Feel free to rename macro; it's just a POC version.

LS

[Petr Cech]

On 08/27/2015 01:26 PM, Lukas Slebodnik wrote:
> Petr,
> what do you think about small simplification.
>
> #include "providers/data_provider_req.h"
>
> +#define be_req_to_str(req_type, be_req_t) \
>    ((req_type) & BE_REQ_FAST) \
>    ? "FAST " #be_req_t \
>    : #be_req_t
> const char *be_req2str(dbus_uint32_t req_type)
> {
>     switch (req_type & BE_REQ_TYPE_MASK) {
>     case BE_REQ_USER:
>    return req_type & BE_REQ_FAST ?
>           "FAST BE_REQ_USER" : "BE_REQ_USER";
>    return be_req_to_str(req_type, BE_REQ_USER);
> case BE_REQ_GROUP:
>     return req_type & BE_REQ_FAST ?
>            "FAST BE_REQ_GROUP" : "BE_REQ_GROUP";
>
> The result of preprocessor is almost the same
> and code look little bit simpler.
>
> const char *be_req2str(dbus_uint32_t req_type)
> {
>      switch (req_type & 0x00FF) {
>      case 0x0001:
>          return ((req_type) & 0x1000) ? "FAST " "BE_REQ_USER" : "BE_REQ_USER";
>      case 0x0002:
>          return req_type & 0x1000 ?
>                 "FAST BE_REQ_GROUP" : "BE_REQ_GROUP";
>      case 0x0003:
>          return req_type & 0x1000 ?
>                 "FAST BE_REQ_INITGROUPS" : "BE_REQ_INITGROUPS";
>
> Feel free to rename macro; it's just a POC version.
>
> LS

Thanks.

There is improved patch attached.

Petr

[Pavel Reichl]

On 08/28/2015 04:31 PM, Petr Cech wrote:
>      "Got request for [%#x][%s][%d][%s]\n", type, be_req2str(type),
>      attr_type, filter);

Petr do you think it could be useful to print attr_type as a string?

[Petr Cech]

On 09/04/2015 04:32 PM, Pavel Reichl wrote:
> On 08/28/2015 04:31 PM, Petr Cech wrote:
> >      "Got request for [%#x][%s][%d][%s]\n", type, be_req2str(type),
> >      attr_type, filter);
>
> Petr do you think it could be useful to print attr_type as a string?

We talked about it offline. It seems to be only value type, nothing 
important.

But there is a new rebased patch, because development you cannot stop.

Petr
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Pavel Reichl]

On 09/08/2015 04:28 PM, Petr Cech wrote:
> On 09/04/2015 04:32 PM, Pavel Reichl wrote:
> > On 08/28/2015 04:31 PM, Petr Cech wrote:
> > >      "Got request for [%#x][%s][%d][%s]\n", type, be_req2str(type),
> > >      attr_type, filter);
> >
> > Petr do you think it could be useful to print attr_type as a string?
>
> We talked about it offline. It seems to be only value type, nothing important.

OK, thanks for looking into it,
> But there is a new rebased patch, because development you cannot stop.

:-)

ci passed: http://sssd-ci.duckdns.org/logs/job/25/62/summary.html

ACK

[Jakub Hrozek]

On Tue, Sep 08, 2015 at 05:53:27PM +0200, Pavel Reichl wrote:
> On 09/08/2015 04:28 PM, Petr Cech wrote:
> > On 09/04/2015 04:32 PM, Pavel Reichl wrote:
> > > On 08/28/2015 04:31 PM, Petr Cech wrote:
> > > >      "Got request for [%#x][%s][%d][%s]\n", type, be_req2str(type),
> > > >      attr_type, filter);
> > >
> > > Petr do you think it could be useful to print attr_type as a string?
> >
> > We talked about it offline. It seems to be only value type, nothing important.
>
> OK, thanks for looking into it,
> > But there is a new rebased patch, because development you cannot stop.
>
> :-)
>
> ci passed: http://sssd-ci.duckdns.org/logs/job/25/62/summary.html
>
> ACK

Sorry for the delay in pushing. Pushed to master now as
376eaf187c13c2a1eaea0ffbdd970b6b563ab74c