[SSSD] [PATCH] Rename SAFEALIGN macros

Jakub Hrozek jhrozek at redhat.com
Wed Sep 4 15:55:40 UTC 2013


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.



More information about the sssd-devel mailing list