[SSSD] [PATCH] Rename SAFEALIGN macros

Michal Židek mzidek at redhat.com
Mon Apr 29 12:36:40 UTC 2013


On 04/26/2013 10:21 AM, Jakub Hrozek wrote:
> On Thu, Apr 25, 2013 at 07:17:20PM -0400, Simo Sorce wrote:
>> On Fri, 2013-04-26 at 00:51 +0200, Jakub Hrozek wrote:
>>> On Thu, Apr 25, 2013 at 03:41:28PM +0200, Michal Židek wrote:
>>>> https://fedorahosted.org/sssd/ticket/1772
>>>>
>>>> Changes done:
>>>> - definitions of safealign macros have been removed from
>>>> src/sss_client/sss_cli.h and src/util/util.h and put into new header
>>>> (the old headers include this new header). This change was done to
>>>> avoid code duplication.
>>>>
>>>> - Macros that copy bytes from a variable to byte buffer have been
>>>> renamed from SAFEALIGN_SET_<type> to SAFEALIGN_VAR2BUF_<type>.
>>>>
>>>> - Macros that copy bytes from a byte buffer to a variable have been
>>>> renamed from SAFEALIGN_COPY_<type> to SAFEALIGN_BUF2VAR_<type>
>>>>
>>>> - Aliases have been added to allow the old names to be used in the
>>>> code (we can remove the aliases when the old names are replaced with
>>>> the new ones on all places in the code, but for now, it is good to
>>>> allow both alternatives, so that this patch can be smaller)
>>>>
>>>> Thanks
>>>> Michal
>>>
>>> You need to add the new header to dist_noinst_HEADERS in Makefile.am
>>> otherwise you break make distcheck.
>>>
>>> Also, I was thinking about how we create the headers for consumption in
>>> both server and the client -- would it be a good idea to differentiate
>>> them on the filesystem level so that it's immediatelly clear that this
>>> header is special?
>>
>> I agree this is important.
>> The header was put in sss_client to make crystal clear this header need
>> to stay slim and can't grow dependencies and cruft as the client library
>> needs to stay minimal.
>>
>
> I think the code is minimal, it's just macros.
>
>> In moving this header we need a way to maintain this notion.
>>
>>> I see two options:
>>>      a) create a new subdirectory, something like src/shared (better name
>>>      welcome) that would contain the code shared between server and client
>>>      b) use some kind of prefix in the filename such as cs_header.h
>>
>> We already have another header in util/ in the same situation.
>> At the time we opted for adding a prominent comment in the header file
>> (option c).
>> We can do the same here, or we can decide to go with a)
>
> That's what Michal did, the comment was there.
>
> The only thing I was afraid of was that someone would navigate with ctags
> into middle of the file and wouldn't see the comment on top. Then again,
> this is something a code review should catch, too.

I agree. I filed a ticket to track this issue:
https://fedorahosted.org/sssd/ticket/1898

For now I would go with the warning message at the beginning of the 
header. The above ticket can be fixed later for all shared headers at once.

New patch attached.

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


More information about the sssd-devel mailing list