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

Stephen Gallagher sgallagh at redhat.com
Tue Sep 8 23:28:59 UTC 2009


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

On 09/08/2009 09:19 AM, Dmitri Pal wrote:
> 
>>
>> 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.
>>
> Yes. We had a lengthy discussion about this. I opened a ticket #166 to
> address it.
> 
> 
>> 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.
> 
> Ok will do.
>>
>> 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.
>>
> Ok will do.
> 
>> Martin also had this cool idea of using something like
>> "strlen(#UINT_MAX)" during configure to get precise values.
>>
> Can you please explain how I can use it in configure?
> Looks cool but not sure how to do it in configure.
> 
>> 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.
>>
> Can you please elaborate? Are you saying that the buffer should be
> unsigned char
> or I am misreading your statement?
> 
>> 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
> I agree but it is hard to do it in the first pass.
> I opened a ticket (#169) to refactor the code when all the cases are
> implemented.
> 
>>
>> 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".
>>
> Jakub, can you please take a look at this function again?
> Was I smoking something? Looking at it again it seems that I am
> allocating shorter buffers when there is a qualifier for numeric types
> and doubles.
> I definitely need to redo this function.
> 
>> 9) Nitpicking: using "len" and "length" variables in the same function
>> seems confusing
>>
> Ok I will come with something better
>> 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
>>
> Ok
> 
>> 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?
> Do not see a big advantage in this case. I considered strtol but chose
> not to use it.
> 
>>
>> 12) Nitpick: bad indenting on file_provider:314 makes the logic harder
>> to read
>>
> Sure will fix.
> 
>> 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 do not use asprintf() for compatibility reasons.
> 
>> 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())
>>
> (Deep deep sigh) I started programming in C in 1989.
> Since then all my C programs were "goto free".
> I prefer to keep it this way.
> 
> 
> I will send an additional patch with fixes later today.
> Thanks for review.
> 
> Dmitri
> 
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel

Pushed to master with the fixes from the code review of "Correcting
Issues" squashed in.

- -- 
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/

iEYEARECAAYFAkqm6LsACgkQeiVVYja6o6MNJwCfXl3MU1QhvYiIJxHr0HpXNHZ5
aFcAoKI7k4OUG7jpzlDnVqOtkuu1PgR+
=YjVM
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list