[SSSD] [PATCH] responder: Access packet header using SAFEALIGN macros

Michal Židek mzidek at redhat.com
Tue Nov 5 13:33:03 UTC 2013


On 11/05/2013 02:06 PM, Simo Sorce wrote:
> On Tue, 2013-11-05 at 08:04 -0500, Simo Sorce wrote:
>> The patch looks ok to me as is, however some of the function have become
>> verbose because you used the raw copying macros.
>>
>> What about having setters and getters for cmd and status too ?
>>
>> Then sss_packet_get_cmd() becomes simply:
>> {
>>      return (enum sss_cli_command)sss_packet_get_cmd(packet);
>> }
>> which more closely matches the current code.
>
> ROTFL, and this is the classic example of rewriting your email while
> reading the code and not proof reading it at the end :-)
>
> Ignore this part, just look at the next bit.
>
>> Also minor nack, the new functions you introduced are not namespaced.
>> Please packet_get_len() -> sss_packet_get_len() and so on.
>>
>> I know these are static for now, but let's keep a naming consistent with
>> sss_packet_set_error() and sss_packet_get_body() etc ...
>>

I was thinking about whether to put those static functions to namespace 
or not. I decided not to do it in the end, but yes, it will probably be 
better if the static getters and setters share the same naming pattern
with non static ones.

So new patch is attached (it is the same patch as before, just the 
function names are changed).

Michal

>> Simo.
>>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-responder-Access-packet-header-using-SAFEALIGN-macro.patch
Type: text/x-patch
Size: 7071 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131105/77563af1/attachment.bin>


More information about the sssd-devel mailing list