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 <mzi...@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 <mzi...@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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
