Hi Steven,
Thanks for review!
Comments and questions below.
Stephen Gallagher wrote:
On 08/14/2009 04:50 PM, Dmitri Pal wrote:
> See the patch comment.
> The wiki page will be really helpful during review:
>
https://fedorahosted.org/sssd/wiki/WikiPage/ELAPIInterface
> ------------------------------------------------------------------------
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
>
https://fedorahosted.org/mailman/listinfo/sssd-devel
Nack
Please use a shorter struct name than "elapi_target_pass_in_data".
Keeping line lengths to < 80 characters gets difficult.
struct elapi_recv_data or something similar would be sufficient.
Similarly, _ctx is preferred in the SSSD over _context (such as
elapi_target_ctx instead of elapi_target_context)
elapi_internal_target_cleanup_handler() would be better as
elapi_target_free()
I will see what I can do about the names.
I think that it would make sense to have a separate patch that deals
with names then if you are concerned with things getting too long.
I have my own pattern of naming. If it does not fly I need to change the
pattern.
Here is what I can do:
a) Remove the word internal at all from the internal functions. May be
it is a not so great idea to have it in the first place
b) Replace context with "ctx"
c) Shorten or abbreviate names of the functions and structures to make
them shorter but still readable.
I think it should be a separate "naming correction patch". I can add it
to this one but it might be harder for you to review.
Your call. I do not have a preference but in the wake of 0.5 release I
do not want to create addition work for you.
Since this code is not called by anyone yet there is no harm to log a
ticket to align naming and move on.
Typo in elapi_internal_add_sink: "Sink refernce before
call" should be
"Sink reference before call"
Will fix that one.
In elapi_internal_create_target, you make reference to relaxing the
checks for adding the sinks and being satisfied with at least one sink
in the list. I think a safer approach would be to define sinks with an
attribute of "Mandatory" and "Preferred". If a sink was mandatory
and
failed to load, it should be an error condition. If a sink was Preferred
and failed to load, the ELAPI should continue on. If all sinks were
"Preferred" and none loaded, this should also be an error condition.
May be, but I am not there yet. One of the issues is that a sink can be
mandatory for one target and optional for another.
I was thinking about this a lot and do not have a good approach yet.
I need to think more about it, this is why there are placeholders in the
code in the areas related to loading sinks.
EALPI_EVT_* macros were probably meant to be ELAPI_EVT_*
Typo - will fix.
When creating a new directory, you also need to git add a new m4
directory (I usually create the empty file m4/.dir to accomplish this)
I thought I did. I definitely created it and thought that I added it
commit.
I do not know what happened. I will try again.
Your unit tests do not build in a parallel build tree.
What do
you mean they do not build in parallel build tree?
The elapi_test is supposed to be built after ELAPI.
You need to make
sure that you use the $(srcdir) and $(builddir) variables correctly.
My library
source is one level above test and this is what I refer to.
This is why when I build a special instance of the libelapi for testing
(being inside elapi_test directory) I reference most of the code as "../"
This all builds and works for me correctly.
Any
path that needs to be relative to the original sources must use
$(srcdir) and any path that needs to be relative to built binaries or
generated scripts needs to use $(builddir)
Yes and this is what happens. My builddir for the test is
elapi_test and $(source) is the same but most of the source code that I
need to rebuild is actually a level above.
I do not see what is wrong and what you want me to change?
So, you need to ensure that your elapi_ut_LDADD entries use
$(builddir)
to find the libraries to link against,
It does.
and you need to ensure that the
-D macros you're passing to the compiler refer to $(srcdir).
As I said the
$(source) is elapi_test when a special instance of the
library is built but the code is one level above.
The
AM_CPPFLAGS are correctly using $(srcdir) as-is.
Thank you for review.
a) Please explain what you want me to do with naming (ticket + sep patch
or jam in this one)
b) Please explain what you want me to do with build. So I do not
understand what is wrong.
I plan to fix:
1) Typo in "reference"
2) Fix macro names
3) Add .dir to commit
4) Anything that comes from your follow up comments about items a) and
b) above.
Thanks
Dmitri
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
--
Thank you,
Dmitri Pal
Engineering Manager IPA project,
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/