On Mon, Aug 26, 2013 at 05:28:48PM +0200, Michal Židek wrote:
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
I like the new names.
ACK.