Dmitri Pal wrote:
Jakub Hrozek wrote:
> On 08/10/2009 08:27 PM, Stephen Gallagher wrote:
>
>> Ack. Waiting for a reply from Jakub as well before pushing these
>>
> patches.
>
> Patch 0001 (INI fix): ACK
>
> Patch 0002 (ELAPI): I have some comments and questions. Most of them are
> just style comments, but I think at least 2) and 8) should be fixed
> before we push the patch.
>
> The code builds cleanly, passes all its unit tests. I have not tested
> functionality any further.
>
> 1) what is ERR_FILE=@elapierrfile@ in Makefile.am? It appears to be some
> kind of leftover, it is not set by configure or used afterwards.
>
>
It was. Removed.
> 2) conf_macros.m4: line 25, seems like a cut-n-paste error, I
think it
> should set elapiconfappdir=
>
>
You are right. Corrected.
> 3) conf_macros.m4: the default values are usually given in form
of
> [value], the file sometime uses [key=value], sometime [default=value]
> and sometimes [value]. It would be nice to stick to one standard.
>
>
Cleaned to be [value].
> 4) In general (this is maybe question also for the others not a
review
> item), is there a reason for passing the macros containing
> configure-time variables as -DMACRO instead of using AC_DEFINE_UNQUOTED
> and pulling them out of config.h? Is is just a matter of style?
>
>
Following Simo's comment did not do anything with it.
> 5) I see that get_default_template was moved and is now
non-static. But
> at the same time, it is not declared in any header. Is it missing from
> the headers or should it be static as previously?
>
>
Exposed it in the header elapi_event.h.
> 6) similar issue, just not strictly related to this patch, but
in
> elapi_event.c, the "undefined", "str_yes" etc. are not declared
as
> static but not used outside elapi_event.c
>
>
Those will probably be translatable strings so it will be handled later
when I add language support.
I opened a ticket not to forget.
https://fedorahosted.org/sssd/ticket/98
> 7) elapi_log.c - does it make sense for variables such as
> default_config_file to be declared "static const char *"? Or perhaps
> even #defined? Also the variable elapi_close_registered could be made
> static
>
>
Added "static" to those.
> 8) in elapi_log.c, the check for handle->appname == NULL on
line 228
> should probably be moved before the TRACE_FLOW_STRING on line 225 as the
> trace macro tries to access the memory in handle->appname. Also maybe
> it's OK to just return ENOMEM and don't bother with errno.
>
>
As for errno it is a valid way of handling error so I guess no change
needed.
I prefer errno, I can't explain why...
Added ticket to address it in a separate patch since fix should be in
the macro not in the ELAPI code.
https://fedorahosted.org/sssd/ticket/99
> 9) why the (void) in elapi_log.c:332? Is it to make clear that we
don't
> care about the return value from col_traverse_collection?
>
>
This is a cleanup, we are destroying object. It is a best effort.
Returning error there is like allowing free() to return an error.
What would you do with such an error?
This is why it is ignored.
> 10) elapi_internal.c:elapi_internal_dump_errors_to_file - maybe
the
> strftime should use E_TIMESTAMP_FORMAT instead of hardcoded "%F" to be
> consistent
>
>
Done.
> Jakub
>
>
>
>
Thank you for the review. I will update and resubmit.
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
Thanks for review!
Patches attached.
--
Thank you,
Dmitri Pal
Engineering Manager IPA project,
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/