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(a)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(a)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(a)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