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
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 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
It's nice to see the attribution retained to the original author of the code. Credit where credit is due.
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.
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)
It's nice to see the attribution retained to the original author of the code. Credit where credit is due.
/me nods
Simo.
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.
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
On Mon, Apr 29, 2013 at 02:36:40PM +0200, Michal Židek wrote:
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
Ack.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Michal Židek -
Simo Sorce