Hi list,
there is patch for [1] attached.
[1] https://fedorahosted.org/sssd/ticket/3084
If you prefer two commits--one for reverting and second with the patch--don't hesitate to tell me it. :-)
Regards
On Wed, Jul 27, 2016 at 10:26:30AM +0200, Petr Cech wrote:
Hi list,
there is patch for [1] attached.
[1] https://fedorahosted.org/sssd/ticket/3084
If you prefer two commits--one for reverting and second with the patch--don't hesitate to tell me it. :-)
Pavel is best suited to review the NULL conversion, but yes, I would prefer two commits. Just call "git revert aa58e216c1f794bd335151f19e79adbb3ddf4c73", that should create the first commit and then convert to NULL as a second patch.
It would be nice to run as many downstream tests as possible as part of the review also.
On 07/27/2016 10:26 AM, Petr Cech wrote:
Hi list,
there is patch for [1] attached.
[1] https://fedorahosted.org/sssd/ticket/3084
If you prefer two commits--one for reverting and second with the patch--don't hesitate to tell me it. :-)
Regards
-- Petr^4 Čech
0001-PROVIDER-Conversion-empty-string-from-D-Bus-to-NULL.patch
From d3942580b3eb1d48a761a91b4c9c68c15cfb8c33 Mon Sep 17 00:00:00 2001 From: Petr Cechpcech@redhat.com Date: Wed, 27 Jul 2016 09:38:25 +0200 Subject: [PATCH] PROVIDER: Conversion empty string from D-Bus to NULL
This patch fixes the issue with empty string recieving from D-Bus. Data providers obtains NULL. So this is simple conversin.
It also revert "LDAP: Lookup services by all protocols unless a protocol is specified". This reverts commit aa58e216c1f794bd335151f19e79adbb3ddf4c73.
Resolves: https://fedorahosted.org/sssd/ticket/3084
src/providers/data_provider/dp_target_id.c | 4 ++-- src/providers/ldap/ldap_id_services.c | 7 ++----- 2 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/src/providers/data_provider/dp_target_id.c b/src/providers/data_provider/dp_target_id.c index f24b2ccb62c07eb900b5c057a1a3dbf824c553a9..593e2e83e43d9c5ff24a63597fe5bf658252e5c2 100644 --- a/src/providers/data_provider/dp_target_id.c +++ b/src/providers/data_provider/dp_target_id.c @@ -62,7 +62,7 @@ static bool check_and_parse_filter(struct dp_id_data *data, {0, 0, 0}}; int i;
- if (filter == NULL) {
- if (filter == NULL && filter[0] != '\0') {
SBUS_IS_STRING_EMPTY is a better choice IMHO.
return false; }
@@ -70,7 +70,7 @@ static bool check_and_parse_filter(struct dp_id_data *data, if (strncmp(filter, types[i].name, types[i].lenght) == 0) { data->filter_type = types[i].type; data->filter_value = &filter[types[i].lenght];
I think we should call SBUS_SET_STRING even in the line above. I know filter can't be NULL but in theory you can pass filter="key=" with empty value and than data->filter_value would be an empty string instead of NULL. I don't think we use an empty value in filter anywhere though.
data->extra_value = extra;
data->extra_value = SBUS_SET_STRING(extra); return true; } }
diff --git a/src/providers/ldap/ldap_id_services.c b/src/providers/ldap/ldap_id_services.c index 401228c20af31ae2df9bb3d35ed25fb6f06b1839..77215127b53297d840eaa4d2f35a75eedb085e43 100644 --- a/src/providers/ldap/ldap_id_services.c +++ b/src/providers/ldap/ldap_id_services.c @@ -89,9 +89,6 @@ services_get_send(TALLOC_CTX *mem_ctx, state->sysdb = sdom->dom->sysdb; state->name = name; state->protocol = protocol;
- if (state->protocol != NULL && state->protocol[0] == '\0') {
state->protocol = NULL;
- } state->filter_type = filter_type; state->noexist_delete = noexist_delete;
@@ -117,8 +114,8 @@ services_get_send(TALLOC_CTX *mem_ctx, ret = sss_filter_sanitize(state, name, &clean_name); if (ret != EOK) goto error;
- if (state->protocol != NULL) {
ret = sss_filter_sanitize(state, state->protocol, &clean_protocol);
- if (protocol) {
Please use if (protocol == NULL)
ret = sss_filter_sanitize(state, protocol, &clean_protocol); if (ret != EOK) goto error; }
I checked other dp targets and this place is the only one that needs changing.
On 07/27/2016 10:57 AM, Pavel Březina wrote:
On 07/27/2016 10:26 AM, Petr Cech wrote:
Hi list,
there is patch for [1] attached.
[1] https://fedorahosted.org/sssd/ticket/3084
If you prefer two commits--one for reverting and second with the patch--don't hesitate to tell me it. :-)
Regards
-- Petr^4 Čech
0001-PROVIDER-Conversion-empty-string-from-D-Bus-to-NULL.patch
From d3942580b3eb1d48a761a91b4c9c68c15cfb8c33 Mon Sep 17 00:00:00 2001 From: Petr Cechpcech@redhat.com Date: Wed, 27 Jul 2016 09:38:25 +0200 Subject: [PATCH] PROVIDER: Conversion empty string from D-Bus to NULL
This patch fixes the issue with empty string recieving from D-Bus. Data providers obtains NULL. So this is simple conversin.
It also revert "LDAP: Lookup services by all protocols unless a protocol is specified". This reverts commit aa58e216c1f794bd335151f19e79adbb3ddf4c73.
Resolves: https://fedorahosted.org/sssd/ticket/3084
src/providers/data_provider/dp_target_id.c | 4 ++-- src/providers/ldap/ldap_id_services.c | 7 ++----- 2 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/src/providers/data_provider/dp_target_id.c b/src/providers/data_provider/dp_target_id.c index f24b2ccb62c07eb900b5c057a1a3dbf824c553a9..593e2e83e43d9c5ff24a63597fe5bf658252e5c2 100644 --- a/src/providers/data_provider/dp_target_id.c +++ b/src/providers/data_provider/dp_target_id.c @@ -62,7 +62,7 @@ static bool check_and_parse_filter(struct dp_id_data *data, {0, 0, 0}}; int i;
- if (filter == NULL) {
- if (filter == NULL && filter[0] != '\0') {
SBUS_IS_STRING_EMPTY is a better choice IMHO.
Addressed.
return false; }
@@ -70,7 +70,7 @@ static bool check_and_parse_filter(struct dp_id_data *data, if (strncmp(filter, types[i].name, types[i].lenght) == 0) { data->filter_type = types[i].type; data->filter_value = &filter[types[i].lenght];
I think we should call SBUS_SET_STRING even in the line above. I know filter can't be NULL but in theory you can pass filter="key=" with empty value and than data->filter_value would be an empty string instead of NULL. I don't think we use an empty value in filter anywhere though.
Addressed.
data->extra_value = extra;
data->extra_value = SBUS_SET_STRING(extra); return true; } }
diff --git a/src/providers/ldap/ldap_id_services.c b/src/providers/ldap/ldap_id_services.c index 401228c20af31ae2df9bb3d35ed25fb6f06b1839..77215127b53297d840eaa4d2f35a75eedb085e43 100644 --- a/src/providers/ldap/ldap_id_services.c +++ b/src/providers/ldap/ldap_id_services.c @@ -89,9 +89,6 @@ services_get_send(TALLOC_CTX *mem_ctx, state->sysdb = sdom->dom->sysdb; state->name = name; state->protocol = protocol;
- if (state->protocol != NULL && state->protocol[0] == '\0') {
state->protocol = NULL;
- } state->filter_type = filter_type; state->noexist_delete = noexist_delete;
@@ -117,8 +114,8 @@ services_get_send(TALLOC_CTX *mem_ctx, ret = sss_filter_sanitize(state, name, &clean_name); if (ret != EOK) goto error;
- if (state->protocol != NULL) {
ret = sss_filter_sanitize(state, state->protocol,
&clean_protocol);
- if (protocol) {
Please use if (protocol == NULL)
Addressed.
This last is changing original code before commit reverting. Reverted commit used the opposite condition.
Is it right this way?
ret = sss_filter_sanitize(state, protocol, &clean_protocol); if (ret != EOK) goto error; }
I checked other dp targets and this place is the only one that needs changing.
Thank you, Jakub and Pavel, for comments. Fixed patch set is attached.
bump
On 07/27/2016 11:17 AM, Petr Cech wrote:
On 07/27/2016 10:57 AM, Pavel Březina wrote:
On 07/27/2016 10:26 AM, Petr Cech wrote:
Hi list,
there is patch for [1] attached.
[1] https://fedorahosted.org/sssd/ticket/3084
If you prefer two commits--one for reverting and second with the patch--don't hesitate to tell me it. :-)
Regards
-- Petr^4 Čech
0001-PROVIDER-Conversion-empty-string-from-D-Bus-to-NULL.patch
From d3942580b3eb1d48a761a91b4c9c68c15cfb8c33 Mon Sep 17 00:00:00 2001 From: Petr Cechpcech@redhat.com Date: Wed, 27 Jul 2016 09:38:25 +0200 Subject: [PATCH] PROVIDER: Conversion empty string from D-Bus to NULL
This patch fixes the issue with empty string recieving from D-Bus. Data providers obtains NULL. So this is simple conversin.
It also revert "LDAP: Lookup services by all protocols unless a protocol is specified". This reverts commit aa58e216c1f794bd335151f19e79adbb3ddf4c73.
Resolves: https://fedorahosted.org/sssd/ticket/3084
src/providers/data_provider/dp_target_id.c | 4 ++-- src/providers/ldap/ldap_id_services.c | 7 ++----- 2 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/src/providers/data_provider/dp_target_id.c b/src/providers/data_provider/dp_target_id.c index f24b2ccb62c07eb900b5c057a1a3dbf824c553a9..593e2e83e43d9c5ff24a63597fe5bf658252e5c2
100644 --- a/src/providers/data_provider/dp_target_id.c +++ b/src/providers/data_provider/dp_target_id.c @@ -62,7 +62,7 @@ static bool check_and_parse_filter(struct dp_id_data *data, {0, 0, 0}}; int i;
- if (filter == NULL) {
- if (filter == NULL && filter[0] != '\0') {
SBUS_IS_STRING_EMPTY is a better choice IMHO.
Addressed.
return false; }
@@ -70,7 +70,7 @@ static bool check_and_parse_filter(struct dp_id_data *data, if (strncmp(filter, types[i].name, types[i].lenght) == 0) { data->filter_type = types[i].type; data->filter_value = &filter[types[i].lenght];
I think we should call SBUS_SET_STRING even in the line above. I know filter can't be NULL but in theory you can pass filter="key=" with empty value and than data->filter_value would be an empty string instead of NULL. I don't think we use an empty value in filter anywhere though.
Addressed.
data->extra_value = extra;
data->extra_value = SBUS_SET_STRING(extra); return true; } }
diff --git a/src/providers/ldap/ldap_id_services.c b/src/providers/ldap/ldap_id_services.c index 401228c20af31ae2df9bb3d35ed25fb6f06b1839..77215127b53297d840eaa4d2f35a75eedb085e43
100644 --- a/src/providers/ldap/ldap_id_services.c +++ b/src/providers/ldap/ldap_id_services.c @@ -89,9 +89,6 @@ services_get_send(TALLOC_CTX *mem_ctx, state->sysdb = sdom->dom->sysdb; state->name = name; state->protocol = protocol;
- if (state->protocol != NULL && state->protocol[0] == '\0') {
state->protocol = NULL;
- } state->filter_type = filter_type; state->noexist_delete = noexist_delete;
@@ -117,8 +114,8 @@ services_get_send(TALLOC_CTX *mem_ctx, ret = sss_filter_sanitize(state, name, &clean_name); if (ret != EOK) goto error;
- if (state->protocol != NULL) {
ret = sss_filter_sanitize(state, state->protocol,
&clean_protocol);
- if (protocol) {
Please use if (protocol == NULL)
Addressed.
This last is changing original code before commit reverting. Reverted commit used the opposite condition.
Is it right this way?
ret = sss_filter_sanitize(state, protocol, &clean_protocol); if (ret != EOK) goto error; }
I checked other dp targets and this place is the only one that needs changing.
Thank you, Jakub and Pavel, for comments. Fixed patch set is attached.
Ack.
On (05/08/16 11:56), Pavel Březina wrote:
On 07/27/2016 11:17 AM, Petr Cech wrote:
On 07/27/2016 10:57 AM, Pavel Březina wrote:
On 07/27/2016 10:26 AM, Petr Cech wrote:
Hi list,
there is patch for [1] attached.
[1] https://fedorahosted.org/sssd/ticket/3084
If you prefer two commits--one for reverting and second with the patch--don't hesitate to tell me it. :-)
Regards
-- Petr^4 Čech
0001-PROVIDER-Conversion-empty-string-from-D-Bus-to-NULL.patch
From d3942580b3eb1d48a761a91b4c9c68c15cfb8c33 Mon Sep 17 00:00:00 2001 From: Petr Cechpcech@redhat.com Date: Wed, 27 Jul 2016 09:38:25 +0200 Subject: [PATCH] PROVIDER: Conversion empty string from D-Bus to NULL
This patch fixes the issue with empty string recieving from D-Bus. Data providers obtains NULL. So this is simple conversin.
It also revert "LDAP: Lookup services by all protocols unless a protocol is specified". This reverts commit aa58e216c1f794bd335151f19e79adbb3ddf4c73.
Resolves: https://fedorahosted.org/sssd/ticket/3084
src/providers/data_provider/dp_target_id.c | 4 ++-- src/providers/ldap/ldap_id_services.c | 7 ++----- 2 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/src/providers/data_provider/dp_target_id.c b/src/providers/data_provider/dp_target_id.c index f24b2ccb62c07eb900b5c057a1a3dbf824c553a9..593e2e83e43d9c5ff24a63597fe5bf658252e5c2
100644 --- a/src/providers/data_provider/dp_target_id.c +++ b/src/providers/data_provider/dp_target_id.c @@ -62,7 +62,7 @@ static bool check_and_parse_filter(struct dp_id_data *data, {0, 0, 0}}; int i;
- if (filter == NULL) {
- if (filter == NULL && filter[0] != '\0') {
SBUS_IS_STRING_EMPTY is a better choice IMHO.
Addressed.
return false; }
@@ -70,7 +70,7 @@ static bool check_and_parse_filter(struct dp_id_data *data, if (strncmp(filter, types[i].name, types[i].lenght) == 0) { data->filter_type = types[i].type; data->filter_value = &filter[types[i].lenght];
I think we should call SBUS_SET_STRING even in the line above. I know filter can't be NULL but in theory you can pass filter="key=" with empty value and than data->filter_value would be an empty string instead of NULL. I don't think we use an empty value in filter anywhere though.
Addressed.
data->extra_value = extra;
data->extra_value = SBUS_SET_STRING(extra); return true; } }
diff --git a/src/providers/ldap/ldap_id_services.c b/src/providers/ldap/ldap_id_services.c index 401228c20af31ae2df9bb3d35ed25fb6f06b1839..77215127b53297d840eaa4d2f35a75eedb085e43
100644 --- a/src/providers/ldap/ldap_id_services.c +++ b/src/providers/ldap/ldap_id_services.c @@ -89,9 +89,6 @@ services_get_send(TALLOC_CTX *mem_ctx, state->sysdb = sdom->dom->sysdb; state->name = name; state->protocol = protocol;
- if (state->protocol != NULL && state->protocol[0] == '\0') {
state->protocol = NULL;
- } state->filter_type = filter_type; state->noexist_delete = noexist_delete;
@@ -117,8 +114,8 @@ services_get_send(TALLOC_CTX *mem_ctx, ret = sss_filter_sanitize(state, name, &clean_name); if (ret != EOK) goto error;
- if (state->protocol != NULL) {
ret = sss_filter_sanitize(state, state->protocol,
&clean_protocol);
- if (protocol) {
Please use if (protocol == NULL)
Addressed.
This last is changing original code before commit reverting. Reverted commit used the opposite condition.
Is it right this way?
ret = sss_filter_sanitize(state, protocol, &clean_protocol); if (ret != EOK) goto error; }
I checked other dp targets and this place is the only one that needs changing.
Thank you, Jakub and Pavel, for comments. Fixed patch set is attached.
Ack.
master: * dc30c60f166ad9adc63a47a1013508a71624ac87 * e4ba22c165be4b0ccd0e1b9fc92f2f1e35f78a82
LS
sssd-devel@lists.fedorahosted.org