[SSSD] [PATCH] ELAPI Adding file provider and CSV format

Jakub Hrozek jhrozek at redhat.com
Mon Sep 7 11:38:36 UTC 2009


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/03/2009 02:27 AM, Dmitri Pal wrote:
> Hi,
> 
> This patch is next significant chunk of code.
> See patch comments for the main description.
> It requires previous patch that has been reviewed but was not pushed yet.
> It addresses some of the suggestions risen during the review.
> I also added couple tickets: to eventually remove all the "Fixme"
> comments and to refactor places where I allocated context structs and
> initialize them.
> 
> Checked with make check, valgrind and did in tree and parallel builds.
> All works, no leaks.
> 
> Sorry that it is too big.
> I need to move on and there are many things that are still missing.
> I will start working on  the next set of the functionality tomorrow
> mainly adding async processing. That would definitely affect some of the
> arguments passed all along so expect changes to many modules.
> 
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel

Sorry for the belated review, the patch is big and took me some time to
read and digest.

My comments:

1) elapi_basic.h:40
#define SEDO struct elapi_data_out

I personally don't like this, this seems like black magic. Why not just
use the struct name? I know it's a little long to type, but that's what
code completion is for.

2) elapi_grow_data()
I think it makes sense to rename the "tmp" variable to something like
"newbuf" and change its type to "char *" since the out_data->buffer is a
char array anyway.

3) file_csv_data_len
I do not like the magic constants like "20" and such in the switch. I
would prefer #defining them as named constants.

Martin also had this cool idea of using something like
"strlen(#UINT_MAX)" during configure to get precise values.

4) struct file_csv_cfg
Is there a reason for defining the qualifier, separator etc. explicitly
as "unsigned char"? Since the buffer is "plain char" which can be either
signed or unsigned depending on implementation, you can compare
different types, no? I think just explicitly casting to unsigned char
would help.

5) file_print_fmt_cfg() and file_read_fmt_cfg()
I generally prefer {constant, function} tuples over big switches, but
that's just a matter of style I guess

6) file_csv_data_len() and file_serialize_csv()
There seems to be an off-by-one error, for bools, the size is 3 (I
assume this is for string "yes"), but I think it should be 4 to account
the trailing '\0'. Also for bool values, I think "true/false" is more
generally understood than "yes/no".

9) Nitpicking: using "len" and "length" variables in the same function
seems confusing

10) file_get_csv_cfg()
Rather than using magic constants like cfg->csvseparator = ','; I think
it's better to define them into something like DFL_CSV_SEPARATOR

11) file_build_set()
Maybe it would be less error prone to convert the mode_leftovers to int
using something like strtol instead of doing it manually?

12) Nitpick: bad indenting on file_provider:314 makes the logic harder
to read

13) file_read_cfg()
This seems ugly to me:
file_cfg->filename = malloc(strlen(appname) + sizeof(FILE_SUFFIX));
strcpy(file_cfg->filename, appname);
strcat(file_cfg->filename, FILE_SUFFIX);

It left me wondering if it's a off-by-one error until I realized
sizeof(FILE_SUFFIX) accounts the terminating null byte.

I think it's way better to use:
asprintf(&file_cfg->filename, "%s%s", appname, FILE_SUFFIX);
But asprintf it's a GNU extenstion, not sure if it matters for you.

14) General comment - maybe instead of freeing individual components
after an operation failed, it is less error prone to "goto fail" and
free everything on one place? (I noticed this in file_split_by_set())

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkqk8LwACgkQHsardTLnvCVUKwCgiK18/8m/sNqt+7CW8leWg/3B
uIMAn1Py/5EHJEvpDKYV70avMVNvWawI
=orr6
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list