[SSSD] [PATCH] Rename SAFEALIGN macros

Simo Sorce simo at redhat.com
Mon Aug 26 13:28:12 UTC 2013


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). */

HTH,
Simo.

P.S: should we start using docbook syntax for this type of comments?
I use them elsewhere and it make it possible to create documentation
quite easily.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list