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

Stephen Gallagher sgallagh at redhat.com
Tue Sep 8 10:45:23 UTC 2009


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

On 09/07/2009 07:38 AM, Jakub Hrozek wrote:
> 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.

We're avoiding asprintf except for when using talloc_asprintf() because
only the latter is portable.

So I don't see a problem with this approach.

> 
> 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())
> 
_______________________________________________
sssd-devel mailing list
sssd-devel at lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel

- -- 
Stephen Gallagher
RHCE 804006346421761

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkqmNb4ACgkQeiVVYja6o6NQwACfYsU6v3V2fb9siDMcUvRsOM7f
YdoAoIkCydWp+VhY6QTYEK1UB88JwkDZ
=dnzK
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list