-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-openlmi.rhcloud.com/r/1699/#review2622
-----------------------------------------------------------
Overall, I like the code, it really well crafted. See some remarks below.
However, I see some higher-level problems:
1) The code is GPLv3. OpenLMI is LGPL v2.1 and we won't accept GPL provider. Please
relicense the provider LGPL2.1. As a consequence, you must also relicense libsss_simpleifp
to LGPL too, as the provider includes /usr/include/sss_dbus.h. Looking at dhash.h, you
already use LGPLv3 for libraries (mixing LGPL2.1 provider with LGPLv3 library is fine,
mixing LGPL with GPL is not).
2) There are no tests. We as OpenLMI can maintain the provider, however to ensure its
functionality we need a test suite. It can be fully blown SSSD installation which we can
test against, or a dummy DBus service which would simulate SSSD's DBus interface
(strongly preferred). We can help with the CIM side of the test suite, we can script CIM
pretty well, however we have no idea how to set up and test SSSD (e.g. check, that debug
logging was really set).
src/sssd/LMI_SSSDBackendProvider.c
<
http://reviewboard-openlmi.rhcloud.com/r/1699/#comment1440>
CMPI_RC_ERR_NOT_FOUND is more appropriate here
src/sssd/LMI_SSSDBackendProviderProvider.c
<
http://reviewboard-openlmi.rhcloud.com/r/1699/#comment1441>
Use CMPI_RC_ERR_ codes when returning stuff from CMPI callback (applies to whole
module)
src/sssd/LMI_SSSDComponentProvider.c
<
http://reviewboard-openlmi.rhcloud.com/r/1699/#comment1442>
This whole file looks useless, did I miss something?
src/sssd/LMI_SSSDDomainProvider.c
<
http://reviewboard-openlmi.rhcloud.com/r/1699/#comment1443>
use ERR_NOT_FOUND
src/sssd/LMI_SSSDDomainSubdomainProvider.c
<
http://reviewboard-openlmi.rhcloud.com/r/1699/#comment1445>
error is not checked?
src/sssd/LMI_SSSDDomainSubdomainProvider.c
<
http://reviewboard-openlmi.rhcloud.com/r/1699/#comment1444>
key.str is not freed on error
src/sssd/LMI_SSSDDomainSubdomainProvider.c
<
http://reviewboard-openlmi.rhcloud.com/r/1699/#comment1446>
Umm, should it throw an error? Or the function can safely continue with
'faulty' hashtable?
src/sssd/LMI_SSSDMonitorProvider.c
<
http://reviewboard-openlmi.rhcloud.com/r/1699/#comment1447>
again, use ERR_NOT_FOUND
src/sssd/LMI_SSSDProviderProvider.c
<
http://reviewboard-openlmi.rhcloud.com/r/1699/#comment1448>
This looks suspicious. What is purpose of this class? MOF file does not show any
information.
In addition, it's GetInstance() just returns whatever it got, without any
checking... You should check if the instance exists and throw ERR_NOT_FOUND otherwise.
src/sssd/LMI_SSSDResponderProvider.c
<
http://reviewboard-openlmi.rhcloud.com/r/1699/#comment1449>
again, use ERR_NOT_FOUND
src/sssd/utils.h
<
http://reviewboard-openlmi.rhcloud.com/r/1699/#comment1439>
I have found only one usage of dhash, LMI_SSSDDomainSubdomainProvider.c
Is it worth adding new dependency to OpenLMI? We already depend on glib and it
provides it's own hash table implementation. SSSDDomainSubdomainProvider look like it
needs just small temporary storage and glib/dhash IMO won't make make any difference
here.
https://developer.gnome.org/glib/2.30/glib-Hash-Tables.html
src/sssd/utils.h
<
http://reviewboard-openlmi.rhcloud.com/r/1699/#comment1438>
Please remove this,
1) we use C99, true/false are keywords there
2) true/false are not even used in your sources :)
- Jan Safranek
On May 14, 2014, 10:55 a.m., Pavel Březina wrote:
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-openlmi.rhcloud.com/r/1699/
-----------------------------------------------------------
(Updated May 14, 2014, 10:55 a.m.)
Review request for OpenLMI Developers.
Repository: openlmi-providers
Description
-------
This is the OpenLMI SSSD provider.
It is yet dependent on an unreleased version of SSSD and even on patches that are not
present in SSSD master branch. We are slowly reviewing and push these patches to the
master. In the mean time, please use the SSSD from my repository, branch #openlmi:
http://fedorapeople.org/cgit/pbrezina/public_git/sssd.git/log/?h=openlmi
The IFP service has to be enabled in SSSD for the provider to be able to communicate with
the daemon.
Diffs
-----
CMakeLists.txt bcea09707d1d87db374cf6561d00a19258b54b3b
README 3466aee84a57e142256fd77188aa5c87ee3167f5
doc/admin/CMakeLists.txt 96e7616e2af807d99b4afe7af81571367e15f286
doc/admin/sssd/index.rst PRE-CREATION
mof/60_LMI_SSSD.mof PRE-CREATION
mof/CMakeLists.txt 0c020a18517ecc469c70a93a6b7b824e0bca3117
openlmi-providers.spec 624ba7711a0836f37143cb16187ff548a647328f
src/CMakeLists.txt e0fbb4e05236fc10164f359b5a13f6406aacbda1
src/sssd/90_LMI_SSSD_Profile.mof.skel PRE-CREATION
src/sssd/CMakeLists.txt PRE-CREATION
src/sssd/LMI_HostedSSSDServiceProvider.c PRE-CREATION
src/sssd/LMI_SSSDAvailableComponentProvider.c PRE-CREATION
src/sssd/LMI_SSSDAvailableDomainProvider.c PRE-CREATION
src/sssd/LMI_SSSDBackendDomainProvider.c PRE-CREATION
src/sssd/LMI_SSSDBackendProvider.c PRE-CREATION
src/sssd/LMI_SSSDBackendProviderProvider.c PRE-CREATION
src/sssd/LMI_SSSDComponentProvider.c PRE-CREATION
src/sssd/LMI_SSSDDomainProvider.c PRE-CREATION
src/sssd/LMI_SSSDDomainSubdomainProvider.c PRE-CREATION
src/sssd/LMI_SSSDMonitorProvider.c PRE-CREATION
src/sssd/LMI_SSSDProviderProvider.c PRE-CREATION
src/sssd/LMI_SSSDResponderProvider.c PRE-CREATION
src/sssd/LMI_SSSDServiceProvider.c PRE-CREATION
src/sssd/cmpiLMI_SSSD-cimprovagt PRE-CREATION
src/sssd/sssd_components.h PRE-CREATION
src/sssd/sssd_components.c PRE-CREATION
src/sssd/sssd_domains.h PRE-CREATION
src/sssd/sssd_domains.c PRE-CREATION
src/sssd/utils.h PRE-CREATION
src/sssd/utils.c PRE-CREATION
Diff:
http://reviewboard-openlmi.rhcloud.com/r/1699/diff/
Testing
-------
Thanks,
Pavel Březina