On Tue, Nov 05, 2013 at 02:33:03PM +0100, Michal Židek wrote:
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.
>>
Simo's concern was addressed, the functions are now namespaced. Patch
works fine (getgrnam, initgroups, login, getpwnam).
ACK