Greetings, my name is David Bambusek, and I am working on an application , that will be used as a tool for querying SSSD database. I am curently a student of FIT VUT Brno and this is topic of my bachelor thesis. It will be a command line application able to query various types of data, that is stored in SSSD database. I made a web page, where I will add information about this application I named sss_query, so far, you can find there information about basic user interface ( www.bambusekd.cz/sss_query.html ).
My project is on my Github account https://github.com/bambusekd/SSSD.git there I created branch sss_query, where I work on my application
I will be glad for any advice, recommendation or comment to it.
Thanks
David Bambusek
On Mon, Feb 25, 2013 at 11:11:01AM +0100, David Bambušek wrote:
Greetings, my name is David Bambusek, and I am working on an application , that will be used as a tool for querying SSSD database. I am curently a student of FIT VUT Brno and this is topic of my bachelor thesis. It will be a command line application able to query various types of data, that is stored in SSSD database. I made a web page, where I will add information about this application I named sss_query, so far, you can find there information about basic user interface ( www.bambusekd.cz/sss_query.html ).
My project is on my Github account https://github.com/bambusekd/SSSD.git there I created branch sss_query, where I work on my application
I will be glad for any advice, recommendation or comment to it.
Thanks
David Bambusek
Hi,
I have some questions and comments related to design:
1) I suggest to read the thread at: https://lists.fedorahosted.org/pipermail/sssd-devel/2012-February/008712.htm... if you haven't seen it already.
2) Have you seen samba's wbinfo tool? Maybe in your thesis you could compare the two.
3) The list of objects to operate on is incomplete. You also need to work with sudo rules, SSH hosts and I would also strongly suggest including domains and subdomains. (for domains, include info like sysdb domain version for instance). There should also be a way to perform unkeyed search (=all objects of type X).
4) Is the output always going to include all attributes or were you going to have a "human readable" and "raw" output? What about group memberships? Are there any plans to merge functionality with the sss_groupshow tool ?
5) There are a couple of tickets in the SSSD trac related to this effort. Depending on the scope of your thesis you might want to assign them to yourself.
The related tickets are: https://fedorahosted.org/sssd/ticket/1220 https://fedorahosted.org/sssd/ticket/1221 https://fedorahosted.org/sssd/ticket/1222 https://fedorahosted.org/sssd/ticket/1223
Feel free to also create another ones.
6) Please also include plans for unit tests in your design and implementation.
Thank you for your comments.
ad. 1. and 2. - I will look at those, thanks
ad. 3. - this is just my first minimalistic version, I will add the missing objects and more options to search soon, my main reason for posting the code was to get some feedback on my coding style and general design of application
ad. 4. - I have already implemented an option, where user can chose which attributes he wants to see on the output( the -S option). Merging sss_groupshow tool was not in my plan, but after I am done with basic functionallity, I could implement some cooperation between these two tools.
ad. 5. - I will asign those tickets to myself
ad. 6. - I will continuously add tests as I implement new functionality
David Bambušek
Dne 25.2.2013 11:50, Jakub Hrozek napsal(a):
On Mon, Feb 25, 2013 at 11:11:01AM +0100, David Bambušek wrote:
Greetings, my name is David Bambusek, and I am working on an application , that will be used as a tool for querying SSSD database. I am curently a student of FIT VUT Brno and this is topic of my bachelor thesis. It will be a command line application able to query various types of data, that is stored in SSSD database. I made a web page, where I will add information about this application I named sss_query, so far, you can find there information about basic user interface ( www.bambusekd.cz/sss_query.html ).
My project is on my Github account https://github.com/bambusekd/SSSD.git there I created branch sss_query, where I work on my application
I will be glad for any advice, recommendation or comment to it.
Thanks
David Bambusek
Hi,
I have some questions and comments related to design:
- I suggest to read the thread at:
https://lists.fedorahosted.org/pipermail/sssd-devel/2012-February/008712.htm... if you haven't seen it already.
- Have you seen samba's wbinfo tool? Maybe in your thesis you could
compare the two.
- The list of objects to operate on is incomplete. You also need to
work with sudo rules, SSH hosts and I would also strongly suggest including domains and subdomains. (for domains, include info like sysdb domain version for instance). There should also be a way to perform unkeyed search (=all objects of type X).
- Is the output always going to include all attributes or were you
going to have a "human readable" and "raw" output? What about group memberships? Are there any plans to merge functionality with the sss_groupshow tool ?
- There are a couple of tickets in the SSSD trac related to this
effort. Depending on the scope of your thesis you might want to assign them to yourself.
The related tickets are: https://fedorahosted.org/sssd/ticket/1220 https://fedorahosted.org/sssd/ticket/1221 https://fedorahosted.org/sssd/ticket/1222 https://fedorahosted.org/sssd/ticket/1223
Feel free to also create another ones.
- Please also include plans for unit tests in your design and
implementation. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Dne 25.2.2013 11:50, Jakub Hrozek napsal(a):
On Mon, Feb 25, 2013 at 11:11:01AM +0100, David Bambušek wrote:
Greetings, my name is David Bambusek, and I am working on an application , that will be used as a tool for querying SSSD database. I am curently a student of FIT VUT Brno and this is topic of my bachelor thesis. It will be a command line application able to query various types of data, that is stored in SSSD database. I made a web page, where I will add information about this application I named sss_query, so far, you can find there information about basic user interface ( www.bambusekd.cz/sss_query.html ).
My project is on my Github account https://github.com/bambusekd/SSSD.git there I created branch sss_query, where I work on my application
I will be glad for any advice, recommendation or comment to it.
Thanks
David Bambusek
Hi,
I have some questions and comments related to design:
- I suggest to read the thread at:
https://lists.fedorahosted.org/pipermail/sssd-devel/2012-February/008712.htm... if you haven't seen it already.
- Have you seen samba's wbinfo tool? Maybe in your thesis you could
compare the two.
- The list of objects to operate on is incomplete. You also need to
work with sudo rules, SSH hosts and I would also strongly suggest including domains and subdomains. (for domains, include info like sysdb domain version for instance). There should also be a way to perform unkeyed search (=all objects of type X).
- Is the output always going to include all attributes or were you
going to have a "human readable" and "raw" output? What about group memberships? Are there any plans to merge functionality with the sss_groupshow tool ?
- There are a couple of tickets in the SSSD trac related to this
effort. Depending on the scope of your thesis you might want to assign them to yourself.
The related tickets are: https://fedorahosted.org/sssd/ticket/1220 https://fedorahosted.org/sssd/ticket/1221 https://fedorahosted.org/sssd/ticket/1222 https://fedorahosted.org/sssd/ticket/1223
Feel free to also create another ones.
- Please also include plans for unit tests in your design and
implementation. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hello, I would like to ask for a little help with tests for my application. I went through source codes of tests in /src/tests and have been little bit confused from them. So far, I have always made tests for applications in bash, where I was testing the application on correct outputs from given inputs, but as I saw, you use quite different approach in SSSD. Do I understand correctly, that you make test for every function that you write? Is there any documentation/tutorial on how to make tests for code zou write? Or could you push me forward little bit or give me piece of advice, so I would know how to make tests for my application?
Thank you very much in advance David Bambusek
On Wed, Mar 27, 2013 at 01:45:29AM +0100, David Bambušek wrote:
Dne 25.2.2013 11:50, Jakub Hrozek napsal(a):
On Mon, Feb 25, 2013 at 11:11:01AM +0100, David Bambušek wrote:
Greetings, my name is David Bambusek, and I am working on an application , that will be used as a tool for querying SSSD database. I am curently a student of FIT VUT Brno and this is topic of my bachelor thesis. It will be a command line application able to query various types of data, that is stored in SSSD database. I made a web page, where I will add information about this application I named sss_query, so far, you can find there information about basic user interface ( www.bambusekd.cz/sss_query.html ).
My project is on my Github account https://github.com/bambusekd/SSSD.git there I created branch sss_query, where I work on my application
I will be glad for any advice, recommendation or comment to it.
Thanks
David Bambusek
Hi,
I have some questions and comments related to design:
- I suggest to read the thread at:
https://lists.fedorahosted.org/pipermail/sssd-devel/2012-February/008712.htm... if you haven't seen it already.
- Have you seen samba's wbinfo tool? Maybe in your thesis you could
compare the two.
- The list of objects to operate on is incomplete. You also need to
work with sudo rules, SSH hosts and I would also strongly suggest including domains and subdomains. (for domains, include info like sysdb domain version for instance). There should also be a way to perform unkeyed search (=all objects of type X).
- Is the output always going to include all attributes or were you
going to have a "human readable" and "raw" output? What about group memberships? Are there any plans to merge functionality with the sss_groupshow tool ?
- There are a couple of tickets in the SSSD trac related to this
effort. Depending on the scope of your thesis you might want to assign them to yourself.
The related tickets are: https://fedorahosted.org/sssd/ticket/1220 https://fedorahosted.org/sssd/ticket/1221 https://fedorahosted.org/sssd/ticket/1222 https://fedorahosted.org/sssd/ticket/1223
Feel free to also create another ones.
- Please also include plans for unit tests in your design and
implementation. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hello, I would like to ask for a little help with tests for my application. I went through source codes of tests in /src/tests and have been little bit confused from them. So far, I have always made tests for applications in bash, where I was testing the application on correct outputs from given inputs, but as I saw, you use quite different approach in SSSD. Do I understand correctly, that you make test for every function that you write? Is there any documentation/tutorial on how to make tests for code zou write? Or could you push me forward little bit or give me piece of advice, so I would know how to make tests for my application?
Thank you very much in advance David Bambusek
The tests in src/tests are usually API tests (except for the latest cmocka-based ones). That's one option -- if your tool has a number of functions internally that walk the database and collect their contents, you could write tests in C for those functions.
But then maybe instead of testing the functions there it might be better to create a test in Python similar to src/tests/pyhbac-test.py that would store an entity to cache using the pysss module (or calling the sss_ tools) and then call your sss_query and assert that the object sss_query found is the one you stored with the pysss module.
A test in bash might do as well as a last resort, but I personally find any bash code larger than a couple of lines unreadable and hard to maintain.
Dne 25.2.2013 11:50, Jakub Hrozek napsal(a):
On Mon, Feb 25, 2013 at 11:11:01AM +0100, David Bambušek wrote:
Greetings, my name is David Bambusek, and I am working on an application , that will be used as a tool for querying SSSD database. I am curently a student of FIT VUT Brno and this is topic of my bachelor thesis. It will be a command line application able to query various types of data, that is stored in SSSD database. I made a web page, where I will add information about this application I named sss_query, so far, you can find there information about basic user interface ( www.bambusekd.cz/sss_query.html ).
My project is on my Github account https://github.com/bambusekd/SSSD.git there I created branch sss_query, where I work on my application
I will be glad for any advice, recommendation or comment to it.
Thanks
David Bambusek
Hi,
I have some questions and comments related to design:
- I suggest to read the thread at:
https://lists.fedorahosted.org/pipermail/sssd-devel/2012-February/008712.htm... if you haven't seen it already.
- Have you seen samba's wbinfo tool? Maybe in your thesis you could
compare the two.
- The list of objects to operate on is incomplete. You also need to
work with sudo rules, SSH hosts and I would also strongly suggest including domains and subdomains. (for domains, include info like sysdb domain version for instance). There should also be a way to perform unkeyed search (=all objects of type X).
- Is the output always going to include all attributes or were you
going to have a "human readable" and "raw" output? What about group memberships? Are there any plans to merge functionality with the sss_groupshow tool ?
- There are a couple of tickets in the SSSD trac related to this
effort. Depending on the scope of your thesis you might want to assign them to yourself.
The related tickets are: https://fedorahosted.org/sssd/ticket/1220 https://fedorahosted.org/sssd/ticket/1221 https://fedorahosted.org/sssd/ticket/1222 https://fedorahosted.org/sssd/ticket/1223
Feel free to also create another ones.
- Please also include plans for unit tests in your design and
implementation. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Greetings, I have uploaded a new version of my sss_query application on my Github account:
https://github.com/bambusekd/SSSD.git branch sss_query
I added the missing objects and also option to perform search for all object of a certain type.
I am still working on part for sudo rules and on tests...
David Bambušek
On Tue, Apr 09, 2013 at 11:24:40PM +0200, David Bambušek wrote:
Dne 25.2.2013 11:50, Jakub Hrozek napsal(a):
On Mon, Feb 25, 2013 at 11:11:01AM +0100, David Bambušek wrote:
Greetings, my name is David Bambusek, and I am working on an application , that will be used as a tool for querying SSSD database. I am curently a student of FIT VUT Brno and this is topic of my bachelor thesis. It will be a command line application able to query various types of data, that is stored in SSSD database. I made a web page, where I will add information about this application I named sss_query, so far, you can find there information about basic user interface ( www.bambusekd.cz/sss_query.html ).
My project is on my Github account https://github.com/bambusekd/SSSD.git there I created branch sss_query, where I work on my application
I will be glad for any advice, recommendation or comment to it.
Thanks
David Bambusek
Hi,
I have some questions and comments related to design:
- I suggest to read the thread at:
https://lists.fedorahosted.org/pipermail/sssd-devel/2012-February/008712.htm... if you haven't seen it already.
- Have you seen samba's wbinfo tool? Maybe in your thesis you could
compare the two.
- The list of objects to operate on is incomplete. You also need to
work with sudo rules, SSH hosts and I would also strongly suggest including domains and subdomains. (for domains, include info like sysdb domain version for instance). There should also be a way to perform unkeyed search (=all objects of type X).
- Is the output always going to include all attributes or were you
going to have a "human readable" and "raw" output? What about group memberships? Are there any plans to merge functionality with the sss_groupshow tool ?
- There are a couple of tickets in the SSSD trac related to this
effort. Depending on the scope of your thesis you might want to assign them to yourself.
The related tickets are: https://fedorahosted.org/sssd/ticket/1220 https://fedorahosted.org/sssd/ticket/1221 https://fedorahosted.org/sssd/ticket/1222 https://fedorahosted.org/sssd/ticket/1223
Feel free to also create another ones.
- Please also include plans for unit tests in your design and
implementation. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Greetings, I have uploaded a new version of my sss_query application on my Github account:
https://github.com/bambusekd/SSSD.git branch sss_query
I added the missing objects and also option to perform search for all object of a certain type.
I am still working on part for sudo rules and on tests...
David Bambušek
The branch doesn't compile:
make[1]: *** No rule to make target `src/providers/proxy/proxy_common.c', needed by `src/providers/proxy/libsss_proxy_la-proxy_common.lo'. Stop.
Seems like you forgot to add proxy_common.c
I would encourage you to regularly rebase your branch atop master, currently it's almost 2 months old and the rebase is just going to get harder.
Also please set your real name and e-mail in .gitconfig: https://fedorahosted.org/sssd/wiki/DevelTutorials One of the commits is authored by "root".
The code should follow our coding guidelines: http://www.freeipa.org/page/Coding_Style
In your code the indentation seems too deep. Also please use spaces after "if", "for" and other keywords. There's quite a couple of trailing whitespace -- if you're a vim user you can set: let c_no_tab_space_error=1 let c_space_errors=1 to your ~/.vimrc
Also try to keep the lines under 80 characters if possible.
I skimmed through the code real quick and I have a couple of comments before I can actually test the tool.
* Why did you re-add srcdir to the PYTHON_TESTS directive? It was explicitly removed. * I think you should learn how talloc works. Here is an excellent tutorial written by one of the SSSD developers: https://talloc.samba.org/talloc/doc/html/libtalloc__tutorial.html There is two patterns I see in the code that are wrong: ** some functions allocate tmp_ctx on NULL and never free it. That's a memory leak. I saw that some of the functions also allocate output data on mem_ctx, you should either allocate the output data on mem_ctx and ditch tmp_ctx or talloc_steal from tmp_ctx to mem_ctx before return. ** some functions allocate structure components on top of a memory context but then free the components of the structure individually (that's in init_domains). You can just free the top-level context.
That's it for now. It's a decent start, but needs some work before it can be accepted upstream.
Since I was really working with two months old version of SSSD, I made completely new mirror of the newest SSSD on my github account and made some changes in code of my sss_query, so it is compatible with it now. There should be no problem with compiling anymore.
I also went through the code and changed it according to coding guidelines. About indentation, I use 4 spaces as coding style suggests, could you please specify where do I have it wrong? I also wrapped most of lines so they are not exceeding the 80 character limit, I just have kept some of them longer, cause in my opinion they would become unreadable, if wrapped, but still they are never longer than about 100 characters.
I also corrected the memory leaks and memory handling through out the application, so hopefully it is alright now.
David Bambušek
2013/4/10 Jakub Hrozek jhrozek@redhat.com
On Tue, Apr 09, 2013 at 11:24:40PM +0200, David Bambušek wrote:
Dne 25.2.2013 11:50, Jakub Hrozek napsal(a):
On Mon, Feb 25, 2013 at 11:11:01AM +0100, David Bambušek wrote:
Greetings, my name is David Bambusek, and I am working on an application , that
will
be used as a tool for querying SSSD database. I am curently a student
of
FIT VUT Brno and this is topic of my bachelor thesis. It will be a
command
line application able to query various types of data, that is stored in SSSD database. I made a web page, where I will add information about
this
application I named sss_query, so far, you can find there information
about
basic user interface ( www.bambusekd.cz/sss_query.html ).
My project is on my Github account https://github.com/bambusekd/SSSD.git there I created branch sss_query, where I work on my application
I will be glad for any advice, recommendation or comment to it.
Thanks
David Bambusek
Hi,
I have some questions and comments related to design:
- I suggest to read the thread at:
https://lists.fedorahosted.org/pipermail/sssd-devel/2012-February/008712.htm...
if you haven't seen it already.
- Have you seen samba's wbinfo tool? Maybe in your thesis you could
compare the two.
- The list of objects to operate on is incomplete. You also need to
work with sudo rules, SSH hosts and I would also strongly suggest including domains and subdomains. (for domains, include info like sysdb domain version for instance). There should also be a way to perform unkeyed search (=all objects of type X).
- Is the output always going to include all attributes or were you
going to have a "human readable" and "raw" output? What about group memberships? Are there any plans to merge functionality with the sss_groupshow tool ?
- There are a couple of tickets in the SSSD trac related to this
effort. Depending on the scope of your thesis you might want to assign them to yourself.
The related tickets are: https://fedorahosted.org/sssd/ticket/1220 https://fedorahosted.org/sssd/ticket/1221 https://fedorahosted.org/sssd/ticket/1222 https://fedorahosted.org/sssd/ticket/1223
Feel free to also create another ones.
- Please also include plans for unit tests in your design and
implementation. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Greetings, I have uploaded a new version of my sss_query application on my Github account:
https://github.com/bambusekd/SSSD.git branch sss_query
I added the missing objects and also option to perform search for all object of a certain type.
I am still working on part for sudo rules and on tests...
David Bambušek
The branch doesn't compile:
make[1]: *** No rule to make target `src/providers/proxy/proxy_common.c', needed by `src/providers/proxy/libsss_proxy_la-proxy_common.lo'. Stop.
Seems like you forgot to add proxy_common.c
I would encourage you to regularly rebase your branch atop master, currently it's almost 2 months old and the rebase is just going to get harder.
Also please set your real name and e-mail in .gitconfig: https://fedorahosted.org/sssd/wiki/DevelTutorials One of the commits is authored by "root".
The code should follow our coding guidelines: http://www.freeipa.org/page/Coding_Style
In your code the indentation seems too deep. Also please use spaces after "if", "for" and other keywords. There's quite a couple of trailing whitespace -- if you're a vim user you can set: let c_no_tab_space_error=1 let c_space_errors=1 to your ~/.vimrc
Also try to keep the lines under 80 characters if possible.
I skimmed through the code real quick and I have a couple of comments before I can actually test the tool.
- Why did you re-add srcdir to the PYTHON_TESTS directive? It was explicitly removed.
- I think you should learn how talloc works. Here is an excellent tutorial written by one of the SSSD developers: https://talloc.samba.org/talloc/doc/html/libtalloc__tutorial.html There is two patterns I see in the code that are wrong: ** some functions allocate tmp_ctx on NULL and never free it. That's a memory leak. I saw that some of the functions also allocate output data on mem_ctx, you should either allocate the output data on mem_ctx and ditch tmp_ctx or talloc_steal from tmp_ctx to mem_ctx before return. ** some functions allocate structure components on top of a memory context but then free the components of the structure individually (that's in init_domains). You can just free the top-level context.
That's it for now. It's a decent start, but needs some work before it can be accepted upstream. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (12/04/13 01:31), David Bambušek wrote:
Since I was really working with two months old version of SSSD, I made completely new mirror of the newest SSSD on my github account and made some changes in code of my sss_query, so it is compatible with it now. There should be no problem with compiling anymore.
I also went through the code and changed it according to coding guidelines. About indentation, I use 4 spaces as coding style suggests, could you please specify where do I have it wrong? I also wrapped most of lines so they are not exceeding the 80 character limit, I just have kept some of them longer, cause in my opinion they would become unreadable, if wrapped, but still they are never longer than about 100 characters.
You spend much more time with reading code like writing code. So code have to be readable. 80 columns is not about terminal limitations, but about code readability. In most cases if you have line longer than 80 characters, you may doing something wrong. Maybe you try to do a lot of thing on one line, or there are a lot of nested blocks (if, while ...)
Some formatting issues, according to SSSD coding style: http://www.freeipa.org/page/Coding_Style --white spaces at end of line --mixing tabs and spaces I think that proper editor configuration could help you with both.
I also corrected the memory leaks and memory handling through out the application, so hopefully it is alright now.
David Bambušek
And some comments to code itself.
For each type you define as #define <type>_SHOW_ATTRS { SYSDB_CONST_1, ... SYSDB_CONST_n, NULL } #define <type>_SHOW_ATTRS_NUM number ... static const char *attrs[] = <type>_SHOW_ATTRS;
--You have to manually define array size, for each ATTR, it is error prone. --You don't need to know array size, because each array is NULL terminated,
So you can transform it to something like this:
// much more readable and shorter then 80 columns const char *SSHHOST_SHOW_ATTRS[] = { SYSDB_SSH_HOST_OC, SYSDB_SSH_KNOWN_HOSTS_EXPIRE, SYSDB_SSH_PUBKEY, NULL }; ... const char **attrs = SSHHOST_SHOW_ATTRS;
And instead of while iteration with counter, you can benefit from NULL terminated array. With this approach you can remove parameters "int <type>_attrs_num" from all functions. - int counter = 0; - while (counter < user_attrs_num){ - if (strcmp(user_attrs[counter],SYSDB_NAME) == 0){ + for (attr_iter = user_attrs; attr_iter; ++attr_iter){ + if (strcmp(*attr_iter, SYSDB_NAME) == 0){
----------------------------------------------------------------------- ----------------------------------------------------------------------- But most important think is that there is a lot of code duplication. For example If I decide to add/remove new attribute to SSHHOST_SHOW_ATTRS I have to: --add/remove SYSDB_CONST to SSHHOST_SHOW_ATTRS --add/remove member from struct sshhost_info --add/remove lines to search_user --add/remove lines to search_all_user (the same like in search user) --add/remove lines to print_user It is also error prone.
You declare 6 structures: user_info, group_info, netgroup_info, service_info autofsm_info, sshhost_info For each structure you create functions with the same pattern. print_<type>_info search_<type> search_all_<type>
Both search function have a lot of similar code. In search function you map <key, value> to structure members. In print function you map structure members to <key, value> and print formated <key, value>. This mapping is useless, because key and value are strings. It would be easier to use hash_table instead of structures. In that case, you can write general print, search, search_all function for all types. SSSD already use hash_table from ding-libs package. Header file dhash.h
And at the end one positive: Output from this utility is quite good.
LS
On Fri, Apr 12, 2013 at 06:42:03PM +0200, Lukas Slebodnik wrote:
On (12/04/13 01:31), David Bambušek wrote:
Since I was really working with two months old version of SSSD, I made completely new mirror of the newest SSSD on my github account and made some changes in code of my sss_query, so it is compatible with it now. There should be no problem with compiling anymore.
I also went through the code and changed it according to coding guidelines. About indentation, I use 4 spaces as coding style suggests, could you please specify where do I have it wrong? I also wrapped most of lines so they are not exceeding the 80 character limit, I just have kept some of them longer, cause in my opinion they would become unreadable, if wrapped, but still they are never longer than about 100 characters.
You spend much more time with reading code like writing code. So code have to be readable. 80 columns is not about terminal limitations, but about code readability. In most cases if you have line longer than 80 characters, you may doing something wrong. Maybe you try to do a lot of thing on one line, or there are a lot of nested blocks (if, while ...)
Some formatting issues, according to SSSD coding style: http://www.freeipa.org/page/Coding_Style --white spaces at end of line --mixing tabs and spaces I think that proper editor configuration could help you with both.
I also corrected the memory leaks and memory handling through out the application, so hopefully it is alright now.
David Bambušek
And some comments to code itself.
For each type you define as #define <type>_SHOW_ATTRS { SYSDB_CONST_1, ... SYSDB_CONST_n, NULL } #define <type>_SHOW_ATTRS_NUM number ... static const char *attrs[] = <type>_SHOW_ATTRS;
--You have to manually define array size, for each ATTR, it is error prone. --You don't need to know array size, because each array is NULL terminated,
So you can transform it to something like this:
// much more readable and shorter then 80 columns const char *SSHHOST_SHOW_ATTRS[] = { SYSDB_SSH_HOST_OC, SYSDB_SSH_KNOWN_HOSTS_EXPIRE, SYSDB_SSH_PUBKEY, NULL }; ... const char **attrs = SSHHOST_SHOW_ATTRS;
And instead of while iteration with counter, you can benefit from NULL terminated array. With this approach you can remove parameters "int <type>_attrs_num" from all functions.
- int counter = 0;
- while (counter < user_attrs_num){
if (strcmp(user_attrs[counter],SYSDB_NAME) == 0){
- for (attr_iter = user_attrs; attr_iter; ++attr_iter){
if (strcmp(*attr_iter, SYSDB_NAME) == 0){
But most important think is that there is a lot of code duplication. For example If I decide to add/remove new attribute to SSHHOST_SHOW_ATTRS I have to: --add/remove SYSDB_CONST to SSHHOST_SHOW_ATTRS --add/remove member from struct sshhost_info --add/remove lines to search_user --add/remove lines to search_all_user (the same like in search user) --add/remove lines to print_user It is also error prone.
You declare 6 structures: user_info, group_info, netgroup_info, service_info autofsm_info, sshhost_info For each structure you create functions with the same pattern. print_<type>_info search_<type> search_all_<type>
Both search function have a lot of similar code. In search function you map <key, value> to structure members. In print function you map structure members to <key, value> and print formated <key, value>. This mapping is useless, because key and value are strings. It would be easier to use hash_table instead of structures. In that case, you can write general print, search, search_all function for all types. SSSD already use hash_table from ding-libs package. Header file dhash.h
And at the end one positive: Output from this utility is quite good.
In addition to Lukas' comments about the code I also wanted to comment on the functionality:
Currently the databases are only readable by root. You should either restrict the tool so that it errors out if not ran by root, or handle permission errors when opening the db gracefully.
The tool must be able to understand and parse fully qualified user names. Use the sss_parse_name() function for that, you can copy its usage from the sss_cache tool. If the FQ names support is good enough, I would consider dropping the -D parameter althogether.
I think the default set of attributes displayed for the user should be the same that getpwnam() returns. See the output of "getent passwd $user".
Currently the tool only prints the raw attribute names from ldb. Do you also plan on printing human readable names ("GID Number" instead of "gidNumber") ?
Some entries might not have "gecos" at all. In that case, fall back to the cn value.
There seems to be a bug when displaying domain info: $ sudo ./sss_query -d -N ipaldap There is no domain "ipaldap"! <--- yes there is Domain name: ipaldap Domain provider: ldap Domain version: 0.15 Domain's subdomains: <--- diplayed twice Domain's subdomains: <---
The "Domain's subdomains" probably shouldn't be displayed at all if there are no subdomains.
I saw some weird behaviour when I had multiple domains configured. When I tried to query user from the first domain in the list sss_query first printed the user, but then said "Object not found".
Similarly when I wanted to query an object from the second domain, I had to use the -D option to specify the exact domain. I would expect the tool to iterate over the domains.
The messages that are user visible must be marked as translatable. See the definition of the ERROR() macro to see how to do that.
When the tool is ready, it's going to require a man page and it needs to be mentioned in sssd.spec.in so it's packaged in the RPMs correctly.
Please ask your thesis mentor if you can keep the copyright assigned to yourself in the thesis. I vaguely remember I had to submit the copyright to FIT VUT back in the day :-)
2013/4/12 Jakub Hrozek jhrozek@redhat.com
On Fri, Apr 12, 2013 at 06:42:03PM +0200, Lukas Slebodnik wrote:
On (12/04/13 01:31), David Bambušek wrote:
Since I was really working with two months old version of SSSD, I made completely new mirror of the newest SSSD on my github account and made
some
changes in code of my sss_query, so it is compatible with it now. There should be no problem with compiling anymore.
I also went through the code and changed it according to coding
guidelines.
About indentation, I use 4 spaces as coding style suggests, could you please specify where do I have it wrong? I also wrapped most of lines so they are not exceeding the 80 character limit, I just have kept some of them longer, cause in my opinion they would become unreadable, if
wrapped,
but still they are never longer than about 100 characters.
You spend much more time with reading code like writing code. So code
have to
be readable. 80 columns is not about terminal limitations, but about code readability. In most cases if you have line longer than 80 characters, you may doing something wrong. Maybe you try to do a lot of thing on one line, or
there are
a lot of nested blocks (if, while ...)
Some formatting issues, according to SSSD coding style: http://www.freeipa.org/page/Coding_Style --white spaces at end of line --mixing tabs and spaces I think that proper editor configuration could help you with both.
I corrected it, so it should be fine now.
I also corrected the memory leaks and memory handling through out the application, so hopefully it is alright now.
David Bambušek
And some comments to code itself.
For each type you define as #define <type>_SHOW_ATTRS { SYSDB_CONST_1, ... SYSDB_CONST_n, NULL } #define <type>_SHOW_ATTRS_NUM number ... static const char *attrs[] = <type>_SHOW_ATTRS;
--You have to manually define array size, for each ATTR, it is error
prone.
--You don't need to know array size, because each array is NULL
terminated,
So you can transform it to something like this:
// much more readable and shorter then 80 columns const char *SSHHOST_SHOW_ATTRS[] = { SYSDB_SSH_HOST_OC, SYSDB_SSH_KNOWN_HOSTS_EXPIRE, SYSDB_SSH_PUBKEY, NULL }; ... const char **attrs = SSHHOST_SHOW_ATTRS;
And instead of while iteration with counter, you can benefit from NULL terminated array. With this approach you can remove parameters "int <type>_attrs_num" from all functions.
- int counter = 0;
- while (counter < user_attrs_num){
if (strcmp(user_attrs[counter],SYSDB_NAME) == 0){
- for (attr_iter = user_attrs; attr_iter; ++attr_iter){
if (strcmp(*attr_iter, SYSDB_NAME) == 0){
That is clever, thanks for advice.
But most important think is that there is a lot of code duplication. For example If I decide to add/remove new attribute to SSHHOST_SHOW_ATTRS I have to: --add/remove SYSDB_CONST to SSHHOST_SHOW_ATTRS --add/remove member from struct sshhost_info --add/remove lines to search_user --add/remove lines to search_all_user (the same like in search user) --add/remove lines to print_user It is also error prone.
You declare 6 structures: user_info, group_info, netgroup_info,
service_info
autofsm_info, sshhost_info
For each structure you create functions with the same pattern. print_<type>_info search_<type> search_all_<type>
Both search function have a lot of similar code. In search function you map <key, value> to structure members. In print function you map structure members to <key, value> and print formated <key, value>. This mapping is useless, because key and
value
are strings. It would be easier to use hash_table instead of structures. In that case, you can write general print, search, search_all function
for all
types. SSSD already use hash_table from ding-libs package. Header file
dhash.h
And at the end one positive: Output from this utility is quite good.
I completely redesigned the tools, made it more general and also used hash table.
In addition to Lukas' comments about the code I also wanted to comment on the functionality:
Currently the databases are only readable by root. You should either restrict the tool so that it errors out if not ran by root, or handle permission errors when opening the db gracefully.
The tool must be able to understand and parse fully qualified user names. Use the sss_parse_name() function for that, you can copy its usage from the sss_cache tool. If the FQ names support is good enough, I would consider dropping the -D parameter althogether.
done
I think the default set of attributes displayed for the user should be the same that getpwnam() returns. See the output of "getent passwd $user".
done
Currently the tool only prints the raw attribute names from ldb. Do you also plan on printing human readable names ("GID Number" instead of "gidNumber") ?
I redesigned all the prints, so now it id nicer for humans :)
Some entries might not have "gecos" at all. In that case, fall back to the cn value.
There seems to be a bug when displaying domain info: $ sudo ./sss_query -d -N ipaldap There is no domain "ipaldap"! <--- yes there is Domain name: ipaldap Domain provider: ldap Domain version: 0.15 Domain's subdomains: <--- diplayed twice Domain's subdomains: <---
The "Domain's subdomains" probably shouldn't be displayed at all if there are no subdomains.
I saw some weird behaviour when I had multiple domains configured. When I tried to query user from the first domain in the list sss_query first printed the user, but then said "Object not found".
Similarly when I wanted to query an object from the second domain, I had to use the -D option to specify the exact domain. I would expect the tool to iterate over the domains.
There was a bug that caused all this behavior, now it should be OK.
The messages that are user visible must be marked as translatable. See the definition of the ERROR() macro to see how to do that.
When the tool is ready, it's going to require a man page and it needs to be mentioned in sssd.spec.in so it's packaged in the RPMs correctly.
I made the man page and also put mention is sssd.spec.in.
Please ask your thesis mentor if you can keep the copyright assigned to yourself in the thesis. I vaguely remember I had to submit the copyright to FIT VUT back in the day :-)
In progress, mentor is busy and not responding at the moment :)
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks for all your advice and tips. I uploaded new version with new design, hopefully fixed all the bugs and added all the missing functionality. I will welcome all comments to new design or reports of mistakes.
Thanks David Bambušek
On (16/04/13 00:12), David Bambušek wrote:
2013/4/12 Jakub Hrozek jhrozek@redhat.com
On Fri, Apr 12, 2013 at 06:42:03PM +0200, Lukas Slebodnik wrote:
On (12/04/13 01:31), David Bambušek wrote:
Since I was really working with two months old version of SSSD, I made completely new mirror of the newest SSSD on my github account and made
some
changes in code of my sss_query, so it is compatible with it now. There should be no problem with compiling anymore.
I also went through the code and changed it according to coding
guidelines.
About indentation, I use 4 spaces as coding style suggests, could you please specify where do I have it wrong? I also wrapped most of lines so they are not exceeding the 80 character limit, I just have kept some of them longer, cause in my opinion they would become unreadable, if
wrapped,
but still they are never longer than about 100 characters.
You spend much more time with reading code like writing code. So code
have to
be readable. 80 columns is not about terminal limitations, but about code readability. In most cases if you have line longer than 80 characters, you may doing something wrong. Maybe you try to do a lot of thing on one line, or
there are
a lot of nested blocks (if, while ...)
Some formatting issues, according to SSSD coding style: http://www.freeipa.org/page/Coding_Style --white spaces at end of line --mixing tabs and spaces I think that proper editor configuration could help you with both.
I corrected it, so it should be fine now.
I also corrected the memory leaks and memory handling through out the application, so hopefully it is alright now.
David Bambušek
And some comments to code itself.
For each type you define as #define <type>_SHOW_ATTRS { SYSDB_CONST_1, ... SYSDB_CONST_n, NULL } #define <type>_SHOW_ATTRS_NUM number ... static const char *attrs[] = <type>_SHOW_ATTRS;
--You have to manually define array size, for each ATTR, it is error
prone.
--You don't need to know array size, because each array is NULL
terminated,
So you can transform it to something like this:
// much more readable and shorter then 80 columns const char *SSHHOST_SHOW_ATTRS[] = { SYSDB_SSH_HOST_OC, SYSDB_SSH_KNOWN_HOSTS_EXPIRE, SYSDB_SSH_PUBKEY, NULL }; ... const char **attrs = SSHHOST_SHOW_ATTRS;
And instead of while iteration with counter, you can benefit from NULL terminated array. With this approach you can remove parameters "int <type>_attrs_num" from all functions.
- int counter = 0;
- while (counter < user_attrs_num){
if (strcmp(user_attrs[counter],SYSDB_NAME) == 0){
- for (attr_iter = user_attrs; attr_iter; ++attr_iter){
if (strcmp(*attr_iter, SYSDB_NAME) == 0){
That is clever, thanks for advice.
But most important think is that there is a lot of code duplication. For example If I decide to add/remove new attribute to SSHHOST_SHOW_ATTRS I have to: --add/remove SYSDB_CONST to SSHHOST_SHOW_ATTRS --add/remove member from struct sshhost_info --add/remove lines to search_user --add/remove lines to search_all_user (the same like in search user) --add/remove lines to print_user It is also error prone.
You declare 6 structures: user_info, group_info, netgroup_info,
service_info
autofsm_info, sshhost_info
For each structure you create functions with the same pattern. print_<type>_info search_<type> search_all_<type>
Both search function have a lot of similar code. In search function you map <key, value> to structure members. In print function you map structure members to <key, value> and print formated <key, value>. This mapping is useless, because key and
value
are strings. It would be easier to use hash_table instead of structures. In that case, you can write general print, search, search_all function
for all
types. SSSD already use hash_table from ding-libs package. Header file
dhash.h
And at the end one positive: Output from this utility is quite good.
I completely redesigned the tools, made it more general and also used hash table.
In addition to Lukas' comments about the code I also wanted to comment on the functionality:
Currently the databases are only readable by root. You should either restrict the tool so that it errors out if not ran by root, or handle permission errors when opening the db gracefully.
The tool must be able to understand and parse fully qualified user names. Use the sss_parse_name() function for that, you can copy its usage from the sss_cache tool. If the FQ names support is good enough, I would consider dropping the -D parameter althogether.
done
I think the default set of attributes displayed for the user should be the same that getpwnam() returns. See the output of "getent passwd $user".
done
Currently the tool only prints the raw attribute names from ldb. Do you also plan on printing human readable names ("GID Number" instead of "gidNumber") ?
I redesigned all the prints, so now it id nicer for humans :)
Some entries might not have "gecos" at all. In that case, fall back to the cn value.
There seems to be a bug when displaying domain info: $ sudo ./sss_query -d -N ipaldap There is no domain "ipaldap"! <--- yes there is Domain name: ipaldap Domain provider: ldap Domain version: 0.15 Domain's subdomains: <--- diplayed twice Domain's subdomains: <---
The "Domain's subdomains" probably shouldn't be displayed at all if there are no subdomains.
I saw some weird behaviour when I had multiple domains configured. When I tried to query user from the first domain in the list sss_query first printed the user, but then said "Object not found".
Similarly when I wanted to query an object from the second domain, I had to use the -D option to specify the exact domain. I would expect the tool to iterate over the domains.
There was a bug that caused all this behavior, now it should be OK.
The messages that are user visible must be marked as translatable. See the definition of the ERROR() macro to see how to do that.
When the tool is ready, it's going to require a man page and it needs to be mentioned in sssd.spec.in so it's packaged in the RPMs correctly.
I made the man page and also put mention is sssd.spec.in.
Please ask your thesis mentor if you can keep the copyright assigned to yourself in the thesis. I vaguely remember I had to submit the copyright to FIT VUT back in the day :-)
In progress, mentor is busy and not responding at the moment :)
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks for all your advice and tips. I uploaded new version with new design, hopefully fixed all the bugs and added all the missing functionality. I will welcome all comments to new design or reports of mistakes.
Thanks David Bambušek
That code is much more better, after redesign code is approximately 35% shorter and much more readable.
In function print_object, you iterate over array of strings (user_attrs), foreach attr in user_attrs: int_value = get_value_from_hash(hash, attr) switch(int_value)
....... case A_<type>: oa->key = g_strdup(SYSDB_<type>); ^^^^^^^^^^^^ this is the same like a "attr" ret_value = talloc_strdup(mem_ctx,ldb_msg_find_attr_as_string(msg, SYSDB_<type>, NULL)); ^^^^^^^^^^^^ this is also "attr" strcpy(final, "Name:\t "); ^^^^^^^^^ human readable version of "attr" break;
......
The purpose of hash_table was to reduce very long switch/case. Hash table will help you with mapping one type to another and code will be much more simpler.
You can transform current hash table <string, int> (SYSDB_<type>, A_<type>) to hash table <string, string> (SYSDB_type, "human readable version of SYSDB_<type>" This approach should help you to remove switch/case from print_object function.
Also function search_object is longer then 250 lines, so you could try to: -- split it to two functions search_object, search_object_all -- or you could try to reuse function parameter "bool all"
LS
On (16/04/13 10:50), Lukas Slebodnik wrote:
On (16/04/13 00:12), David Bambušek wrote:
2013/4/12 Jakub Hrozek jhrozek@redhat.com
On Fri, Apr 12, 2013 at 06:42:03PM +0200, Lukas Slebodnik wrote:
On (12/04/13 01:31), David Bambušek wrote:
Since I was really working with two months old version of SSSD, I made completely new mirror of the newest SSSD on my github account and made
some
changes in code of my sss_query, so it is compatible with it now. There should be no problem with compiling anymore.
I also went through the code and changed it according to coding
guidelines.
About indentation, I use 4 spaces as coding style suggests, could you please specify where do I have it wrong? I also wrapped most of lines so they are not exceeding the 80 character limit, I just have kept some of them longer, cause in my opinion they would become unreadable, if
wrapped,
but still they are never longer than about 100 characters.
You spend much more time with reading code like writing code. So code
have to
be readable. 80 columns is not about terminal limitations, but about code readability. In most cases if you have line longer than 80 characters, you may doing something wrong. Maybe you try to do a lot of thing on one line, or
there are
a lot of nested blocks (if, while ...)
Some formatting issues, according to SSSD coding style: http://www.freeipa.org/page/Coding_Style --white spaces at end of line --mixing tabs and spaces I think that proper editor configuration could help you with both.
I corrected it, so it should be fine now.
I also corrected the memory leaks and memory handling through out the application, so hopefully it is alright now.
David Bambušek
And some comments to code itself.
For each type you define as #define <type>_SHOW_ATTRS { SYSDB_CONST_1, ... SYSDB_CONST_n, NULL } #define <type>_SHOW_ATTRS_NUM number ... static const char *attrs[] = <type>_SHOW_ATTRS;
--You have to manually define array size, for each ATTR, it is error
prone.
--You don't need to know array size, because each array is NULL
terminated,
So you can transform it to something like this:
// much more readable and shorter then 80 columns const char *SSHHOST_SHOW_ATTRS[] = { SYSDB_SSH_HOST_OC, SYSDB_SSH_KNOWN_HOSTS_EXPIRE, SYSDB_SSH_PUBKEY, NULL }; ... const char **attrs = SSHHOST_SHOW_ATTRS;
And instead of while iteration with counter, you can benefit from NULL terminated array. With this approach you can remove parameters "int <type>_attrs_num" from all functions.
- int counter = 0;
- while (counter < user_attrs_num){
if (strcmp(user_attrs[counter],SYSDB_NAME) == 0){
- for (attr_iter = user_attrs; attr_iter; ++attr_iter){
if (strcmp(*attr_iter, SYSDB_NAME) == 0){
That is clever, thanks for advice.
But most important think is that there is a lot of code duplication. For example If I decide to add/remove new attribute to SSHHOST_SHOW_ATTRS I have to: --add/remove SYSDB_CONST to SSHHOST_SHOW_ATTRS --add/remove member from struct sshhost_info --add/remove lines to search_user --add/remove lines to search_all_user (the same like in search user) --add/remove lines to print_user It is also error prone.
You declare 6 structures: user_info, group_info, netgroup_info,
service_info
autofsm_info, sshhost_info
For each structure you create functions with the same pattern. print_<type>_info search_<type> search_all_<type>
Both search function have a lot of similar code. In search function you map <key, value> to structure members. In print function you map structure members to <key, value> and print formated <key, value>. This mapping is useless, because key and
value
are strings. It would be easier to use hash_table instead of structures. In that case, you can write general print, search, search_all function
for all
types. SSSD already use hash_table from ding-libs package. Header file
dhash.h
And at the end one positive: Output from this utility is quite good.
I completely redesigned the tools, made it more general and also used hash table.
In addition to Lukas' comments about the code I also wanted to comment on the functionality:
Currently the databases are only readable by root. You should either restrict the tool so that it errors out if not ran by root, or handle permission errors when opening the db gracefully.
The tool must be able to understand and parse fully qualified user names. Use the sss_parse_name() function for that, you can copy its usage from the sss_cache tool. If the FQ names support is good enough, I would consider dropping the -D parameter althogether.
done
I think the default set of attributes displayed for the user should be the same that getpwnam() returns. See the output of "getent passwd $user".
done
Currently the tool only prints the raw attribute names from ldb. Do you also plan on printing human readable names ("GID Number" instead of "gidNumber") ?
I redesigned all the prints, so now it id nicer for humans :)
Some entries might not have "gecos" at all. In that case, fall back to the cn value.
There seems to be a bug when displaying domain info: $ sudo ./sss_query -d -N ipaldap There is no domain "ipaldap"! <--- yes there is Domain name: ipaldap Domain provider: ldap Domain version: 0.15 Domain's subdomains: <--- diplayed twice Domain's subdomains: <---
The "Domain's subdomains" probably shouldn't be displayed at all if there are no subdomains.
I saw some weird behaviour when I had multiple domains configured. When I tried to query user from the first domain in the list sss_query first printed the user, but then said "Object not found".
Similarly when I wanted to query an object from the second domain, I had to use the -D option to specify the exact domain. I would expect the tool to iterate over the domains.
There was a bug that caused all this behavior, now it should be OK.
The messages that are user visible must be marked as translatable. See the definition of the ERROR() macro to see how to do that.
When the tool is ready, it's going to require a man page and it needs to be mentioned in sssd.spec.in so it's packaged in the RPMs correctly.
I made the man page and also put mention is sssd.spec.in.
Please ask your thesis mentor if you can keep the copyright assigned to yourself in the thesis. I vaguely remember I had to submit the copyright to FIT VUT back in the day :-)
In progress, mentor is busy and not responding at the moment :)
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks for all your advice and tips. I uploaded new version with new design, hopefully fixed all the bugs and added all the missing functionality. I will welcome all comments to new design or reports of mistakes.
Thanks David Bambušek
That code is much more better, after redesign code is approximately 35% shorter and much more readable.
In function print_object, you iterate over array of strings (user_attrs), foreach attr in user_attrs: int_value = get_value_from_hash(hash, attr) switch(int_value)
....... case A_<type>: oa->key = g_strdup(SYSDB_<type>); ^^^^^^^^^^^^ this is the same like a "attr" ret_value = talloc_strdup(mem_ctx,ldb_msg_find_attr_as_string(msg, SYSDB_<type>, NULL)); ^^^^^^^^^^^^ this is also "attr" strcpy(final, "Name:\t "); ^^^^^^^^^ human readable version of "attr" break; ......
The purpose of hash_table was to reduce very long switch/case. Hash table will help you with mapping one type to another and code will be much more simpler.
You can transform current hash table <string, int> (SYSDB_<type>, A_<type>) to hash table <string, string> (SYSDB_type, "human readable version of SYSDB_<type>" This approach should help you to remove switch/case from print_object function.
Also function search_object is longer then 250 lines, so you could try to: -- split it to two functions search_object, search_object_all -- or you could try to reuse function parameter "bool all"
LS
I forgot to mention one important thing in previous email.
You have used hash table from glib, but glib is optional dependency in sssd. For example in debian distribution, sssd does not depend on glib(libglib). You should use hash table from ding-libs(libdhash)
Fedora distribution have package libdhash-devel and you can find exmaples in directory /usr/share/doc/libdhash-devel-0.4.3/
LS
2013/4/16 Lukas Slebodnik lslebodn@redhat.com
On (16/04/13 10:50), Lukas Slebodnik wrote:
On (16/04/13 00:12), David Bambušek wrote:
2013/4/12 Jakub Hrozek jhrozek@redhat.com
On Fri, Apr 12, 2013 at 06:42:03PM +0200, Lukas Slebodnik wrote:
On (12/04/13 01:31), David Bambušek wrote:
Since I was really working with two months old version of SSSD, I
made
completely new mirror of the newest SSSD on my github account and
made
some
changes in code of my sss_query, so it is compatible with it now.
There
should be no problem with compiling anymore.
I also went through the code and changed it according to coding
guidelines.
About indentation, I use 4 spaces as coding style suggests, could
you
please specify where do I have it wrong? I also wrapped most of
lines so
they are not exceeding the 80 character limit, I just have kept
some of
them longer, cause in my opinion they would become unreadable, if
wrapped,
but still they are never longer than about 100 characters.
You spend much more time with reading code like writing code. So code
have to
be readable. 80 columns is not about terminal limitations, but about code
readability.
In most cases if you have line longer than 80 characters, you may
doing
something wrong. Maybe you try to do a lot of thing on one line, or
there are
a lot of nested blocks (if, while ...)
Some formatting issues, according to SSSD coding style: http://www.freeipa.org/page/Coding_Style --white spaces at end of line --mixing tabs and spaces I think that proper editor configuration could help you with both.
I corrected it, so it should be fine now.
I also corrected the memory leaks and memory handling through out
the
application, so hopefully it is alright now.
David Bambušek
And some comments to code itself.
For each type you define as #define <type>_SHOW_ATTRS { SYSDB_CONST_1, ... SYSDB_CONST_n, NULL } #define <type>_SHOW_ATTRS_NUM number ... static const char *attrs[] = <type>_SHOW_ATTRS;
--You have to manually define array size, for each ATTR, it is error
prone.
--You don't need to know array size, because each array is NULL
terminated,
So you can transform it to something like this:
// much more readable and shorter then 80 columns const char *SSHHOST_SHOW_ATTRS[] = { SYSDB_SSH_HOST_OC, SYSDB_SSH_KNOWN_HOSTS_EXPIRE, SYSDB_SSH_PUBKEY, NULL }; ... const char **attrs = SSHHOST_SHOW_ATTRS;
And instead of while iteration with counter, you can benefit from NULL terminated array. With this approach you can remove parameters "int <type>_attrs_num" from all functions.
- int counter = 0;
- while (counter < user_attrs_num){
if (strcmp(user_attrs[counter],SYSDB_NAME) == 0){
- for (attr_iter = user_attrs; attr_iter; ++attr_iter){
if (strcmp(*attr_iter, SYSDB_NAME) == 0){
That is clever, thanks for advice.
But most important think is that there is a lot of code duplication.
For
example If I decide to add/remove new attribute to SSHHOST_SHOW_ATTRS I have to: --add/remove SYSDB_CONST to SSHHOST_SHOW_ATTRS --add/remove member from struct sshhost_info --add/remove lines to search_user --add/remove lines to search_all_user (the same like in search
user)
--add/remove lines to print_user
It is also error prone.
You declare 6 structures: user_info, group_info, netgroup_info,
service_info
autofsm_info, sshhost_info
For each structure you create functions with the same pattern. print_<type>_info search_<type> search_all_<type>
Both search function have a lot of similar code. In search function you map <key, value> to structure members. In print function you map structure members to <key, value> and print formated <key, value>. This mapping is useless, because key and
value
are strings. It would be easier to use hash_table instead of
structures.
In that case, you can write general print, search, search_all
function
for all
types. SSSD already use hash_table from ding-libs package. Header
file
dhash.h
And at the end one positive: Output from this utility is quite good.
I completely redesigned the tools, made it more general and also used
hash
table.
In addition to Lukas' comments about the code I also wanted to comment
on
the functionality:
Currently the databases are only readable by root. You should either restrict the tool so that it errors out if not ran by root, or handle permission errors when opening the db gracefully.
The tool must be able to understand and parse fully qualified user names. Use the sss_parse_name() function for that, you can copy its usage from the sss_cache tool. If the FQ names support is good enough,
I
would consider dropping the -D parameter althogether.
done
I think the default set of attributes displayed for the user should be the same that getpwnam() returns. See the output of "getent passwd $user".
done
Currently the tool only prints the raw attribute names from ldb. Do you also plan on printing human readable names ("GID Number" instead of "gidNumber") ?
I redesigned all the prints, so now it id nicer for humans :)
Some entries might not have "gecos" at all. In that case, fall back to the cn value.
There seems to be a bug when displaying domain info: $ sudo ./sss_query -d -N ipaldap There is no domain "ipaldap"! <--- yes there is Domain name: ipaldap Domain provider: ldap Domain version: 0.15 Domain's subdomains: <--- diplayed twice Domain's subdomains: <---
The "Domain's subdomains" probably shouldn't be displayed at all if there are no subdomains.
I saw some weird behaviour when I had multiple domains configured. When I tried to query user from the first domain in the list sss_query first printed the user, but then said "Object not found".
Similarly when I wanted to query an object from the second domain, I
had
to use the -D option to specify the exact domain. I would expect the
tool
to iterate over the domains.
There was a bug that caused all this behavior, now it should be OK.
The messages that are user visible must be marked as translatable. See the definition of the ERROR() macro to see how to do that.
When the tool is ready, it's going to require a man page and it needs
to
be mentioned in sssd.spec.in so it's packaged in the RPMs correctly.
I made the man page and also put mention is sssd.spec.in.
Please ask your thesis mentor if you can keep the copyright assigned to yourself in the thesis. I vaguely remember I had to submit the
copyright
to FIT VUT back in the day :-)
In progress, mentor is busy and not responding at the moment :)
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks for all your advice and tips. I uploaded new version with new design, hopefully fixed all the bugs and added all the missing functionality. I will welcome all comments to new design or reports of mistakes.
Thanks David Bambušek
That code is much more better, after redesign code is approximately 35% shorter and much more readable.
In function print_object, you iterate over array of strings (user_attrs), foreach attr in user_attrs: int_value = get_value_from_hash(hash, attr) switch(int_value)
....... case A_<type>: oa->key = g_strdup(SYSDB_<type>); ^^^^^^^^^^^^ this is the same like a "attr" ret_value =
talloc_strdup(mem_ctx,ldb_msg_find_attr_as_string(msg, SYSDB_<type>, NULL));
^^^^^^^^^^^^
this is also "attr"
strcpy(final, "Name:\t "); ^^^^^^^^^ human readable version of "attr" break; ......
The purpose of hash_table was to reduce very long switch/case. Hash table
will
help you with mapping one type to another and code will be much more
simpler.
You can transform current hash table <string, int> (SYSDB_<type>,
A_<type>)
to hash table <string, string> (SYSDB_type, "human readable version of
SYSDB_<type>"
This approach should help you to remove switch/case from print_object
function.
OK, now I have finally understood how to use it correctly, thanks a lot, it is really a big line saver and much better solution than was mine.
Also function search_object is longer then 250 lines, so you could try to: -- split it to two functions search_object, search_object_all -- or you could try to reuse function parameter "bool all"
LS
Functions are now split into three, one for certain object, one for all objects and one for domains and subdomains.
I forgot to mention one important thing in previous email.
You have used hash table from glib, but glib is optional dependency in sssd. For example in debian distribution, sssd does not depend on glib(libglib). You should use hash table from ding-libs(libdhash)
Fedora distribution have package libdhash-devel and you can find exmaples in directory /usr/share/doc/libdhash-devel-0.4.3/
LS
Sorry for that, I used it by mistake, already corrected.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
So the newest version is already on my github, with all the issues mentioned above fixed.
David Bambušek
On (16/04/13 23:46), David Bambušek wrote:
2013/4/16 Lukas Slebodnik lslebodn@redhat.com
On (16/04/13 10:50), Lukas Slebodnik wrote:
On (16/04/13 00:12), David Bambušek wrote:
2013/4/12 Jakub Hrozek jhrozek@redhat.com
On Fri, Apr 12, 2013 at 06:42:03PM +0200, Lukas Slebodnik wrote:
On (12/04/13 01:31), David Bambušek wrote: >Since I was really working with two months old version of SSSD, I
made
>completely new mirror of the newest SSSD on my github account and
made
some
>changes in code of my sss_query, so it is compatible with it now.
There
>should be no problem with compiling anymore. > >I also went through the code and changed it according to coding
guidelines.
>About indentation, I use 4 spaces as coding style suggests, could
you
>please specify where do I have it wrong? I also wrapped most of
lines so
>they are not exceeding the 80 character limit, I just have kept
some of
>them longer, cause in my opinion they would become unreadable, if
wrapped,
>but still they are never longer than about 100 characters.
You spend much more time with reading code like writing code. So code
have to
be readable. 80 columns is not about terminal limitations, but about code
readability.
In most cases if you have line longer than 80 characters, you may
doing
something wrong. Maybe you try to do a lot of thing on one line, or
there are
a lot of nested blocks (if, while ...)
Some formatting issues, according to SSSD coding style: http://www.freeipa.org/page/Coding_Style --white spaces at end of line --mixing tabs and spaces I think that proper editor configuration could help you with both.
I corrected it, so it should be fine now.
>I also corrected the memory leaks and memory handling through out
the
>application, so hopefully it is alright now. > >David Bambušek >
And some comments to code itself.
For each type you define as #define <type>_SHOW_ATTRS { SYSDB_CONST_1, ... SYSDB_CONST_n, NULL } #define <type>_SHOW_ATTRS_NUM number ... static const char *attrs[] = <type>_SHOW_ATTRS;
--You have to manually define array size, for each ATTR, it is error
prone.
--You don't need to know array size, because each array is NULL
terminated,
So you can transform it to something like this:
// much more readable and shorter then 80 columns const char *SSHHOST_SHOW_ATTRS[] = { SYSDB_SSH_HOST_OC, SYSDB_SSH_KNOWN_HOSTS_EXPIRE, SYSDB_SSH_PUBKEY, NULL }; ... const char **attrs = SSHHOST_SHOW_ATTRS;
And instead of while iteration with counter, you can benefit from NULL terminated array. With this approach you can remove parameters "int <type>_attrs_num" from all functions.
- int counter = 0;
- while (counter < user_attrs_num){
if (strcmp(user_attrs[counter],SYSDB_NAME) == 0){
- for (attr_iter = user_attrs; attr_iter; ++attr_iter){
if (strcmp(*attr_iter, SYSDB_NAME) == 0){
That is clever, thanks for advice.
But most important think is that there is a lot of code duplication.
For
example If I decide to add/remove new attribute to SSHHOST_SHOW_ATTRS I have to: --add/remove SYSDB_CONST to SSHHOST_SHOW_ATTRS --add/remove member from struct sshhost_info --add/remove lines to search_user --add/remove lines to search_all_user (the same like in search
user)
--add/remove lines to print_user
It is also error prone.
You declare 6 structures: user_info, group_info, netgroup_info,
service_info
autofsm_info, sshhost_info
For each structure you create functions with the same pattern. print_<type>_info search_<type> search_all_<type>
Both search function have a lot of similar code. In search function you map <key, value> to structure members. In print function you map structure members to <key, value> and print formated <key, value>. This mapping is useless, because key and
value
are strings. It would be easier to use hash_table instead of
structures.
In that case, you can write general print, search, search_all
function
for all
types. SSSD already use hash_table from ding-libs package. Header
file
dhash.h
And at the end one positive: Output from this utility is quite good.
I completely redesigned the tools, made it more general and also used
hash
table.
In addition to Lukas' comments about the code I also wanted to comment
on
the functionality:
Currently the databases are only readable by root. You should either restrict the tool so that it errors out if not ran by root, or handle permission errors when opening the db gracefully.
The tool must be able to understand and parse fully qualified user names. Use the sss_parse_name() function for that, you can copy its usage from the sss_cache tool. If the FQ names support is good enough,
I
would consider dropping the -D parameter althogether.
done
I think the default set of attributes displayed for the user should be the same that getpwnam() returns. See the output of "getent passwd $user".
done
Currently the tool only prints the raw attribute names from ldb. Do you also plan on printing human readable names ("GID Number" instead of "gidNumber") ?
I redesigned all the prints, so now it id nicer for humans :)
Some entries might not have "gecos" at all. In that case, fall back to the cn value.
There seems to be a bug when displaying domain info: $ sudo ./sss_query -d -N ipaldap There is no domain "ipaldap"! <--- yes there is Domain name: ipaldap Domain provider: ldap Domain version: 0.15 Domain's subdomains: <--- diplayed twice Domain's subdomains: <---
The "Domain's subdomains" probably shouldn't be displayed at all if there are no subdomains.
I saw some weird behaviour when I had multiple domains configured. When I tried to query user from the first domain in the list sss_query first printed the user, but then said "Object not found".
Similarly when I wanted to query an object from the second domain, I
had
to use the -D option to specify the exact domain. I would expect the
tool
to iterate over the domains.
There was a bug that caused all this behavior, now it should be OK.
The messages that are user visible must be marked as translatable. See the definition of the ERROR() macro to see how to do that.
When the tool is ready, it's going to require a man page and it needs
to
be mentioned in sssd.spec.in so it's packaged in the RPMs correctly.
I made the man page and also put mention is sssd.spec.in.
Please ask your thesis mentor if you can keep the copyright assigned to yourself in the thesis. I vaguely remember I had to submit the
copyright
to FIT VUT back in the day :-)
In progress, mentor is busy and not responding at the moment :)
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks for all your advice and tips. I uploaded new version with new design, hopefully fixed all the bugs and added all the missing functionality. I will welcome all comments to new design or reports of mistakes.
Thanks David Bambušek
That code is much more better, after redesign code is approximately 35% shorter and much more readable.
In function print_object, you iterate over array of strings (user_attrs), foreach attr in user_attrs: int_value = get_value_from_hash(hash, attr) switch(int_value)
....... case A_<type>: oa->key = g_strdup(SYSDB_<type>); ^^^^^^^^^^^^ this is the same like a "attr" ret_value =
talloc_strdup(mem_ctx,ldb_msg_find_attr_as_string(msg, SYSDB_<type>, NULL));
^^^^^^^^^^^^
this is also "attr"
strcpy(final, "Name:\t "); ^^^^^^^^^ human readable version of "attr" break; ......
The purpose of hash_table was to reduce very long switch/case. Hash table
will
help you with mapping one type to another and code will be much more
simpler.
You can transform current hash table <string, int> (SYSDB_<type>,
A_<type>)
to hash table <string, string> (SYSDB_type, "human readable version of
SYSDB_<type>"
This approach should help you to remove switch/case from print_object
function.
OK, now I have finally understood how to use it correctly, thanks a lot, it is really a big line saver and much better solution than was mine.
Also function search_object is longer then 250 lines, so you could try to: -- split it to two functions search_object, search_object_all -- or you could try to reuse function parameter "bool all"
LS
Functions are now split into three, one for certain object, one for all objects and one for domains and subdomains.
I forgot to mention one important thing in previous email.
You have used hash table from glib, but glib is optional dependency in sssd. For example in debian distribution, sssd does not depend on glib(libglib). You should use hash table from ding-libs(libdhash)
Fedora distribution have package libdhash-devel and you can find exmaples in directory /usr/share/doc/libdhash-devel-0.4.3/
LS
Sorry for that, I used it by mistake, already corrected.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
So the newest version is already on my github, with all the issues mentioned above fixed.
David Bambušek
#include <stddef.h> #include <glib.h> ^^^ You forgot to remove header file #include "dhash.h"
...
if ( (entry_type == TYPE_DOMAIN) || (entry_type == TYPE_DOMAIN_ALL) || (entry_type == TYPE_SUBDOMAIN)) { print_domain_info(tctx, search, entry_type); } else if (!all) { search_object(tctx, hashed_attrs, tctx, object_type, entry_type, protocol, search, attribs, all, domain); ^^^^^ every time false You split search_object to two functions, so this argument is useless in this function and should be removed and also related code from function } else { search_object_all(tctx, hashed_attrs, tctx, object_type, entry_type, attribs, domain); }
After splitting search_object -> {search_object, search_object_all} enum sss_entry_type can be reduced by removing TYPE_<type>_ALL and ALL_SHIFT should be also removed.
We don't like compile time warnings: CC src/tools/sss_query-sss_query.o src/tools/sss_query.c: In function ‘setup_hash’: src/tools/sss_query.c:144:21: warning: assignment discards ‘const’ qualifier from pointer target type [enabled by default] src/tools/sss_query.c: In function ‘search_object’: src/tools/sss_query.c:389:29: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] src/tools/sss_query.c: In function ‘search_object_all’: src/tools/sss_query.c:524:25: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] src/tools/sss_query.c:412:28: warning: unused parameter ‘type’ [-Wunused-parameter] src/tools/sss_query.c: In function ‘delete_callback’: src/tools/sss_query.c:541:61: warning: unused parameter ‘type’ [-Wunused-parameter] src/tools/sss_query.c:541:73: warning: unused parameter ‘pvt’ [-Wunused-parameter] src/tools/sss_query.c: In function ‘parse_attributes’: src/tools/sss_query.c:556:22: warning: assignment from incompatible pointer type [enabled by default]
LS
On Tue, 2013-04-16 at 23:46 +0200, David Bambušek wrote:
So the newest version is already on my github, with all the issues mentioned above fixed.
Can you please squash commits ?
It is not interesting to see corrections and you need to squash them in reasonable patches for final submission anyway, better do it often and earlier IMO.
Simo.
On Fri, Apr 19, 2013 at 02:13:21PM -0400, Simo Sorce wrote:
On Tue, 2013-04-16 at 23:46 +0200, David Bambušek wrote:
So the newest version is already on my github, with all the issues mentioned above fixed.
Can you please squash commits ?
It is not interesting to see corrections and you need to squash them in reasonable patches for final submission anyway, better do it often and earlier IMO.
+1
It's worth noting that David stopped by the office when we had an Open House day this week and we went through the code in person. I think the tool mostly behaves good now, there are still some final touches needed, such as marking several strings as translatable and not naming variables in UPPERCASE even if they are static.
But the last iterations looked way better than the first ones, definitely a big improvement.
Dne 19.4.2013 21:08, Jakub Hrozek napsal(a):
On Fri, Apr 19, 2013 at 02:13:21PM -0400, Simo Sorce wrote:
On Tue, 2013-04-16 at 23:46 +0200, David Bambušek wrote:
So the newest version is already on my github, with all the issues mentioned above fixed.
Can you please squash commits ?
It is not interesting to see corrections and you need to squash them in reasonable patches for final submission anyway, better do it often and earlier IMO.
+1
It's worth noting that David stopped by the office when we had an Open House day this week and we went through the code in person. I think the tool mostly behaves good now, there are still some final touches needed, such as marking several strings as translatable and not naming variables in UPPERCASE even if they are static.
But the last iterations looked way better than the first ones, definitely a big improvement. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I hope I will be done with all the little things that need to be corrected by the end of this week. And sure, I will squash those commits ;)
David
I have made some final changes: - renamed those variables from upper case to lower case - used sss_hash_xxx function to create hash table, so there is less work with de-allocation now - I also included the intern name of sysdb attributes on output so now it looks like: <human_readable_attr> (<sysdb_attr>): <value> GID Number (gidNumber): 0123456789 - and finally there are no warnings during compilation
I squeezed all the commits into one and I made a patch, that is attached...
2013/4/20 David Bambušek bambusekd@gmail.com
Dne 19.4.2013 21:08, Jakub Hrozek napsal(a):
On Fri, Apr 19, 2013 at 02:13:21PM -0400, Simo Sorce wrote:
On Tue, 2013-04-16 at 23:46 +0200, David Bambušek wrote:
So the newest version is already on my github, with all the issues mentioned above fixed.
Can you please squash commits ?
It is not interesting to see corrections and you need to squash them in reasonable patches for final submission anyway, better do it often and earlier IMO.
+1
It's worth noting that David stopped by the office when we had an Open House day this week and we went through the code in person. I think the tool mostly behaves good now, there are still some final touches needed, such as marking several strings as translatable and not naming variables in UPPERCASE even if they are static.
But the last iterations looked way better than the first ones, definitely a big improvement. ______________________________**_________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.**org sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.**org/mailman/listinfo/sssd-**develhttps://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I hope I will be done with all the little things that need to be corrected by the end of this week. And sure, I will squash those commits ;)
David
Hello all agian, I am attaching text of my bachelor work, if anyone of you would be interested in reading it and giving any comments or recommendations, I would be just glad. First chapters will probably not be very interesting for you, so in case you are gonna read it, just skip to chapter about SSSD and read further.
Best regards David
2013/4/22 David Bambušek bambusekd@gmail.com
I have made some final changes:
- renamed those variables from upper case to lower case
- used sss_hash_xxx function to create hash table, so there is less work
with de-allocation now
- I also included the intern name of sysdb attributes on output so now it
looks like: <human_readable_attr> (<sysdb_attr>): <value> GID Number (gidNumber): 0123456789
- and finally there are no warnings during compilation
I squeezed all the commits into one and I made a patch, that is attached...
2013/4/20 David Bambušek bambusekd@gmail.com
Dne 19.4.2013 21:08, Jakub Hrozek napsal(a):
On Fri, Apr 19, 2013 at 02:13:21PM -0400, Simo Sorce wrote:
On Tue, 2013-04-16 at 23:46 +0200, David Bambušek wrote:
So the newest version is already on my github, with all the issues mentioned above fixed.
Can you please squash commits ?
It is not interesting to see corrections and you need to squash them in reasonable patches for final submission anyway, better do it often and earlier IMO.
+1
It's worth noting that David stopped by the office when we had an Open House day this week and we went through the code in person. I think the tool mostly behaves good now, there are still some final touches needed, such as marking several strings as translatable and not naming variables in UPPERCASE even if they are static.
But the last iterations looked way better than the first ones, definitely a big improvement. ______________________________**_________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.**org sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.**org/mailman/listinfo/sssd-**develhttps://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I hope I will be done with all the little things that need to be corrected by the end of this week. And sure, I will squash those commits ;)
David
On 04/23/2013 05:35 PM, David Bambušek wrote:
Hello all agian, I am attaching text of my bachelor work, if anyone of you would be interested in reading it and giving any comments or recommendations, I would be just glad. First chapters will probably not be very interesting for you, so in case you are gonna read it, just skip to chapter about SSSD and read further.
I scanned through it pretty quickly. 1) There is a typo on page 28 "autofsm maps" 2) I am not sure I like ">" at the beginning of the output 3) What is the result when the -N and -uid are provided at the same time and they do not match? Who wins? Or it is an error? 4) In general having something on return codes, errors, logging would be beneficial 5) What about the permissions and SELinux? Anything should be done/described there?
Thanks Dmitri
Best regards David
2013/4/22 David Bambušek <bambusekd@gmail.com mailto:bambusekd@gmail.com>
I have made some final changes: - renamed those variables from upper case to lower case - used sss_hash_xxx function to create hash table, so there is less work with de-allocation now - I also included the intern name of sysdb attributes on output so now it looks like: <human_readable_attr> (<sysdb_attr>): <value> GID Number (gidNumber): 0123456789 - and finally there are no warnings during compilation I squeezed all the commits into one and I made a patch, that is attached... 2013/4/20 David Bambušek <bambusekd@gmail.com <mailto:bambusekd@gmail.com>> Dne 19.4.2013 21:08, Jakub Hrozek napsal(a): On Fri, Apr 19, 2013 at 02:13:21PM -0400, Simo Sorce wrote: On Tue, 2013-04-16 at 23:46 +0200, David Bambušek wrote: So the newest version is already on my github, with all the issues mentioned above fixed. Can you please squash commits ? It is not interesting to see corrections and you need to squash them in reasonable patches for final submission anyway, better do it often and earlier IMO. +1 It's worth noting that David stopped by the office when we had an Open House day this week and we went through the code in person. I think the tool mostly behaves good now, there are still some final touches needed, such as marking several strings as translatable and not naming variables in UPPERCASE even if they are static. But the last iterations looked way better than the first ones, definitely a big improvement. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org <mailto:sssd-devel@lists.fedorahosted.org> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel I hope I will be done with all the little things that need to be corrected by the end of this week. And sure, I will squash those commits ;) David
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks for comment 1) corrected, thanks for noticing 2) that ">" is not displayed on the real output, I just put it into text so it is clear what is the output, but you are right that it looks weird, so I removed it 3,4) you can use just one of them, otherwise it is an error. I added a section where all the errors are enumerated 5) I might add a section on this topic, thanks for recommendation
Actualized version attached...
David
2013/4/24 Dmitri Pal dpal@redhat.com
On 04/23/2013 05:35 PM, David Bambušek wrote:
Hello all agian, I am attaching text of my bachelor work, if anyone of you would be interested in reading it and giving any comments or recommendations, I would be just glad. First chapters will probably not be very interesting for you, so in case you are gonna read it, just skip to chapter about SSSD and read further.
I scanned through it pretty quickly.
- There is a typo on page 28 "autofsm maps"
- I am not sure I like ">" at the beginning of the output
- What is the result when the -N and -uid are provided at the same time
and they do not match? Who wins? Or it is an error? 4) In general having something on return codes, errors, logging would be beneficial 5) What about the permissions and SELinux? Anything should be done/described there?
Thanks Dmitri
Best regards David
2013/4/22 David Bambušek bambusekd@gmail.com
I have made some final changes:
- renamed those variables from upper case to lower case
- used sss_hash_xxx function to create hash table, so there is less work
with de-allocation now
- I also included the intern name of sysdb attributes on output so now
it looks like: <human_readable_attr> (<sysdb_attr>): <value> GID Number (gidNumber): 0123456789
- and finally there are no warnings during compilation
I squeezed all the commits into one and I made a patch, that is attached...
2013/4/20 David Bambušek bambusekd@gmail.com
Dne 19.4.2013 21:08, Jakub Hrozek napsal(a):
On Fri, Apr 19, 2013 at 02:13:21PM -0400, Simo Sorce wrote:
On Tue, 2013-04-16 at 23:46 +0200, David Bambušek wrote:
So the newest version is already on my github, with all the issues mentioned above fixed.
Can you please squash commits ?
It is not interesting to see corrections and you need to squash them in reasonable patches for final submission anyway, better do it often and earlier IMO.
+1
It's worth noting that David stopped by the office when we had an Open House day this week and we went through the code in person. I think the tool mostly behaves good now, there are still some final touches needed, such as marking several strings as translatable and not naming variables in UPPERCASE even if they are static.
But the last iterations looked way better than the first ones, definitely a big improvement. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I hope I will be done with all the little things that need to be corrected by the end of this week. And sure, I will squash those commits ;)
David
sssd-devel mailing listsssd-devel@lists.fedorahosted.orghttps://lists.fedorahosted.org/mailman/listinfo/sssd-devel
-- Thank you, Dmitri Pal
Sr. Engineering Manager for IdM portfolio Red Hat Inc.
Looking to carve out IT costs?www.redhat.com/carveoutcosts/
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Mon, Apr 29, 2013 at 03:18:38PM +0200, David Bambušek wrote:
Thanks for comment
- corrected, thanks for noticing
- that ">" is not displayed on the real output, I just put it into text
so it is clear what is the output, but you are right that it looks weird, so I removed it 3,4) you can use just one of them, otherwise it is an error. I added a section where all the errors are enumerated 5) I might add a section on this topic, thanks for recommendation
Actualized version attached...
David
David, I think you attached the thesis not the patch by accident.
I should have checked the message before approving it too, my fault.
I attached the thesis on purpose, patch did not change from the last time I posted it here. But nevertheless, patch is attached to this email. Sorry for confusion.
2013/4/29 Jakub Hrozek jhrozek@redhat.com
On Mon, Apr 29, 2013 at 03:18:38PM +0200, David Bambušek wrote:
Thanks for comment
- corrected, thanks for noticing
- that ">" is not displayed on the real output, I just put it into text
so it is clear what is the output, but you are right that it looks weird, so I removed it 3,4) you can use just one of them, otherwise it is an error. I added a section where all the errors are enumerated 5) I might add a section on this topic, thanks for recommendation
Actualized version attached...
David
David, I think you attached the thesis not the patch by accident.
I should have checked the message before approving it too, my fault. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On 04/29/2013 09:18 AM, David Bambušek wrote:
Thanks for comment
- corrected, thanks for noticing
- that ">" is not displayed on the real output, I just put it into
text so it is clear what is the output, but you are right that it looks weird, so I removed it 3,4) you can use just one of them, otherwise it is an error. I added a section where all the errors are enumerated
I noticed you have multiple formatting issues in the newly added errors section. The section seems OK from the contents POW.
- I might add a section on this topic, thanks for recommendation
Actualized version attached...
David
2013/4/24 Dmitri Pal <dpal@redhat.com mailto:dpal@redhat.com>
On 04/23/2013 05:35 PM, David Bambušek wrote:
Hello all agian, I am attaching text of my bachelor work, if anyone of you would be interested in reading it and giving any comments or recommendations, I would be just glad. First chapters will probably not be very interesting for you, so in case you are gonna read it, just skip to chapter about SSSD and read further.
I scanned through it pretty quickly. 1) There is a typo on page 28 "autofsm maps" 2) I am not sure I like ">" at the beginning of the output 3) What is the result when the -N and -uid are provided at the same time and they do not match? Who wins? Or it is an error? 4) In general having something on return codes, errors, logging would be beneficial 5) What about the permissions and SELinux? Anything should be done/described there? Thanks Dmitri
Best regards David 2013/4/22 David Bambušek <bambusekd@gmail.com <mailto:bambusekd@gmail.com>> I have made some final changes: - renamed those variables from upper case to lower case - used sss_hash_xxx function to create hash table, so there is less work with de-allocation now - I also included the intern name of sysdb attributes on output so now it looks like: <human_readable_attr> (<sysdb_attr>): <value> GID Number (gidNumber): 0123456789 - and finally there are no warnings during compilation I squeezed all the commits into one and I made a patch, that is attached... 2013/4/20 David Bambušek <bambusekd@gmail.com <mailto:bambusekd@gmail.com>> Dne 19.4.2013 21:08, Jakub Hrozek napsal(a): On Fri, Apr 19, 2013 at 02:13:21PM -0400, Simo Sorce wrote: On Tue, 2013-04-16 at 23:46 +0200, David Bambušek wrote: So the newest version is already on my github, with all the issues mentioned above fixed. Can you please squash commits ? It is not interesting to see corrections and you need to squash them in reasonable patches for final submission anyway, better do it often and earlier IMO. +1 It's worth noting that David stopped by the office when we had an Open House day this week and we went through the code in person. I think the tool mostly behaves good now, there are still some final touches needed, such as marking several strings as translatable and not naming variables in UPPERCASE even if they are static. But the last iterations looked way better than the first ones, definitely a big improvement. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org <mailto:sssd-devel@lists.fedorahosted.org> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel I hope I will be done with all the little things that need to be corrected by the end of this week. And sure, I will squash those commits ;) David _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org <mailto:sssd-devel@lists.fedorahosted.org> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
-- Thank you, Dmitri Pal Sr. Engineering Manager for IdM portfolio Red Hat Inc. ------------------------------- Looking to carve out IT costs? www.redhat.com/carveoutcosts/ <http://www.redhat.com/carveoutcosts/> _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org <mailto:sssd-devel@lists.fedorahosted.org> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
David, please consult the contents of your tech. report primarily with me, as I'm the contact person for this. It might be good for other guys just in case they are curious but please don't expect them to write any kind of extensive feedback, as they are very busy with other work.
Thank you for understanding Jan
On 29. 4. 2013 at 15:18:38, David Bambušek wrote:
Thanks for comment
- corrected, thanks for noticing
- that ">" is not displayed on the real output, I just put it into text
so it is clear what is the output, but you are right that it looks weird, so I removed it 3,4) you can use just one of them, otherwise it is an error. I added a section where all the errors are enumerated 5) I might add a section on this topic, thanks for recommendation
Actualized version attached...
David
2013/4/24 Dmitri Pal dpal@redhat.com
On 04/23/2013 05:35 PM, David Bambušek wrote: Hello all agian,
I am attaching text of my bachelor work, if anyone of you would be
interested in reading it and giving any comments or recommendations, I would be just glad. First chapters will probably not be very interesting for you, so in case you are gonna read it, just skip to chapter about SSSD and read further.
I scanned through it pretty quickly.
- There is a typo on page 28 "autofsm maps"
- I am not sure I like ">" at the beginning of the output
- What is the result when the -N and -uid are provided at the same time
and they do not match? Who wins? Or it is an error? 4) In general having something on return codes, errors, logging would be beneficial 5) What about the permissions and SELinux? Anything should be done/described there?
Thanks Dmitri
Best regards David
2013/4/22 David Bambušek bambusekd@gmail.com
I have made some final changes:
- renamed those variables from upper case to lower case
- used sss_hash_xxx function to create hash table, so there is less work
with de-allocation now
- I also included the intern name of sysdb attributes on output so now
it looks like: <human_readable_attr> (<sysdb_attr>): <value> GID Number (gidNumber): 0123456789
- and finally there are no warnings during compilation
I squeezed all the commits into one and I made a patch, that is
attached...
2013/4/20 David Bambušek bambusekd@gmail.com
Dne 19.4.2013 21:08, Jakub Hrozek napsal(a): On Fri, Apr 19, 2013 at 02:13:21PM -0400, Simo Sorce wrote:
On Tue, 2013-04-16 at 23:46 +0200, David Bambušek wrote: > So the newest version is already on my github, with all the issues > mentioned above fixed. > > Can you please squash commits ?
It is not interesting to see corrections and you need to squash them in reasonable patches for final submission anyway, better do it often and earlier IMO.
+1
It's worth noting that David stopped by the office when we had an Open House day this week and we went through the code in person. I think the tool mostly behaves good now, there are still some final touches needed, such as marking several strings as translatable and not naming variables in UPPERCASE even if they are static.
But the last iterations looked way better than the first ones, definitely a big improvement. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
I hope I will be done with all the little things that need to be
corrected by the end of this week. And sure, I will squash those commits ;)
David
On 04/16/2013 12:12 AM, David Bambušek wrote:
Please ask your thesis mentor if you can keep the copyright assigned to yourself in the thesis. I vaguely remember I had to submit the copyright to FIT VUT back in the day :-)
In progress, mentor is busy and not responding at the moment :)
Looking at my thesis, and trying to remember the process, the university owns copyright for your thesis, but just the theoretical part, you own the source code, so the copyright seems placed correctly.
Ondra
2013/4/16 Ondrej Kos okos@redhat.com
On 04/16/2013 12:12 AM, David Bambušek wrote:
Please ask your thesis mentor if you can keep the copyright assigned
to yourself in the thesis. I vaguely remember I had to submit the copyright to FIT VUT back in the day :-)
In progress, mentor is busy and not responding at the moment :)
Looking at my thesis, and trying to remember the process, the university owns copyright for your thesis, but just the theoretical part, you own the source code, so the copyright seems placed correctly.
Ondra
-- Ondrej Kos Associate Software Engineer Identity Management Red Hat Czech
______________________________**_________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.**org sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.**org/mailman/listinfo/sssd-**develhttps://lists.fedorahosted.org/mailman/listinfo/sssd-devel
OK, thanks, but still I will rather double check it with my mentor, just to be sure ;)
David
sssd-devel@lists.fedorahosted.org