[SSSD] [PATCH] Rename SAFEALIGN macros

Michal Židek mzidek at redhat.com
Mon Aug 26 15:28:48 UTC 2013


On 08/26/2013 04:07 PM, Jakub Hrozek wrote:
> On Mon, Aug 26, 2013 at 03:57:49PM +0200, Michal Židek wrote:
>> On 08/26/2013 03:28 PM, Simo Sorce wrote:
>>> On Mon, 2013-08-26 at 14:48 +0200, Michal Židek wrote:
>>>> Hi,
>>>>
>>>> about 3 months ago I renamed the SAFEALIGN macros (and added aliases
>>>> for backward compatibility) from
>>>>
>>>> SAFEALIGN_COPY_<type>
>>>> SAFEALIGN_SET_<type>
>>>>
>>>> to
>>>>
>>>> SAFEALIGN_BUF2VAR_<type>
>>>> SAFEALIGN_VAR2BUF_<type>
>>>>
>>>> according to request in ticket
>>>> https://fedorahosted.org/sssd/ticket/1772
>>>>
>>>> But, now I think I used wrong new names for these macros, because the
>>>> SAFEALIGN_BUF2VAR_<type> is not meant to only copy value from buffer to
>>>> variable, it can be used the other way around as well (it is basically
>>>> just memcpy with bytes counting). So the original SAFEALIGN_COPY_<> was
>>>> IMO much better name for the macro.
>>>>
>>>> The second macro SAFEALIGN_VAR2BUF_<type> is used to SET the buffer at
>>>> given address with specified value. This can be used if the
>>>> value we copy to buffer is just expression (like 3 + 5), #DEFINEd
>>>> constant or literal (so we can not get its address with &, so
>>>> SAFEALIGN_COPY_<type> can not be used). The original name of this macro
>>>> was
>>>> SAFEALIGN_SET_<type>
>>>> which was better than the new one. I wrongly interpreted the macros when
>>>> creating the new names and realized it now when I wanted to use them.
>>>>
>>>> The reason why the name change was requested is most likely because
>>>> SAFEALIGN_SET_UINT32
>>>> might look like it should be used when setting uint32_t variable to
>>>> a specific value taken from buffer. The parameter 'value' is
>>>> expression of type uint32_t and not address to some uint32_t value
>>>> in buffer. Another reason why people might have been confused with the
>>>> old names was that it was not clear that with SAFEALIGN_SET the 'value'
>>>> is not a pointer to value but the value itself. I think this was the
>>>> reason why the ticket was created.
>>>>
>>>> So, I would prefer to change the names once again (the new ones are not
>>>> used anywhere so far) to the original ones with one little difference.
>>>>
>>>> The SAFEALIGN_SET_<type> will be SAFEALIGN_SETMEM_<type>. It makes more
>>>> clear that we want to change memory at given address (and not set some
>>>> variable of type <type>).
>>>
>>> If you want to use something familiar then use 'MEMSET' just like the
>>> memset(3) function. Although I am not sure that's what you wants.
>>>
>>> Maybe what you really want is SAFEALIGN_ASSIGN() to mean you are
>>> assigning the variable a value.
>>>
>>>> With additional comments showing the expected types of all arguments,
>>>> this should be clear enough and hopefully nobody will be confused in
>>>> the future.
>>>>
>>>> Patch is attached.
>>>
>>>
>>> +/* SAFEALIGN_SETMEM_INT64(void *dest, int64_t value, size_t *pctr)
>>> + * Will store store given value to memory at address pointed by dest.
>>> + * If pctr is not NULL, the value of *pctr will be raised by the
>>> + * number of stored bytes (in this case sizeof(int64_t)). */
>>>
>>> I would reword this as follow (not doesn't change just the name,
>>> corrects also the language used).
>>>
>>> /* SAFEALIGN_ASSIGN_INT64(void *dest, int64_t value, size_t *pctr)
>>>   * This macro will safely assign an int64 value to the memory
>>>   * location pointed by 'dest'. If the pctr pointer is not NULL, its
>>>   * value is incremented by sizeof(int64_t). */
>> s/its value/value pointed by 'pctr'
>> because it is not the pointer that is incremented. But other than that
>> your version is much more readable, so thanks for correction :-)
>>
>> I do not have strong opinion on this, but doesn't
>> SAFEALIGN_ASSIGN_INT64
>> involve the same problem as the original
>> SAFEALIGN_SET_INT64 ?
>>
>> The problem was that it looked like you are setting a value
>> to uint64_t variable, now it looks like you are ASSIGNing a
>> value to int64_t variable (or at least the direction is not clear
>> from the name). With the SETMEM it is more explicit that you are
>> modifying the memory at specific address (I originally wanted to
>> use MEMSET_INT64, but it looked like memsetting int64 variable as
>> well, so I choose SETMEM).
>
> I agree with Michal that we don't want to repeat the problem with the
> old names. The confustion on what is being set or copied led to very
> hard to find bugs..

New patch is attached.

Michal

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Rename-SAFEALIGN-macros.patch
Type: text/x-patch
Size: 5793 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130826/b1a6acab/attachment.bin>


More information about the sssd-devel mailing list