First patch is a minor bug in unit test I found when I was writing new tests. The rest is described in commit message.
On (02/07/14 13:09), Pavel Březina wrote:
First patch is a minor bug in unit test I found when I was writing new tests. The rest is described in commit message.
From de3ed7bf0e9784058241e4b532c72e324e3dd635 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Tue, 1 Jul 2014 11:55:41 +0200 Subject: [PATCH 2/4] sss_sifp: set output parameters if attribute is NULL
There are two cases that may happen when a user calls Get or GetAll:
- the attribute is missing
- the attribute is empty
sss_sifp has two error code to distinguish between those two cases:
- SSS_SIFP_ATTR_MISSING
- SSS_SIFP_ATTR_NULL
Usually the caller is not interested on situations when the attribute is empty and it can be considered as error. Having it as a separate error code instead of setting the output value to NULL is necesarry since attribute does not have to be a pointer.
This patch however sets pointer type attributes to NULL since it may simplify the code path when the caller is actually interested in this information (e. g. empty server list on domain objects).
It is not possible to send a NULL string over a D-Bus nor it is possible to have hash table NULL with current code so these two scenarios are not tested. However, it is handled in sss_sifp_attr code for completeness.
src/lib/sifp/sss_sifp_attrs.c | 63 +++++--- src/tests/cmocka/test_sss_sifp.c | 338 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 380 insertions(+), 21 deletions(-)
diff --git a/src/lib/sifp/sss_sifp_attrs.c b/src/lib/sifp/sss_sifp_attrs.c index 6d10c46119989b42e79cc80f251fa275d07b1cd0..5a0241f85f734891f5273d1f04e5721be859b263 100644 --- a/src/lib/sifp/sss_sifp_attrs.c +++ b/src/lib/sifp/sss_sifp_attrs.c @@ -41,23 +41,31 @@ out = attr->data.field[0]; \ } while (0)
-#define GET_ATTR_ARRAY(attrs, name, rtype, field, out_num, out_val) do { \ +#define GET_ATTR_ARRAY(attrs, name, rtype, field, out_num, out_val, ret) \ +do { \ sss_sifp_attr *attr = sss_sifp_find_attr(attrs, name); \ \ if (attr == NULL) { \
return SSS_SIFP_ATTR_MISSING; \
ret = SSS_SIFP_ATTR_MISSING; \break; \
return SSS_SIFP_INCORRECT_TYPE; \
ret = SSS_SIFP_INCORRECT_TYPE; \break; \
return SSS_SIFP_ATTR_NULL; \
out_num = 0; \out_val = NULL; \ret = SSS_SIFP_ATTR_NULL; \break; \\- ret = SSS_SIFP_OK; \
} while (0)
@@ -151,6 +159,7 @@ sss_sifp_find_attr_as_string(sss_sifp_attr **attrs, GET_ATTR(attrs, name, SSS_SIFP_ATTR_TYPE_STRING, str, value);
function sss_sifp_find_attr_as_string can return SSS_SIFP_ATTR_NULL also from code generated by macro GET_ATTR. Do we want to set value to NULL there?
Other wise patches looks good.
LS
On 07/10/2014 02:08 PM, Lukas Slebodnik wrote:
On (02/07/14 13:09), Pavel Březina wrote:
First patch is a minor bug in unit test I found when I was writing new tests. The rest is described in commit message.
From de3ed7bf0e9784058241e4b532c72e324e3dd635 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Tue, 1 Jul 2014 11:55:41 +0200 Subject: [PATCH 2/4] sss_sifp: set output parameters if attribute is NULL
There are two cases that may happen when a user calls Get or GetAll:
- the attribute is missing
- the attribute is empty
sss_sifp has two error code to distinguish between those two cases:
- SSS_SIFP_ATTR_MISSING
- SSS_SIFP_ATTR_NULL
Usually the caller is not interested on situations when the attribute is empty and it can be considered as error. Having it as a separate error code instead of setting the output value to NULL is necesarry since attribute does not have to be a pointer.
This patch however sets pointer type attributes to NULL since it may simplify the code path when the caller is actually interested in this information (e. g. empty server list on domain objects).
It is not possible to send a NULL string over a D-Bus nor it is possible to have hash table NULL with current code so these two scenarios are not tested. However, it is handled in sss_sifp_attr code for completeness.
src/lib/sifp/sss_sifp_attrs.c | 63 +++++--- src/tests/cmocka/test_sss_sifp.c | 338 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 380 insertions(+), 21 deletions(-)
diff --git a/src/lib/sifp/sss_sifp_attrs.c b/src/lib/sifp/sss_sifp_attrs.c index 6d10c46119989b42e79cc80f251fa275d07b1cd0..5a0241f85f734891f5273d1f04e5721be859b263 100644 --- a/src/lib/sifp/sss_sifp_attrs.c +++ b/src/lib/sifp/sss_sifp_attrs.c @@ -41,23 +41,31 @@ out = attr->data.field[0]; \ } while (0)
-#define GET_ATTR_ARRAY(attrs, name, rtype, field, out_num, out_val) do { \ +#define GET_ATTR_ARRAY(attrs, name, rtype, field, out_num, out_val, ret) \ +do { \ sss_sifp_attr *attr = sss_sifp_find_attr(attrs, name); \ \ if (attr == NULL) { \
return SSS_SIFP_ATTR_MISSING; \
ret = SSS_SIFP_ATTR_MISSING; \break; \
return SSS_SIFP_INCORRECT_TYPE; \
ret = SSS_SIFP_INCORRECT_TYPE; \break; \
return SSS_SIFP_ATTR_NULL; \
out_num = 0; \out_val = NULL; \ret = SSS_SIFP_ATTR_NULL; \break; \\- ret = SSS_SIFP_OK; \
} while (0)
@@ -151,6 +159,7 @@ sss_sifp_find_attr_as_string(sss_sifp_attr **attrs, GET_ATTR(attrs, name, SSS_SIFP_ATTR_TYPE_STRING, str, value);
function sss_sifp_find_attr_as_string can return SSS_SIFP_ATTR_NULL also from code generated by macro GET_ATTR. Do we want to set value to NULL there?
Other wise patches looks good.
LS
Thanks, fixed.
On 07/16/2014 01:37 PM, Pavel Březina wrote:
On 07/10/2014 02:08 PM, Lukas Slebodnik wrote:
On (02/07/14 13:09), Pavel Březina wrote:
First patch is a minor bug in unit test I found when I was writing new tests. The rest is described in commit message.
From de3ed7bf0e9784058241e4b532c72e324e3dd635 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Tue, 1 Jul 2014 11:55:41 +0200 Subject: [PATCH 2/4] sss_sifp: set output parameters if attribute is NULL
There are two cases that may happen when a user calls Get or GetAll:
- the attribute is missing
- the attribute is empty
sss_sifp has two error code to distinguish between those two cases:
- SSS_SIFP_ATTR_MISSING
- SSS_SIFP_ATTR_NULL
Usually the caller is not interested on situations when the attribute is empty and it can be considered as error. Having it as a separate error code instead of setting the output value to NULL is necesarry since attribute does not have to be a pointer.
This patch however sets pointer type attributes to NULL since it may simplify the code path when the caller is actually interested in this information (e. g. empty server list on domain objects).
It is not possible to send a NULL string over a D-Bus nor it is possible to have hash table NULL with current code so these two scenarios are not tested. However, it is handled in sss_sifp_attr code for completeness.
src/lib/sifp/sss_sifp_attrs.c | 63 +++++--- src/tests/cmocka/test_sss_sifp.c | 338 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 380 insertions(+), 21 deletions(-)
diff --git a/src/lib/sifp/sss_sifp_attrs.c b/src/lib/sifp/sss_sifp_attrs.c index 6d10c46119989b42e79cc80f251fa275d07b1cd0..5a0241f85f734891f5273d1f04e5721be859b263 100644 --- a/src/lib/sifp/sss_sifp_attrs.c +++ b/src/lib/sifp/sss_sifp_attrs.c @@ -41,23 +41,31 @@ out = attr->data.field[0]; \ } while (0)
-#define GET_ATTR_ARRAY(attrs, name, rtype, field, out_num, out_val) do { \ +#define GET_ATTR_ARRAY(attrs, name, rtype, field, out_num, out_val, ret) \ +do { \ sss_sifp_attr *attr = sss_sifp_find_attr(attrs, name); \
\ if (attr == NULL) { \
returnSSS_SIFP_ATTR_MISSING; \
ret =SSS_SIFP_ATTR_MISSING; \
break; \
} \
\ if (attr->type != rtype) { \
returnSSS_SIFP_INCORRECT_TYPE; \
ret =SSS_SIFP_INCORRECT_TYPE; \
break; \
} \
\ if (attr->data.field == NULL) { \
returnSSS_SIFP_ATTR_NULL; \
out_num =0; \
out_val =NULL; \
ret =SSS_SIFP_ATTR_NULL; \
break; \
} \
\ out_num = attr->num_values; \ out_val = attr->data.field; \
\
- ret =
SSS_SIFP_OK; \ } while (0)
@@ -151,6 +159,7 @@ sss_sifp_find_attr_as_string(sss_sifp_attr **attrs, GET_ATTR(attrs, name, SSS_SIFP_ATTR_TYPE_STRING, str, value);
function sss_sifp_find_attr_as_string can return SSS_SIFP_ATTR_NULL also from code generated by macro GET_ATTR. Do we want to set value to NULL there?
Other wise patches looks good.
LS
Thanks, fixed.
Sorry, I did not commit the changes. So one more time.
Is there any way to make git format-patch to warn if there are unstaged changes?
On (16/07/14 13:46), Pavel Březina wrote:
On 07/16/2014 01:37 PM, Pavel Březina wrote:
On 07/10/2014 02:08 PM, Lukas Slebodnik wrote:
On (02/07/14 13:09), Pavel Březina wrote:
First patch is a minor bug in unit test I found when I was writing new tests. The rest is described in commit message.
From de3ed7bf0e9784058241e4b532c72e324e3dd635 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Tue, 1 Jul 2014 11:55:41 +0200 Subject: [PATCH 2/4] sss_sifp: set output parameters if attribute is NULL
There are two cases that may happen when a user calls Get or GetAll:
- the attribute is missing
- the attribute is empty
sss_sifp has two error code to distinguish between those two cases:
- SSS_SIFP_ATTR_MISSING
- SSS_SIFP_ATTR_NULL
Usually the caller is not interested on situations when the attribute is empty and it can be considered as error. Having it as a separate error code instead of setting the output value to NULL is necesarry since attribute does not have to be a pointer.
This patch however sets pointer type attributes to NULL since it may simplify the code path when the caller is actually interested in this information (e. g. empty server list on domain objects).
It is not possible to send a NULL string over a D-Bus nor it is possible to have hash table NULL with current code so these two scenarios are not tested. However, it is handled in sss_sifp_attr code for completeness.
src/lib/sifp/sss_sifp_attrs.c | 63 +++++--- src/tests/cmocka/test_sss_sifp.c | 338 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 380 insertions(+), 21 deletions(-)
diff --git a/src/lib/sifp/sss_sifp_attrs.c b/src/lib/sifp/sss_sifp_attrs.c index 6d10c46119989b42e79cc80f251fa275d07b1cd0..5a0241f85f734891f5273d1f04e5721be859b263 100644 --- a/src/lib/sifp/sss_sifp_attrs.c +++ b/src/lib/sifp/sss_sifp_attrs.c @@ -41,23 +41,31 @@ out = attr->data.field[0]; \ } while (0)
-#define GET_ATTR_ARRAY(attrs, name, rtype, field, out_num, out_val) do { \ +#define GET_ATTR_ARRAY(attrs, name, rtype, field, out_num, out_val, ret) \ +do { \ sss_sifp_attr *attr = sss_sifp_find_attr(attrs, name); \
\ if (attr == NULL) { \
returnSSS_SIFP_ATTR_MISSING; \
ret =SSS_SIFP_ATTR_MISSING; \
break; \
} \
\ if (attr->type != rtype) { \
returnSSS_SIFP_INCORRECT_TYPE; \
ret =SSS_SIFP_INCORRECT_TYPE; \
break; \
} \
\ if (attr->data.field == NULL) { \
returnSSS_SIFP_ATTR_NULL; \
out_num =0; \
out_val =NULL; \
ret =SSS_SIFP_ATTR_NULL; \
break; \
} \
\ out_num = attr->num_values; \ out_val = attr->data.field; \
\
- ret =
SSS_SIFP_OK; \ } while (0)
@@ -151,6 +159,7 @@ sss_sifp_find_attr_as_string(sss_sifp_attr **attrs, GET_ATTR(attrs, name, SSS_SIFP_ATTR_TYPE_STRING, str, value);
function sss_sifp_find_attr_as_string can return SSS_SIFP_ATTR_NULL also from code generated by macro GET_ATTR. Do we want to set value to NULL there?
Other wise patches looks good.
LS
Thanks, fixed.
Sorry, I did not commit the changes. So one more time.
Is there any way to make git format-patch to warn if there are unstaged changes?
Thank you.
ACK to all
LS
On Wed, Jul 16, 2014 at 02:10:16PM +0200, Lukas Slebodnik wrote:
Thank you.
ACK to all
LS
Should we push all three at once?
Normally we only bump the version number before the release. I suggest we create a blocker ticket for 1.12.1 to remind us to push the last patch instead, in case we had to do some more changes..
On (16/07/14 14:38), Jakub Hrozek wrote:
On Wed, Jul 16, 2014 at 02:10:16PM +0200, Lukas Slebodnik wrote:
Thank you.
ACK to all
LS
Should we push all three at once?
Normally we only bump the version number before the release. I suggest we create a blocker ticket for 1.12.1 to remind us to push the last patch instead, in case we had to do some more changes..
It is your decision :-) You will push patches. I just ACK-ed patches.
LS
On Wed, Jul 16, 2014 at 03:49:37PM +0200, Lukas Slebodnik wrote:
On (16/07/14 14:38), Jakub Hrozek wrote:
On Wed, Jul 16, 2014 at 02:10:16PM +0200, Lukas Slebodnik wrote:
Thank you.
ACK to all
LS
Should we push all three at once?
Normally we only bump the version number before the release. I suggest we create a blocker ticket for 1.12.1 to remind us to push the last patch instead, in case we had to do some more changes..
It is your decision :-)
I will only push the first two patches and filed https://fedorahosted.org/sssd/ticket/2382 to push the last one.
I filed it directly to 1.12.1, I hope it's fine to bypass the triage with a simple reminder to push a patch.
You will push patches. I just ACK-ed patches.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Wed, Jul 16, 2014 at 05:53:32PM +0200, Jakub Hrozek wrote:
On Wed, Jul 16, 2014 at 05:46:07PM +0200, Jakub Hrozek wrote:
I will only push the first two patches
master: 973be642f3d33ba21ea9c06791295f09efcdba46 b7080f1a2c6c97224c41f6347ca3743e1054faec
We're releasing 1.12.1, so I pushed the version-info patch, too: 706d211b5d6e32d11a1c6ffc8065ca8be4d4d8c5
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Pavel Březina