On (10/03/16 11:54), Jakub Hrozek wrote:
Hi,
attached are patches that in my opinion make libipa_hbac easier to use
by external projects like pam_hbac.
[PATCH 1/4] libipa_hbac: Do not use C99
- even though we use C99 in the deamon, pam_hbac needs to run on very
exotic platforms. I don't think the code readability is decreased too
much
[PATCH 2/4] libipa_hbac: Add more debug messages
- just adds debug messages
[PATCH 3/4] libipa_hbac: Fix typo in constant name
- fixes a typo that breaks compilation on non-GNU systems
[PATCH 4/4] libipa_hbac: Move the library to src/lib/ipa_hbac
- This might be a bit controversial, but in my opinion a library
shouldn't be present in the provider subdirectory. Please also note
I added "I$(top_srcdir)/src/util", this is just so that I can copy the
libipa_hbac code without any changes to pam_hbac (even changes to
header file locations..)
From 3ae3c1fba3e14d3299b121c03c166489fba55741 Mon Sep 17 00:00:00
2001
From: Jakub Hrozek <jhrozek(a)redhat.com>
Date: Mon, 22 Feb 2016 09:44:19 +0100
Subject: [PATCH 1/4] libipa_hbac: Do not use C99
libipa_hbac can be used by external consumers like pam_hbac who run on
old platforms that do not support C99. Refrain from using C99 features
in that codebase.
---
src/providers/ipa/hbac_evaluator.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/providers/ipa/hbac_evaluator.c b/src/providers/ipa/hbac_evaluator.c
index f4b76d8e6ca3b63b092f65cdb4aac9e98e8c717e..36376abb4eb834681cf62030c2bc1ba11e1369ce
100644
--- a/src/providers/ipa/hbac_evaluator.c
+++ b/src/providers/ipa/hbac_evaluator.c
@@ -146,6 +146,8 @@ enum hbac_eval_result hbac_evaluate(struct hbac_rule **rules,
struct hbac_eval_req *hbac_req,
struct hbac_info **info)
{
+ uint32_t i;
+
enum hbac_error_code ret;
enum hbac_eval_result result = HBAC_EVAL_DENY;
enum hbac_eval_result_int intermediate_result;
@@ -163,7 +165,7 @@ enum hbac_eval_result hbac_evaluate(struct hbac_rule **rules,
(*info)->rule_name = NULL;
}
- for (uint32_t i = 0; rules[i]; i++) {
+ for (i = 0; rules[i]; i++) {
hbac_rule_debug_print(rules[i]);
intermediate_result = hbac_evaluate_rule(rules[i], hbac_req, &ret);
if (intermediate_result == HBAC_EVAL_UNMATCHED) {
@@ -381,6 +383,8 @@ const char *hbac_error_string(enum hbac_error_code code)
static void hbac_request_element_debug_print(struct hbac_request_element *el,
const char *label)
{
+ int i;
+
if (el) {
if (el->name) {
HBAC_DEBUG(HBAC_DBG_TRACE, "\t\t%s [%s]\n", label, el->name);
@@ -389,7 +393,7 @@ static void hbac_request_element_debug_print(struct
hbac_request_element *el,
if (el->groups) {
if (el->groups[0]) {
HBAC_DEBUG(HBAC_DBG_TRACE, "\t\t%s_group:\n", label);
- for (int i = 0; el->groups[i]; i++) {
+ for (i = 0; el->groups[i]; i++) {
HBAC_DEBUG(HBAC_DBG_TRACE, "\t\t\t[%s]\n",
el->groups[i]);
}
} else {
@@ -434,6 +438,8 @@ static void hbac_req_debug_print(struct hbac_eval_req *req)
static void hbac_rule_element_debug_print(struct hbac_rule_element *el,
const char *label)
{
+ int i;
+
if (el) {
HBAC_DEBUG(HBAC_DBG_TRACE, "\t\tcategory [%#x] [%s]\n",
el->category,
(el->category == HBAC_CATEGORY_ALL) ? "ALL" :
"NONE");
@@ -441,7 +447,7 @@ static void hbac_rule_element_debug_print(struct hbac_rule_element
*el,
if (el->names) {
if (el->names[0]) {
HBAC_DEBUG(HBAC_DBG_TRACE, "\t\t%s_names:\n", label);
- for (int i = 0; el->names[i]; i++) {
+ for (i = 0; el->names[i]; i++) {
HBAC_DEBUG(HBAC_DBG_TRACE, "\t\t\t[%s]\n",
el->names[i]);
}
} else {
@@ -452,7 +458,7 @@ static void hbac_rule_element_debug_print(struct hbac_rule_element
*el,
if (el->groups) {
if (el->groups[0]) {
HBAC_DEBUG(HBAC_DBG_TRACE, "\t\t%s_groups:\n", label);
- for (int i = 0; el->groups[i]; i++) {
+ for (i = 0; el->groups[i]; i++) {
HBAC_DEBUG(HBAC_DBG_TRACE, "\t\t\t[%s]\n",
el->groups[i]);
}
} else {
LGTM but it would be good to enforce old standard with compiler flags.
@see POC in attached diff.
From 58bf614791539f407495a4b09cb4cccc0bf1f51f Mon Sep 17 00:00:00
2001
From: Jakub Hrozek <jhrozek(a)redhat.com>
Date: Thu, 10 Mar 2016 07:38:06 +0100
Subject: [PATCH 2/4] libipa_hbac: Add more debug messages
Adding more debug messages proved to be useful during pam_hbac
development.
---
src/providers/ipa/hbac_evaluator.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
ACK
From 36496d9b8b7850baab9d8c67f35e5d699379c0a6 Mon Sep 17 00:00:00
2001
From: Jakub Hrozek <jhrozek(a)redhat.com>
Date: Thu, 10 Mar 2016 07:45:28 +0100
Subject: [PATCH 3/4] libipa_hbac: Fix typo in constant name
On platforms without the format attribute, libhbac could not be
compiled.
---
src/providers/ipa/ipa_hbac.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/providers/ipa/ipa_hbac.h b/src/providers/ipa/ipa_hbac.h
index ee5f1919b0e216bd6dc09e26b1488fc4bf2d28ef..22dd8ffc401d2e6a2b6e4f83e99447b50192102b
100644
--- a/src/providers/ipa/ipa_hbac.h
+++ b/src/providers/ipa/ipa_hbac.h
@@ -53,7 +53,7 @@ enum hbac_debug_level {
#ifdef HAVE_FUNCTION_ATTRIBUTE_FORMAT
#define HBAC_ATTRIBUTE_PRINTF(a1, a2) __attribute__((format(printf, a1, a2)))
#else
-#define HABC_ATTRIBUTE_PRINTF(a1, a2)
+#define HBAC_ATTRIBUTE_PRINTF(a1, a2)
I'm sorry for typo.
Obvious ACK.
From 606e66fe7c18a9e78cd1709f6a95e3a53be24991 Mon Sep 17 00:00:00
2001
From: Jakub Hrozek <jhrozek(a)redhat.com>
Date: Thu, 10 Mar 2016 08:19:58 +0100
Subject: [PATCH 4/4] libipa_hbac: Move the library to src/lib/ipa_hbac
Moving the library to the lib directory will force maintainers to think
twice about changes, because it would be obvious this is a library.
Also don't use includes from sssd source tree paths, but add the util
path to Makefile's CFLAGS so that other projects can copy the
hbac_evaluator.c file verbatim.
---
Makefile.am | 18 +++++++++++-------
configure.ac | 2 +-
src/{providers/ipa => lib/ipa_hbac}/hbac_evaluator.c | 4 ++--
src/{providers/ipa => lib/ipa_hbac}/ipa_hbac.doxy.in | 0
src/{providers/ipa => lib/ipa_hbac}/ipa_hbac.exports | 0
src/{providers/ipa => lib/ipa_hbac}/ipa_hbac.h | 0
src/{providers/ipa => lib/ipa_hbac}/ipa_hbac.pc.in | 0
src/providers/ipa/ipa_access.c | 1 -
src/providers/ipa/ipa_hbac_common.c | 1 -
src/providers/ipa/ipa_hbac_private.h | 2 +-
src/python/pyhbac.c | 2 +-
src/tests/ipa_hbac-tests.c | 2 +-
12 files changed, 17 insertions(+), 15 deletions(-)
rename src/{providers/ipa => lib/ipa_hbac}/hbac_evaluator.c (99%)
rename src/{providers/ipa => lib/ipa_hbac}/ipa_hbac.doxy.in (100%)
rename src/{providers/ipa => lib/ipa_hbac}/ipa_hbac.exports (100%)
rename src/{providers/ipa => lib/ipa_hbac}/ipa_hbac.h (100%)
rename src/{providers/ipa => lib/ipa_hbac}/ipa_hbac.pc.in (100%)
LGTM
LS