-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 09/03/2010 08:11 AM, Jan Zelený wrote:
> 0001-Dead-assignments-cleanup-in-providers-code.patch:
> Here the biggest change is in prototype of sdap_access_decide_offline, which
> IMO
> doesn't need to return any value, since it returns alway EOK now and there is
> no code which could be failing.
Nack
The change in child_common is wrong. The assignment of ret = errno is
intentional, because ret was supposed to be used in the DEBUG statement
below, rather than errno. It's supposed to be a protection in case the
DEBUG macro ever changes to something that might reset errno internally.
So the real bug here is that we are printing errno instead of ret.
Also:
../src/providers/ldap/ldap_id_enum.c: In function ‘enum_groups_send’:
../src/providers/ldap/ldap_id_enum.c:524: warning: unused variable
‘attr_name’
if (be_is_offline(state->be_ctx)) {
/* Ok, we're offline. Return from the cache */
- - ret = sdap_access_decide_offline(req);
+ sdap_access_decide_offline(req);
goto finished;
}
Please add ret = EOK before 'goto finished'. While it just happens to be
true right now, I'd rather we kept this explicit here, in case we ever
add something above this that changes ret.
> 0002-Dead-assignments-cleanup-in-NSS-responder.patch
Ack.
> 0003-Dead-assignments-cleanup-in-memberof-module.patch:
> Here I'm not entirely sure about return code LDB_ERR_OPERATIONS_ERROR. The
> variable ret can contain either 0 or -1 (in case of insufficient memory, I
> checked the ldb code). I think returning -1 further isn't the right choice,
> thus either ENOMEM or LDB_ERR_OPERATIONS_ERR seem to be acceptable. I chose
> the latter one, because it indicates generic ldb error. ENOMEM could be
> convenient now, but in the future the behavior of ldb_schema_attribute_add
> might change and ENOMEM would become misleading.
>
Ack.
LDB_ERR_OPERATIONS_ERROR is the right response here. We have to pass
back an LDB_ERR_* error code or the module loader won't know what to do
with it. Since LDB_ERR_OPERATIONS_ERROR is a generic error, it should
suffice even if ldb_schema_attribute_add changes in the future.
Using ENOMEM would be dangerous, as it overlaps with
LDB_ERR_CONFIDENTIALITY_REQUIRED and would result in significant
debugging confusion.
> 0004-Dead-assignments-cleanup-in-various-places-in-SSSD.patch
> The last hunk should be fixing this: http://jhrozek.fedorapeople.org/sssd-
> clang-llvm/report-G4QCn7.html#EndPath . IMO the original code had a flaw
> there,
> which would make the inner while cycle useless in case write() actually wrote
> less bytes than it could. This way it should be fixed.
Ack.
- --
Stephen Gallagher
RHCE 804006346421761
Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
iEYEARECAAYFAkyBRRgACgkQeiVVYja6o6N2YACgjwmUaQgUDUdRzK52Izio7rUO
7boAnRhQc6lNUtMUsrqFfSQERZKuBjYa
=0pRx
-----END PGP SIGNATURE-----
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
