[SSSD] [PATCH 4/7] responder: Use SAFEALIGN macros where appropriate.

Michal Židek mzidek at redhat.com
Thu Nov 28 12:04:08 UTC 2013


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 at 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



More information about the sssd-devel mailing list