On 11/14/2013 01:14 PM, Lukas Slebodnik wrote:>>From
- ((uint32_t *)body)[0] = num-skipped; /* num results */
- ((uint32_t *)body)[1] = 0; /* reserved */
- SAFEALIGN_SETMEM_UINT32(body, num - skipped, NULL); /* num
results */
- SAFEALIGN_SETMEM_UINT32(body + sizeof(uint32_t), 0, NULL); /*
reserved */
Here is an conflict due to patch "NSS: Set packet length for initgroups"
Resolved.
return EOK;}
New patch is attached.
Michal
On (15/11/13 17:28), Michal Židek wrote:
On 11/14/2013 01:14 PM, Lukas Slebodnik wrote:>>From
- ((uint32_t *)body)[0] = num-skipped; /* num results */
- ((uint32_t *)body)[1] = 0; /* reserved */
- SAFEALIGN_SETMEM_UINT32(body, num - skipped, NULL); /* num
results */
- SAFEALIGN_SETMEM_UINT32(body + sizeof(uint32_t), 0, NULL);
/* reserved */
Here is an conflict due to patch "NSS: Set packet length for initgroups"
Resolved.
return EOK;}
New patch is attached.
Michal
From 29f8a7522d60949cb1d37b0e66669509f39fb71a Mon Sep 17 00:00:00 2001 From: Michal Zidek mzidek@redhat.com Date: Wed, 28 Aug 2013 12:46:58 +0200 Subject: [PATCH 4/7] responder: Use SAFEALIGN macros where appropriate.
https://fedorahosted.org/sssd/ticket/1359
src/responder/autofs/autofssrv_cmd.c | 8 ++++-- src/responder/common/responder_cmd.c | 20 ++++++++++---- src/responder/nss/nsssrv_cmd.c | 52 +++++++++++++++++++---------------- src/responder/nss/nsssrv_mmap_cache.c | 2 +- src/responder/nss/nsssrv_netgroup.c | 18 ++++++++---- src/responder/nss/nsssrv_services.c | 9 ++++-- 6 files changed, 70 insertions(+), 39 deletions(-)
Few warnings are not fixed in the file src/responder/nss/nsssrv_cmd.c and patches {0005, 0006} do not change this file.
CC src/responder/nss/nsssrv_cmd.o src/responder/nss/nsssrv_cmd.c:1017:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[0] = 1; /* num results */ ^~~~~~~~~~~~~~~~ src/responder/nss/nsssrv_cmd.c:1018:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[1] = 0; /* reserved */ ^~~~~~~~~~~~~~~~ src/responder/nss/nsssrv_cmd.c:1019:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[2] = (uint32_t) SSS_ID_TYPE_GID; ^~~~~~~~~~~~~~~~ src/responder/nss/nsssrv_cmd.c:4400:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[0] = 1; /* num results */ ^~~~~~~~~~~~~~~~ src/responder/nss/nsssrv_cmd.c:4401:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[1] = 0; /* reserved */ ^~~~~~~~~~~~~~~~ src/responder/nss/nsssrv_cmd.c:4402:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[2] = (uint32_t) SSS_ID_TYPE_GID; ^~~~~~~~~~~~~~~~ 6 warnings generated.
LS
On 11/18/2013 04:02 PM, Lukas Slebodnik wrote:
Few warnings are not fixed in the file src/responder/nss/nsssrv_cmd.c and patches {0005, 0006} do not change this file.
CC src/responder/nss/nsssrv_cmd.o src/responder/nss/nsssrv_cmd.c:1017:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[0] = 1; /* num results */ ^~~~~~~~~~~~~~~~ src/responder/nss/nsssrv_cmd.c:1018:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[1] = 0; /* reserved */ ^~~~~~~~~~~~~~~~ src/responder/nss/nsssrv_cmd.c:1019:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[2] = (uint32_t) SSS_ID_TYPE_GID; ^~~~~~~~~~~~~~~~ src/responder/nss/nsssrv_cmd.c:4400:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[0] = 1; /* num results */ ^~~~~~~~~~~~~~~~ src/responder/nss/nsssrv_cmd.c:4401:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[1] = 0; /* reserved */ ^~~~~~~~~~~~~~~~ src/responder/nss/nsssrv_cmd.c:4402:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[2] = (uint32_t) SSS_ID_TYPE_GID; ^~~~~~~~~~~~~~~~ 6 warnings generated.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks. For the review. New patch is attached.
Michal
On (19/11/13 14:01), Michal Židek wrote:
On 11/18/2013 04:02 PM, Lukas Slebodnik wrote:
Few warnings are not fixed in the file src/responder/nss/nsssrv_cmd.c and patches {0005, 0006} do not change this file.
CC src/responder/nss/nsssrv_cmd.o src/responder/nss/nsssrv_cmd.c:1017:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[0] = 1; /* num results */ ^~~~~~~~~~~~~~~~ src/responder/nss/nsssrv_cmd.c:1018:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[1] = 0; /* reserved */ ^~~~~~~~~~~~~~~~ src/responder/nss/nsssrv_cmd.c:1019:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[2] = (uint32_t) SSS_ID_TYPE_GID; ^~~~~~~~~~~~~~~~ src/responder/nss/nsssrv_cmd.c:4400:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[0] = 1; /* num results */ ^~~~~~~~~~~~~~~~ src/responder/nss/nsssrv_cmd.c:4401:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[1] = 0; /* reserved */ ^~~~~~~~~~~~~~~~ src/responder/nss/nsssrv_cmd.c:4402:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[2] = (uint32_t) SSS_ID_TYPE_GID; ^~~~~~~~~~~~~~~~ 6 warnings generated.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks. For the review. New patch is attached.
Michal
Macros SAFEALIGN_SETMEM_UINT32, SAFEALIGN_COPY_UINT32 are not used with the same purpose. Once, you used 1st macro to store value into buffer and another time you used 2nd macro for the same purpose.
- ((uint32_t *)body)[0] = num; /* num results */
- ((uint32_t *)body)[1] = 0; /* reserved */
- SAFEALIGN_COPY_UINT32(body, &num, NULL); /* num results */
- SAFEALIGN_SETMEM_UINT32(body + sizeof(uint32_t), 0, NULL); /* reserved */
You also mixed SAFEALIGN_SETMEM_UINT32 with counter
- SAFEALIGN_SETMEM_UINT32(body, 1, &pctr); /* num results */
- SAFEALIGN_SETMEM_UINT32(body + pctr, 0, &pctr); /* reserved */
- SAFEALIGN_SETMEM_UINT32(body + pctr, SSS_ID_TYPE_GID, &pctr);
and another time without counter.
/* Got some results */SAFEALIGN_SETMEM_UINT32(body, 1, NULL);/* Reserved padding */SAFEALIGN_SETMEM_UINT32(body + sizeof(uint32_t), 0, NULL);
LS
On 11/28/2013 11:59 AM, Lukas Slebodnik wrote:
On (19/11/13 14:01), Michal Židek wrote:
On 11/18/2013 04:02 PM, Lukas Slebodnik wrote:
Few warnings are not fixed in the file src/responder/nss/nsssrv_cmd.c and patches {0005, 0006} do not change this file.
CC src/responder/nss/nsssrv_cmd.o src/responder/nss/nsssrv_cmd.c:1017:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[0] = 1; /* num results */ ^~~~~~~~~~~~~~~~ src/responder/nss/nsssrv_cmd.c:1018:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[1] = 0; /* reserved */ ^~~~~~~~~~~~~~~~ src/responder/nss/nsssrv_cmd.c:1019:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[2] = (uint32_t) SSS_ID_TYPE_GID; ^~~~~~~~~~~~~~~~ src/responder/nss/nsssrv_cmd.c:4400:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[0] = 1; /* num results */ ^~~~~~~~~~~~~~~~ src/responder/nss/nsssrv_cmd.c:4401:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[1] = 0; /* reserved */ ^~~~~~~~~~~~~~~~ src/responder/nss/nsssrv_cmd.c:4402:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[2] = (uint32_t) SSS_ID_TYPE_GID; ^~~~~~~~~~~~~~~~ 6 warnings generated.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks. For the review. New patch is attached.
Michal
Macros SAFEALIGN_SETMEM_UINT32, SAFEALIGN_COPY_UINT32 are not used with the same purpose.Once, you used 1st macro to store value into buffer and another time you used 2nd macro for the same purpose.
I did not use it for the same purpose.
- ((uint32_t *)body)[0] = num; /* num results */
- ((uint32_t *)body)[1] = 0; /* reserved */
- SAFEALIGN_COPY_UINT32(body, &num, NULL); /* num results */
Here you copy num to buffer.
- SAFEALIGN_SETMEM_UINT32(body + sizeof(uint32_t), 0, NULL); /* reserved */
Here you set memory in buffer to value 0.
To express it more clearly these two codes are equivalent: a) // Here we use variable. uint32_t variable = 145; SAFEALIGN_COPY_UINT32(buffer, &variable, NULL);
b) // Here we use numeric literal 145 SAFEALIGN_SETMEM_UINT32(buffer, 145, NULL);
So with the macro _COPY you need to provide address of source (in the example it is &variable) and the content will be copied to the memory pointed by first parameter (in the example it is buffer).
The macro _SETMEM needs just value, that you want to store at the target address. It internally creates tmp variable, stores the value there and then does the same thing as _COPY.
You also mixed SAFEALIGN_SETMEM_UINT32 with counter
- SAFEALIGN_SETMEM_UINT32(body, 1, &pctr); /* num results */
- SAFEALIGN_SETMEM_UINT32(body + pctr, 0, &pctr); /* reserved */
- SAFEALIGN_SETMEM_UINT32(body + pctr, SSS_ID_TYPE_GID, &pctr);
^^^^^^^^^^^^^^^ This is not address, but direct value that we want to store.
and another time without counter.
/* Got some results */SAFEALIGN_SETMEM_UINT32(body, 1, NULL);
^^ Value, not address.
/* Reserved padding */SAFEALIGN_SETMEM_UINT32(body + sizeof(uint32_t), 0, NULL);
^^ Same here.
LS
Michal
On (19/11/13 14:01), Michal Židek wrote:
On 11/18/2013 04:02 PM, Lukas Slebodnik wrote:
Few warnings are not fixed in the file src/responder/nss/nsssrv_cmd.c and patches {0005, 0006} do not change this file.
CC src/responder/nss/nsssrv_cmd.o src/responder/nss/nsssrv_cmd.c:1017:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[0] = 1; /* num results */ ^~~~~~~~~~~~~~~~ src/responder/nss/nsssrv_cmd.c:1018:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[1] = 0; /* reserved */ ^~~~~~~~~~~~~~~~ src/responder/nss/nsssrv_cmd.c:1019:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[2] = (uint32_t) SSS_ID_TYPE_GID; ^~~~~~~~~~~~~~~~ src/responder/nss/nsssrv_cmd.c:4400:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[0] = 1; /* num results */ ^~~~~~~~~~~~~~~~ src/responder/nss/nsssrv_cmd.c:4401:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[1] = 0; /* reserved */ ^~~~~~~~~~~~~~~~ src/responder/nss/nsssrv_cmd.c:4402:6: warning: cast from 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align] ((uint32_t *)body)[2] = (uint32_t) SSS_ID_TYPE_GID; ^~~~~~~~~~~~~~~~ 6 warnings generated.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks. For the review. New patch is attached.
Michal
From 3cdb7e7640630f28ac7d766459d57bb509e9ee9e Mon Sep 17 00:00:00 2001 From: Michal Zidek mzidek@redhat.com Date: Wed, 28 Aug 2013 12:46:58 +0200 Subject: [PATCH] responder: Use SAFEALIGN macros where appropriate.
This patch needs to be rebased.
LS
On 01/22/2014 07:50 PM, Lukas Slebodnik wrote:
This patch needs to be rebased.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Rebased patch attached.
Michal
On (29/01/14 15:39), Michal Židek wrote:
On 01/22/2014 07:50 PM, Lukas Slebodnik wrote:
This patch needs to be rebased.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Rebased patch attached.
Michal
From e00ca05680e8752fa9a1a3919b8c251aa79769a4 Mon Sep 17 00:00:00 2001 From: Michal Zidek mzidek@redhat.com Date: Wed, 28 Aug 2013 12:46:58 +0200 Subject: [PATCH] responder: Use SAFEALIGN macros where appropriate.
https://fedorahosted.org/sssd/ticket/1359
src/responder/autofs/autofssrv_cmd.c | 8 +++- src/responder/common/responder_cmd.c | 20 +++++++--- src/responder/nss/nsssrv_cmd.c | 70 +++++++++++++++++++---------------- src/responder/nss/nsssrv_mmap_cache.c | 2 +- src/responder/nss/nsssrv_netgroup.c | 18 ++++++--- src/responder/nss/nsssrv_services.c | 9 +++-- 6 files changed, 80 insertions(+), 47 deletions(-)
diff --git a/src/responder/autofs/autofssrv_cmd.c b/src/responder/autofs/autofssrv_cmd.c
OK
diff --git a/src/responder/common/responder_cmd.c b/src/responder/common/responder_cmd.c index 3a3fca9..111a19c 100644 --- a/src/responder/common/responder_cmd.c +++ b/src/responder/common/responder_cmd.c @@ -51,8 +51,12 @@ int sss_cmd_empty_packet(struct sss_packet *packet) if (ret != EOK) return ret;
sss_packet_get_body(packet, &body, &blen);
- ((uint32_t *)body)[0] = 0; /* num results */
- ((uint32_t *)body)[1] = 0; /* reserved */
/* num results */
SAFEALIGN_SETMEM_UINT32(body, 0, NULL);
/* reserved */
SAFEALIGN_SETMEM_UINT32(body + sizeof(uint32_t), 0, NULL);
return EOK;
} @@ -97,6 +101,7 @@ int sss_cmd_get_version(struct cli_ctx *cctx) size_t blen; int ret; uint32_t client_version;
- uint32_t protocol_version; int i; static struct cli_protocol_version *cli_protocol_version = NULL;
@@ -133,9 +138,14 @@ int sss_cmd_get_version(struct cli_ctx *cctx) return ret; } sss_packet_get_body(cctx->creq->out, &body, &blen);
- ((uint32_t *)body)[0] = cctx->cli_protocol_version!=NULL ?
cctx->cli_protocol_version->version : 0;- DEBUG(5, ("Offered version [%d].\n", ((uint32_t *)body)[0]));
- if (cctx->cli_protocol_version != NULL) {
protocol_version = cctx->cli_protocol_version->version;- } else {
protocol_version = 0;- }
Is there special reason why you replace ternary operator with if .. else ?
SAFEALIGN_COPY_UINT32(body, &protocol_version, NULL);
DEBUG(SSSDBG_FUNC_DATA, ("Offered version [%d].\n", protocol_version));
sss_cmd_done(cctx, NULL); return EOK;
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c index 6a1e6a0..1f6ebc3 100644 --- a/src/responder/nss/nsssrv_cmd.c +++ b/src/responder/nss/nsssrv_cmd.c
OK
diff --git a/src/responder/nss/nsssrv_mmap_cache.c b/src/responder/nss/nsssrv_mmap_cache.c index 8655a1a..36110d6 100644 --- a/src/responder/nss/nsssrv_mmap_cache.c +++ b/src/responder/nss/nsssrv_mmap_cache.c
OK
diff --git a/src/responder/nss/nsssrv_netgroup.c b/src/responder/nss/nsssrv_netgroup.c index 3fc4b64..0ed0a81 100644 --- a/src/responder/nss/nsssrv_netgroup.c +++ b/src/responder/nss/nsssrv_netgroup.c @@ -680,8 +680,12 @@ static void nss_cmd_setnetgrent_done(struct tevent_req *req) }
sss_packet_get_body(packet, &body, &blen);
((uint32_t *)body)[0] = 1; /* Got some results */((uint32_t *)body)[1] = 0; /* reserved */
/* Got some results. */SAFEALIGN_SETMEM_UINT32(body, 1, NULL);/* reserved */SAFEALIGN_SETMEM_UINT32(body + sizeof(uint32_t), 0, NULL); } sss_cmd_done(cmdctx->cctx, cmdctx);@@ -842,7 +846,7 @@ static errno_t nss_cmd_getnetgrent_process(struct nss_cmd_ctx *cmdctx, if (blen != sizeof(uint32_t)) { return EINVAL; }
- num = *((uint32_t *)body);
SAFEALIGN_COPY_UINT32(&num, body, NULL);
/* create response packet */ ret = sss_packet_new(client->creq, 0,
@@ -981,8 +985,12 @@ static errno_t nss_cmd_retnetgrent(struct cli_ctx *client, }
sss_packet_get_body(packet, &body, &blen);
- ((uint32_t *)body)[0] = num; /* num results */
- ((uint32_t *)body)[1] = 0; /* reserved */
- /* num results */
- SAFEALIGN_COPY_UINT32(body, &num, NULL);
- /* reserved */
- SAFEALIGN_COPY_UINT32(body + sizeof(uint32_t), &num, NULL);
^^^^^^ Even if it is reserved, it can be confusing in future why we sending num result twice.
return EOK;} diff --git a/src/responder/nss/nsssrv_services.c b/src/responder/nss/nsssrv_services.c index 390e84e..80c59e2 100644 --- a/src/responder/nss/nsssrv_services.c +++ b/src/responder/nss/nsssrv_services.c
OK
LS
On 01/31/2014 06:12 PM, Lukas Slebodnik wrote:
On (29/01/14 15:39), Michal Židek wrote:
On 01/22/2014 07:50 PM, Lukas Slebodnik wrote:
This patch needs to be rebased.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Rebased patch attached.
Michal
From e00ca05680e8752fa9a1a3919b8c251aa79769a4 Mon Sep 17 00:00:00 2001 From: Michal Zidek mzidek@redhat.com Date: Wed, 28 Aug 2013 12:46:58 +0200 Subject: [PATCH] responder: Use SAFEALIGN macros where appropriate.
https://fedorahosted.org/sssd/ticket/1359
src/responder/autofs/autofssrv_cmd.c | 8 +++- src/responder/common/responder_cmd.c | 20 +++++++--- src/responder/nss/nsssrv_cmd.c | 70 +++++++++++++++++++---------------- src/responder/nss/nsssrv_mmap_cache.c | 2 +- src/responder/nss/nsssrv_netgroup.c | 18 ++++++--- src/responder/nss/nsssrv_services.c | 9 +++-- 6 files changed, 80 insertions(+), 47 deletions(-)
diff --git a/src/responder/autofs/autofssrv_cmd.c b/src/responder/autofs/autofssrv_cmd.c
OK
diff --git a/src/responder/common/responder_cmd.c b/src/responder/common/responder_cmd.c index 3a3fca9..111a19c 100644 --- a/src/responder/common/responder_cmd.c +++ b/src/responder/common/responder_cmd.c @@ -51,8 +51,12 @@ int sss_cmd_empty_packet(struct sss_packet *packet) if (ret != EOK) return ret;
sss_packet_get_body(packet, &body, &blen);
- ((uint32_t *)body)[0] = 0; /* num results */
- ((uint32_t *)body)[1] = 0; /* reserved */
/* num results */
SAFEALIGN_SETMEM_UINT32(body, 0, NULL);
/* reserved */
SAFEALIGN_SETMEM_UINT32(body + sizeof(uint32_t), 0, NULL);
return EOK;
} @@ -97,6 +101,7 @@ int sss_cmd_get_version(struct cli_ctx *cctx) size_t blen; int ret; uint32_t client_version;
- uint32_t protocol_version; int i; static struct cli_protocol_version *cli_protocol_version = NULL;
@@ -133,9 +138,14 @@ int sss_cmd_get_version(struct cli_ctx *cctx) return ret; } sss_packet_get_body(cctx->creq->out, &body, &blen);
- ((uint32_t *)body)[0] = cctx->cli_protocol_version!=NULL ?
cctx->cli_protocol_version->version : 0;- DEBUG(5, ("Offered version [%d].\n", ((uint32_t *)body)[0]));
- if (cctx->cli_protocol_version != NULL) {
protocol_version = cctx->cli_protocol_version->version;- } else {
protocol_version = 0;- }
Is there special reason why you replace ternary operator with if .. else ?
SAFEALIGN_COPY_UINT32(body, &protocol_version, NULL);
DEBUG(SSSDBG_FUNC_DATA, ("Offered version [%d].\n", protocol_version));
sss_cmd_done(cctx, NULL); return EOK;
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c index 6a1e6a0..1f6ebc3 100644 --- a/src/responder/nss/nsssrv_cmd.c +++ b/src/responder/nss/nsssrv_cmd.c
OK
diff --git a/src/responder/nss/nsssrv_mmap_cache.c b/src/responder/nss/nsssrv_mmap_cache.c index 8655a1a..36110d6 100644 --- a/src/responder/nss/nsssrv_mmap_cache.c +++ b/src/responder/nss/nsssrv_mmap_cache.c
OK
diff --git a/src/responder/nss/nsssrv_netgroup.c b/src/responder/nss/nsssrv_netgroup.c index 3fc4b64..0ed0a81 100644 --- a/src/responder/nss/nsssrv_netgroup.c +++ b/src/responder/nss/nsssrv_netgroup.c @@ -680,8 +680,12 @@ static void nss_cmd_setnetgrent_done(struct tevent_req *req) }
sss_packet_get_body(packet, &body, &blen);
((uint32_t *)body)[0] = 1; /* Got some results */((uint32_t *)body)[1] = 0; /* reserved */
/* Got some results. */SAFEALIGN_SETMEM_UINT32(body, 1, NULL);/* reserved */SAFEALIGN_SETMEM_UINT32(body + sizeof(uint32_t), 0, NULL); } sss_cmd_done(cmdctx->cctx, cmdctx);@@ -842,7 +846,7 @@ static errno_t nss_cmd_getnetgrent_process(struct nss_cmd_ctx *cmdctx, if (blen != sizeof(uint32_t)) { return EINVAL; }
- num = *((uint32_t *)body);
SAFEALIGN_COPY_UINT32(&num, body, NULL);
/* create response packet */ ret = sss_packet_new(client->creq, 0,
@@ -981,8 +985,12 @@ static errno_t nss_cmd_retnetgrent(struct cli_ctx *client, }
sss_packet_get_body(packet, &body, &blen);
- ((uint32_t *)body)[0] = num; /* num results */
- ((uint32_t *)body)[1] = 0; /* reserved */
- /* num results */
- SAFEALIGN_COPY_UINT32(body, &num, NULL);
- /* reserved */
- SAFEALIGN_COPY_UINT32(body + sizeof(uint32_t), &num, NULL);
^^^^^^Even if it is reserved, it can be confusing in future why we sending num result twice.
return EOK;} diff --git a/src/responder/nss/nsssrv_services.c b/src/responder/nss/nsssrv_services.c index 390e84e..80c59e2 100644 --- a/src/responder/nss/nsssrv_services.c +++ b/src/responder/nss/nsssrv_services.c
OK
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks for the review.
Just for the record, Lukas requested one more cosmetic change in the code, to use the last parameter of the SAFEALIGN macros to increment the bindex value (in src/responder/nss/nsssrv_cmd.c) for better readability.
New patch attached.
Michal
On (31/01/14 19:02), Michal Židek wrote:
On 01/31/2014 06:12 PM, Lukas Slebodnik wrote:
On (29/01/14 15:39), Michal Židek wrote:
On 01/22/2014 07:50 PM, Lukas Slebodnik wrote:
This patch needs to be rebased.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Rebased patch attached.
Michal
From e00ca05680e8752fa9a1a3919b8c251aa79769a4 Mon Sep 17 00:00:00 2001 From: Michal Zidek mzidek@redhat.com Date: Wed, 28 Aug 2013 12:46:58 +0200 Subject: [PATCH] responder: Use SAFEALIGN macros where appropriate.
https://fedorahosted.org/sssd/ticket/1359
src/responder/autofs/autofssrv_cmd.c | 8 +++- src/responder/common/responder_cmd.c | 20 +++++++--- src/responder/nss/nsssrv_cmd.c | 70 +++++++++++++++++++---------------- src/responder/nss/nsssrv_mmap_cache.c | 2 +- src/responder/nss/nsssrv_netgroup.c | 18 ++++++--- src/responder/nss/nsssrv_services.c | 9 +++-- 6 files changed, 80 insertions(+), 47 deletions(-)
diff --git a/src/responder/autofs/autofssrv_cmd.c b/src/responder/autofs/autofssrv_cmd.c
OK
diff --git a/src/responder/common/responder_cmd.c b/src/responder/common/responder_cmd.c index 3a3fca9..111a19c 100644 --- a/src/responder/common/responder_cmd.c +++ b/src/responder/common/responder_cmd.c @@ -51,8 +51,12 @@ int sss_cmd_empty_packet(struct sss_packet *packet) if (ret != EOK) return ret;
sss_packet_get_body(packet, &body, &blen);
- ((uint32_t *)body)[0] = 0; /* num results */
- ((uint32_t *)body)[1] = 0; /* reserved */
/* num results */
SAFEALIGN_SETMEM_UINT32(body, 0, NULL);
/* reserved */
SAFEALIGN_SETMEM_UINT32(body + sizeof(uint32_t), 0, NULL);
return EOK;
} @@ -97,6 +101,7 @@ int sss_cmd_get_version(struct cli_ctx *cctx) size_t blen; int ret; uint32_t client_version;
- uint32_t protocol_version; int i; static struct cli_protocol_version *cli_protocol_version = NULL;
@@ -133,9 +138,14 @@ int sss_cmd_get_version(struct cli_ctx *cctx) return ret; } sss_packet_get_body(cctx->creq->out, &body, &blen);
- ((uint32_t *)body)[0] = cctx->cli_protocol_version!=NULL ?
cctx->cli_protocol_version->version : 0;- DEBUG(5, ("Offered version [%d].\n", ((uint32_t *)body)[0]));
- if (cctx->cli_protocol_version != NULL) {
protocol_version = cctx->cli_protocol_version->version;- } else {
protocol_version = 0;- }
Is there special reason why you replace ternary operator with if .. else ?
SAFEALIGN_COPY_UINT32(body, &protocol_version, NULL);
DEBUG(SSSDBG_FUNC_DATA, ("Offered version [%d].\n", protocol_version));
sss_cmd_done(cctx, NULL); return EOK;
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c index 6a1e6a0..1f6ebc3 100644 --- a/src/responder/nss/nsssrv_cmd.c +++ b/src/responder/nss/nsssrv_cmd.c
OK
diff --git a/src/responder/nss/nsssrv_mmap_cache.c b/src/responder/nss/nsssrv_mmap_cache.c index 8655a1a..36110d6 100644 --- a/src/responder/nss/nsssrv_mmap_cache.c +++ b/src/responder/nss/nsssrv_mmap_cache.c
OK
diff --git a/src/responder/nss/nsssrv_netgroup.c b/src/responder/nss/nsssrv_netgroup.c index 3fc4b64..0ed0a81 100644 --- a/src/responder/nss/nsssrv_netgroup.c +++ b/src/responder/nss/nsssrv_netgroup.c @@ -680,8 +680,12 @@ static void nss_cmd_setnetgrent_done(struct tevent_req *req) }
sss_packet_get_body(packet, &body, &blen);
((uint32_t *)body)[0] = 1; /* Got some results */((uint32_t *)body)[1] = 0; /* reserved */
/* Got some results. */SAFEALIGN_SETMEM_UINT32(body, 1, NULL);/* reserved */SAFEALIGN_SETMEM_UINT32(body + sizeof(uint32_t), 0, NULL); } sss_cmd_done(cmdctx->cctx, cmdctx);@@ -842,7 +846,7 @@ static errno_t nss_cmd_getnetgrent_process(struct nss_cmd_ctx *cmdctx, if (blen != sizeof(uint32_t)) { return EINVAL; }
- num = *((uint32_t *)body);
SAFEALIGN_COPY_UINT32(&num, body, NULL);
/* create response packet */ ret = sss_packet_new(client->creq, 0,
@@ -981,8 +985,12 @@ static errno_t nss_cmd_retnetgrent(struct cli_ctx *client, }
sss_packet_get_body(packet, &body, &blen);
- ((uint32_t *)body)[0] = num; /* num results */
- ((uint32_t *)body)[1] = 0; /* reserved */
- /* num results */
- SAFEALIGN_COPY_UINT32(body, &num, NULL);
- /* reserved */
- SAFEALIGN_COPY_UINT32(body + sizeof(uint32_t), &num, NULL);
^^^^^^Even if it is reserved, it can be confusing in future why we sending num result twice.
return EOK;} diff --git a/src/responder/nss/nsssrv_services.c b/src/responder/nss/nsssrv_services.c index 390e84e..80c59e2 100644 --- a/src/responder/nss/nsssrv_services.c +++ b/src/responder/nss/nsssrv_services.c
OK
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks for the review.
Just for the record, Lukas requested one more cosmetic change in the code, to use the last parameter of the SAFEALIGN macros to increment the bindex value (in src/responder/nss/nsssrv_cmd.c) for better readability.
New patch attached.
Michal
From 4030e7c0a3eca06d096f44b6ae35ffc750578a9a Mon Sep 17 00:00:00 2001 From: Michal Zidek mzidek@redhat.com Date: Wed, 28 Aug 2013 12:46:58 +0200 Subject: [PATCH] responder: Use SAFEALIGN macros where appropriate.
https://fedorahosted.org/sssd/ticket/1359
src/responder/autofs/autofssrv_cmd.c | 8 +++- src/responder/common/responder_cmd.c | 18 +++++--- src/responder/nss/nsssrv_cmd.c | 77 +++++++++++++++++++---------------- src/responder/nss/nsssrv_mmap_cache.c | 2 +- src/responder/nss/nsssrv_netgroup.c | 18 +++++--- src/responder/nss/nsssrv_services.c | 9 ++-- 6 files changed, 81 insertions(+), 51 deletions(-)
- /* 0-3: 32bit unsigned number of results
* 4-7: 32bit unsigned (reserved/padding) */- bindex = 2 * sizeof(uint32_t);
- /* skip first entry, it's the user entry */
- bindex = 0; for (i = 0; i < num; i++) { gid = ldb_msg_find_attr_as_uint64(res->msgs[i + 1], SYSDB_GIDNUM, 0); posix = ldb_msg_find_attr_as_string(res->msgs[i + 1], SYSDB_POSIX, NULL);
@@ -3571,8 +3575,7 @@ static int fill_initgr(struct sss_packet *packet, struct ldb_result *res) return EFAULT; } }
((uint32_t *)body)[2 + bindex] = gid;bindex++;
SAFEALIGN_COPY_UINT32(body + bindex, &gid, &bindex); /* do not add the GID of the original primary group is the user is * already and explicit member of the group. */@@ -3582,14 +3585,13 @@ static int fill_initgr(struct sss_packet *packet, struct ldb_result *res) }
if (orig_primary_gid != 0) {
((uint32_t *)body)[2 + bindex] = orig_primary_gid;bindex++;
SAFEALIGN_COPY_UINT32(body + bindex, &orig_primary_gid, &bindex); num++;
- ((uint32_t *)body)[0] = num-skipped; /* num results */
- ((uint32_t *)body)[1] = 0; /* reserved */
- blen = (2 + bindex) * sizeof(uint32_t);
- SAFEALIGN_SETMEM_UINT32(body, num - skipped, NULL); /* num results */
- SAFEALIGN_SETMEM_UINT32(body + sizeof(uint32_t), 0, NULL); /* reserved */
- blen = bindex;
Almost good, except few warnings (in my case errors) src/responder/nss/nsssrv_cmd.c:3578:52: error: incompatible pointer types passing 'int *' to parameter of type 'size_t *' (aka 'unsigned long *') [-Werror,-Wincompatible-pointer-types] SAFEALIGN_COPY_UINT32(body + bindex, &gid, &bindex); ^~~~~~~ src/util/util_safealign.h:79:51: note: expanded from macro 'SAFEALIGN_COPY_UINT32' safealign_memcpy(dest, src, sizeof(uint32_t), pctr) ^ src/util/util_safealign.h:49:65: note: passing argument to parameter 'counter' here safealign_memcpy(void *dest, const void *src, size_t n, size_t *counter) ^ src/responder/nss/nsssrv_cmd.c:3588:65: error: incompatible pointer types passing 'int *' to parameter of type 'size_t *' (aka 'unsigned long *') [-Werror,-Wincompatible-pointer-types] SAFEALIGN_COPY_UINT32(body + bindex, &orig_primary_gid, &bindex); ^~~~~~~ src/util/util_safealign.h:79:51: note: expanded from macro 'SAFEALIGN_COPY_UINT32' safealign_memcpy(dest, src, sizeof(uint32_t), pctr) ^ src/util/util_safealign.h:49:65: note: passing argument to parameter 'counter' here safealign_memcpy(void *dest, const void *src, size_t n, size_t *counter) ^ CC src/responder/nss/nsssrv_services.o
LS
On 01/31/2014 07:45 PM, Lukas Slebodnik wrote:
Almost good, except few warnings (in my case errors) src/responder/nss/nsssrv_cmd.c:3578:52: error: incompatible pointer types passing 'int *' to parameter of type 'size_t *' (aka 'unsigned long *') [-Werror,-Wincompatible-pointer-types] SAFEALIGN_COPY_UINT32(body + bindex, &gid, &bindex); ^~~~~~~ src/util/util_safealign.h:79:51: note: expanded from macro 'SAFEALIGN_COPY_UINT32' safealign_memcpy(dest, src, sizeof(uint32_t), pctr) ^ src/util/util_safealign.h:49:65: note: passing argument to parameter 'counter' here safealign_memcpy(void *dest, const void *src, size_t n, size_t *counter) ^ src/responder/nss/nsssrv_cmd.c:3588:65: error: incompatible pointer types passing 'int *' to parameter of type 'size_t *' (aka 'unsigned long *') [-Werror,-Wincompatible-pointer-types] SAFEALIGN_COPY_UINT32(body + bindex, &orig_primary_gid, &bindex); ^~~~~~~ src/util/util_safealign.h:79:51: note: expanded from macro 'SAFEALIGN_COPY_UINT32' safealign_memcpy(dest, src, sizeof(uint32_t), pctr) ^ src/util/util_safealign.h:49:65: note: passing argument to parameter 'counter' here safealign_memcpy(void *dest, const void *src, size_t n, size_t *counter) ^ CC src/responder/nss/nsssrv_services.o
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
New patch attached.
Michal
On (31/01/14 19:51), Michal Židek wrote:
On 01/31/2014 07:45 PM, Lukas Slebodnik wrote:
Almost good, except few warnings (in my case errors) src/responder/nss/nsssrv_cmd.c:3578:52: error: incompatible pointer types passing 'int *' to parameter of type 'size_t *' (aka 'unsigned long *') [-Werror,-Wincompatible-pointer-types] SAFEALIGN_COPY_UINT32(body + bindex, &gid, &bindex); ^~~~~~~ src/util/util_safealign.h:79:51: note: expanded from macro 'SAFEALIGN_COPY_UINT32' safealign_memcpy(dest, src, sizeof(uint32_t), pctr) ^ src/util/util_safealign.h:49:65: note: passing argument to parameter 'counter' here safealign_memcpy(void *dest, const void *src, size_t n, size_t *counter) ^ src/responder/nss/nsssrv_cmd.c:3588:65: error: incompatible pointer types passing 'int *' to parameter of type 'size_t *' (aka 'unsigned long *') [-Werror,-Wincompatible-pointer-types] SAFEALIGN_COPY_UINT32(body + bindex, &orig_primary_gid, &bindex); ^~~~~~~ src/util/util_safealign.h:79:51: note: expanded from macro 'SAFEALIGN_COPY_UINT32' safealign_memcpy(dest, src, sizeof(uint32_t), pctr) ^ src/util/util_safealign.h:49:65: note: passing argument to parameter 'counter' here safealign_memcpy(void *dest, const void *src, size_t n, size_t *counter) ^ CC src/responder/nss/nsssrv_services.o
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
New patch attached.
Michal
From 752013562af83c338344613b7278f7cc2cae6a36 Mon Sep 17 00:00:00 2001 From: Michal Zidek mzidek@redhat.com Date: Wed, 28 Aug 2013 12:46:58 +0200 Subject: [PATCH] responder: Use SAFEALIGN macros where appropriate.
https://fedorahosted.org/sssd/ticket/1359
I tested patch and works fine. Thank you for you effort to remove cast-align warnings. There are few warnings in sssd build but they are unrelated to this patch.
ACK
On Mon, Feb 03, 2014 at 11:34:21AM +0100, Lukas Slebodnik wrote:
New patch attached.
Michal
From 752013562af83c338344613b7278f7cc2cae6a36 Mon Sep 17 00:00:00 2001 From: Michal Zidek mzidek@redhat.com Date: Wed, 28 Aug 2013 12:46:58 +0200 Subject: [PATCH] responder: Use SAFEALIGN macros where appropriate.
https://fedorahosted.org/sssd/ticket/1359
I tested patch and works fine. Thank you for you effort to remove cast-align warnings.
Indeed, this must have been the longest thread in history.
There are few warnings in sssd build but they are unrelated to this patch.
ACK
Pushed to master.
Do you still plan on adding some more patches? If not, can you close the ticket?
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Michal Židek