On (15/11/13 17:22), Michal Židek wrote:
On 11/14/2013 01:14 PM, Lukas Slebodnik wrote:> On (05/11/13
15:18),
Michal Židek wrote:
>>>>>Thanks for the responses.
>>>>>
>>>>>I decided to reduce the patches for this thread to only
>>>>>similar or simple changes. I think it is still quite enough
>>>>>for a review. I will post patches for the rest of the issues
>>>>>(like the one with
sockaddr) in
>>>>>separate threads. I hope this will make review easier. So
>>>>>after
applying
>>>>>these patches there will still be a lot of warnings.
>>>>>
>>>>>New patches attached.
>>>>>
>>>>>PS: Sorry for delayed answer.
>>>>>
>>>>>
>>>
>>>Sending patches rebased on top of current master.
>>>
>>>Michal
>>>
>>From 93a3b127e24ed26068ca7cfea41992a86440d8c2 Mon Sep 17 00:00:00
>>2001
>>>From: Michal Zidek<mzidek(a)redhat.com> Date: Wed, 21 Aug 2013
>>>17:17:06 +0200 Subject: [PATCH 1/7] sss_client: Use
>>>SAFEALIGN_COPY_<type> macros where diff --git
>>>a/src/sss_client/idmap/sss_nss_idmap.c
b/src/sss_client/idmap/sss_nss_idmap.c
>>>index e0faf6e..69f825f 100644 ---
>>>a/src/sss_client/idmap/sss_nss_idmap.c +++
>>>b/src/sss_client/idmap/sss_nss_idmap.c @@ -108,7 +108,7 @@ static
>>>int sss_nss_getyyybyxxx(union input inp,
enum sss_cli_command cmd ,
>>>goto done; }
>>>
>>>- num_results = ((uint32_t *)repbuf)[0]; +
>>>SAFEALIGN_COPY_UINT32(&num_results, repbuf, NULL); if
>>>(num_results == 0) { ret = ENOENT; goto done; @@ -117,7 +117,7 @@
>>>static int sss_nss_getyyybyxxx(union input inp,
enum sss_cli_command cmd ,
>>>goto done; }
>>>
>Please add comment why comment why you skipped one unit32_t There is
>already similar comment on the other place in this patch. /* Skip
>first two 32 bit values (number of results and * reserved padding)
>*/
Added.
Thank you
>
>>>- out->type = ((uint32_t *)repbuf)[2]; +
>>>SAFEALIGN_COPY_UINT32(&out->type, repbuf + 2 *
sizeof(uint32_t), NULL);
>>>
>>>data_len = replen - DATA_START;
>>>
>>>diff --git a/src/sss_client/nss_group.c
>>>b/src/sss_client/nss_group.c index e6ea54b..a7fb093 100644 ---
>>>a/src/sss_client/nss_group.c +++ b/src/sss_client/nss_group.c @@
>>>-298,7 +298,7 @@ enum nss_status _nss_sss_initgroups_dyn(const
char *user, gid_t group,
>>>}
>>>
>>>/* no results if not found */ - num_ret = ((uint32_t
>>>*)repbuf)[0]; + SAFEALIGN_COPY_UINT32(&num_ret, repbuf,
>>>NULL); if (num_ret == 0) { free(repbuf); nret =
>>>NSS_STATUS_NOTFOUND; @@ -328,9 +328,13 @@ enum nss_status
>>>_nss_sss_initgroups_dyn(const
char *user, gid_t group,
>>>*size = newsize; }
>>>
>The same issue is here.
>>>- rbuf = &((uint32_t *)repbuf)[2]; + /* Skip first two 32
>>>bit values (number of results and + * reserved padding) */
^^^^^^^^^^^^^^^^^^^^^
The comment was there. Maybe I missed something?
>>>+ buf_index = 2 * sizeof(uint32_t); + for (l = 0; l < max_ret;
>>>l++) { - (*groups)[*start] = rbuf[l]; +
>>>SAFEALIGN_COPY_UINT32(&(*groups)[*start], repbuf + buf_index, +
>>>&buf_index); *start += 1; }
>>>
>Otherwise, conditions look much nicer with variables like a
>"num_results" instead of "buffer magic" (((uint32_t *)repbuf)[0]
==
>0)
New patch attached.
Michal
From b486261db01893b60d3ccd8bd871a93747eaaf49 Mon Sep 17 00:00:00
2001
From: Michal Zidek <mzidek(a)redhat.com>
Date: Wed, 21 Aug 2013 17:17:06 +0200
Subject: [PATCH 1/7] sss_client: Use SAFEALIGN_COPY_<type> macros where
appropriate.
ACK
LS