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

Dmitri Pal dpal at redhat.com
Tue Sep 8 13:19:32 UTC 2009


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




More information about the sssd-devel mailing list