-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 08/17/2009 04:48 PM, Dmitri Pal wrote:
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.
I can live with a separate patch for names.
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.
Sorry, I should have mentioned that was not a review comment, merely a
recommendation on a question I saw in the comments.
> 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?
With automake, there are two variables that are automatically populated:
$(srcdir) and $(builddir). These refer to the original path of the
source files and to the output path of the built files. If you are
building in-tree, $(srcdir) == $(builddir). However, in order to be
fully GNU-compliant, we also support building in what are referred to as
parallel build trees. This means that you run configure from a directory
outside the normal source tree, and all configure output, compiler
output and completed binaries are built into this new build tree instead
of in the original source tree. This is done for two reasons: 1) During
development, it's easier to ensure that you don't accidentally commit
generated files alongside code, because all generated files (after
autoreconf, anyway) are built in a different directory. 2) Many release
engineering systems will use a single source checkout to build multiple
releases. They may mount the source checkout by NFS on several different
machines and build them from different architectures.
An illustration (e.g. How Steve Builds the SSSD):
I check out the latest code into the 'sssd' directory.
cd sssd
mkdir x86_64
cd x86_64
../configure <configure arguments>
Notice the ../configure (instead of ./configure). The output from
configure will generate a new build tree inside the x86_64 subdirectory.
This directory will have its own set of common, replace, server and
sss_client directories, each with the Makefile output by configure. Now
I can run 'make' from within the x86_64 directory and it will build the
temporary objects, final objects, manpages and scripts in the x86_64
tree instead of the main source tree.
So in this example, if I was building the elapi_test, my $(srcdir)
variable would be /sssd/common/elapi/elapi_test and my $(builddir)
variable would be /sssd/x86_64/common/elapi/elapi_test.
So, in order to link against a built library, my link path needs to be
relative to the $(builddir) and not the $(srcdir). But if I want to add
an include path, it needs to use the $(srcdir) variable so that it can
find it in the original source path.
The easiest way I've found to make sure it's all working is to always
perform parallel builds and never build in-tree. Plus, it makes cleanup
a snap (rm -Rf x86_64 is more reliable than 'make clean' or 'make
distclean' if you've been playing around with the contents of Makefile.am)
> 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
- --
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/
iEYEARECAAYFAkqKnDwACgkQeiVVYja6o6PyRwCgkDmleHecHf0CTJXyMFLaKQII
jzMAnRakn5EmPql14mcaQOheP/u5apTE
=R4Ac
-----END PGP SIGNATURE-----