[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