On Wed, 2013-04-10 at 11:40 +0200, Jakub Hrozek wrote:
On Wed, Apr 10, 2013 at 08:35:51AM +0200, Ondra Hujňák wrote:
> Hi,
>
> I'm about to develop new provider for RADIUS protocol. I just made some stub and
wanted to ask you guys to check the coding style.
>
> My work is available at github repo
https://github.com/hujon/sssd.git in branch rad.
>
> So far it's just a really preliminary review which lacks support for RADIUS. I
just wanted to know if I'm heading the right way.
>
> Ondrej Hujnak
Hi Ondra,
the stub looks OK, I just have a couple of style comments so far
(below).
But most importantly, do you have some kind of design document?
Anything that describes how the provider should behave and what the
functionality will be like? For instance I don't know which PAM tasks
are supported by RADIUS so I can't say if all the actions in
get_rad_ctx() should be supported or not.
As per the style:
* Don't mix tabs and spaces. Follow the coding guidelines on
http://www.freeipa.org/page/Coding_Style
* Put a space after keywords ("if (" not "if(") and no space after
* opening "(".
* Using the new SSSDBG_* DEBUG levels is preferred over the old decimal
levels
* Please put a "copyright blob" on top of every new file. You can copy
the blobs from other files but please check with your advisor who
holds the copyright since you're doing the project as a thesis -- is
it you or the university?
Also we have recently started to use new SSSD-specific error codes in
favor of errno. You can take a look at commits
233a3c6c48972b177e60d6ef4cecfacd3cf31659 or
dfd71fc92db940b2892cc996911cec03d7b6c52b
I would strongly prefer if the new provider used the new error codes.
When this feature is accepted upstream, it would be an experimental
feature I think. Can you modify the autoconf scripts so that this new
provider is only built when --enable-all-experimental-features is used?
Check out commit b2f9e5b7d553172401a340eb4a9c3abda6b5db43 to see how a
feature can be marked experimental.
Also please always use 4 spaces indentation and do not indent 'case' in
'switch' statements. Do not use C++ style comments (// comment).
Try not to create too many header files, you do not need one header file
for each .c file unless you really need to keep them separate.
Usually for small modules I would create at most a radius_common.h and a
radius_private.h files and the latter only if you really need a
differentiation.
Simo.
--
Simo Sorce * Red Hat, Inc * New York