Tool for querying SSSD database

[David Bambušek]

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

[Jakub Hrozek]

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

[David Bambušek]

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:
>
>  1) I suggest to read the thread at:
>  https://lists.fedorahosted.org/pipermail/sssd-devel/2012-February/008712....
>  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.
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[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:
>
>  1) I suggest to read the thread at:
>  https://lists.fedorahosted.org/pipermail/sssd-devel/2012-February/008712....
>  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.
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)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

[Jakub Hrozek]

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:
>  >
>  >1) I suggest to read the thread at:
>  >https://lists.fedorahosted.org/pipermail/sssd-devel/2012-February/008712.html
>  >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.
>  >_______________________________________________
>  >sssd-devel mailing list
>  >sssd-devel(a)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.

[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:
>
>  1) I suggest to read the thread at:
>  https://lists.fedorahosted.org/pipermail/sssd-devel/2012-February/008712....
>  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.
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)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

[Jakub Hrozek]

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:
>  >
>  >1) I suggest to read the thread at:
>  >https://lists.fedorahosted.org/pipermail/sssd-devel/2012-February/008712.html
>  >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.
>  >_______________________________________________
>  >sssd-devel mailing list
>  >sssd-devel(a)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.

[David Bambušek]

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(a)redhat.com&gt;

>  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:
>  > >
>  > >1) I suggest to read the thread at:
>  > >
>  https://lists.fedorahosted.org/pipermail/sssd-devel/2012-February/008712....
>  > >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.
>  > >_______________________________________________
>  > >sssd-devel mailing list
>  > &gt;sssd-devel(a)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(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>

[Lukas Slebodnik]

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

[Jakub Hrozek]

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

[David Bambušek]

2013/4/12 Jakub Hrozek <jhrozek(a)redhat.com&gt;

>  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(a)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

[Lukas Slebodnik]

On (16/04/13 00:12), David Bambušek wrote:
> 2013/4/12 Jakub Hrozek <jhrozek(a)redhat.com&gt;
>
> > 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(a)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

[Lukas Slebodnik]

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(a)redhat.com&gt;
> >
> >> 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(a)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

[David Bambušek]

2013/4/16 Lukas Slebodnik <lslebodn(a)redhat.com&gt;

>  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(a)redhat.com&gt;
>  >>
>  >>> 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(a)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(a)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

[Lukas Slebodnik]

On (16/04/13 23:46), David Bambušek wrote:
> 2013/4/16 Lukas Slebodnik <lslebodn(a)redhat.com&gt;
>
> > 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(a)redhat.com&gt;
> > >>
> > >>> 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(a)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(a)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

[Simo Sorce]

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.

-- 
Simo Sorce * Red Hat, Inc * New York

[Jakub Hrozek]

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.

[David Bambušek]

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(a)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

[David Bambušek]

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(a)gmail.com&gt;

>  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(a)lists.fedorahosted.**org <sssd-devel(a)lists.fedorahosted.org&gt;
> >
> https://lists.fedorahosted.**org/mailman/listinfo/sssd-**devel<https:/...
> >
>  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
>

[David Bambušek]

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(a)gmail.com&gt;

>  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(a)gmail.com&gt;
>
> > 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(a)lists.fedorahosted.**org <sssd-devel(a)lists.fedorahosted.org&gt;
> >>
> https://lists.fedorahosted.**org/mailman/listinfo/sssd-**devel<https:/...
> >>
> > 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
> >
>
>

[Dmitri Pal]

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(a)gmail.com
>  <mailto:[e-mail]>>
>
>      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(a)gmail.com
>      <mailto:[e-mail]>>
>
>          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(a)lists.fedorahosted.org
>              <mailto:[e-mail]>
>              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(a)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/

[David Bambušek]

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(a)redhat.com&gt;

>   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(a)gmail.com&gt;
>
> >     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(a)gmail.com&gt;
> >
> >> 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(a)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
> [e-mail]://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(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
>

[Jakub Hrozek]

On Mon, Apr 29, 2013 at 03:18:38PM +0200, David Bambušek wrote:
>  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 
David, I think you attached the thesis not the patch by accident.

I should have checked the message before approving it too, my fault.

[David Bambušek]

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(a)redhat.com&gt;

>  On Mon, Apr 29, 2013 at 03:18:38PM +0200, David Bambušek wrote:
>  > 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
>
>  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(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>

[Dmitri Pal]

On 04/29/2013 09:18 AM, David Bambušek wrote:
>  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 
I noticed you have multiple formatting issues in the newly added errors
section. The section seems OK from the contents POW.

>  5) I might add a section on this topic, thanks for recommendation
>
>  Actualized version attached...
>
>  David
>
>
>  2013/4/24 Dmitri Pal <dpal(a)redhat.com <mailto:[e-mail]>>
>
>      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(a)gmail.com
> >     <mailto:[e-mail]>>
> >
> >         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(a)gmail.com
> >         <mailto:[e-mail]>>
> >
> >             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(a)lists.fedorahosted.org
> >                 <mailto:[e-mail]>
> >                 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(a)lists.fedorahosted.org
> <mailto:[e-mail]>
> >     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(a)lists.fedorahosted.org
>      <mailto:[e-mail]>
>      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/

[Jan Zelený]

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
>  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(a)redhat.com&gt;
>  
>  >  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(a)gmail.com&gt;
>  > 
>  >>     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(a)gmail.com&gt;
>  >> 
>  >>> 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(a)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

[Ondrej Kos]

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

[David Bambušek]

2013/4/16 Ondrej Kos <okos(a)redhat.com&gt;

>  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(a)lists.fedorahosted.**org <sssd-devel(a)lists.fedorahosted.org&gt;
>
> https://lists.fedorahosted.**org/mailman/listinfo/sssd-**devel<https:/...
>  
OK, thanks, but still I will rather double check it with my mentor, just to
be sure ;)

David