<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
On 09/26/2012 02:40 PM, Dmitri Pal wrote:
<blockquote cite="mid:50634C02.8030906@redhat.com" type="cite">
<pre wrap="">Hi,
And again the same set + one patch that fixes Makefile.am
</pre>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
sssd-devel mailing list
<a class="moz-txt-link-abbreviated" href="mailto:sssd-devel@lists.fedorahosted.org">sssd-devel@lists.fedorahosted.org</a>
<a class="moz-txt-link-freetext" href="https://lists.fedorahosted.org/mailman/listinfo/sssd-devel">https://lists.fedorahosted.org/mailman/listinfo/sssd-devel</a>
</pre>
</blockquote>
<pre>Gunther provided a review and found two issues.</pre>
<pre>--------------------------------------------------
</pre>
<pre>In ini/ini_get_valueobj.c:ini_get_config_valueobj() line 132 the
function returns "error" which is still set to EOK while I think EINVAL
is more appropriate here:
</pre>
<pre>
ini/ini_get_valueobj.c | 2 +-
</pre>
<pre> 1 file changed, 1 insertion(+), 1 deletion(-)
</pre>
<pre>
diff --git a/ini/ini_get_valueobj.c b/ini/ini_get_valueobj.c
</pre>
<pre>index 6ed822e..6587e1d 100644
</pre>
<pre>--- a/ini/ini_get_valueobj.c
</pre>
<pre>+++ b/ini/ini_get_valueobj.c
</pre>
<pre>@@ -129,7 +129,7 @@ int ini_get_config_valueobj(const char *section,
</pre>
<pre> /* Do we have a name ? */
</pre>
<pre> if (name == NULL) {
</pre>
<pre> TRACE_ERROR_NUMBER("Name is NULL it will not be found.", EINVAL);
</pre>
<pre>- return error;
</pre>
<pre>+ return EINVAL;
</pre>
<pre> }
</pre>
<pre>
/* Empty section means look for the default one */
</pre>
<pre>
</pre>
<pre>Then I think there are some pointer dereferences, which theoretically
can be NULL pointers and there are no explicit checks, like here in
</pre>
<pre>ini/ini_list_valueobj.c:
</pre>
<pre>
ini/ini_list_valueobj.c | 2 +-
</pre>
<pre> 1 file changed, 1 insertion(+), 1 deletion(-)
</pre>
<pre>
diff --git a/ini/ini_list_valueobj.c b/ini/ini_list_valueobj.c
</pre>
<pre>index ef53e10..dc0992f 100644
</pre>
<pre>--- a/ini/ini_list_valueobj.c
</pre>
<pre>+++ b/ini/ini_list_valueobj.c
</pre>
<pre>@@ -116,7 +116,7 @@ char **ini_get_attribute_list(struct ini_cfgobj
*ini_config,
</pre>
<pre>
/* Pass it to the function from collection API */
</pre>
<pre> list = col_collection_to_list(subcollection, size, error);
</pre>
<pre>-
</pre>
<pre>+ /* cant list be NULL at this point ? */
</pre>
<pre> col_destroy_collection(subcollection);
</pre>
<pre>
/* Our list of attributes has a special extra attribute - remove it */
</pre>
<pre>
--------------------------------------------------------------------
Both are in patch 6.
At first I was not sure about the first issue but after a discussion with Stephen I decided to make a change.
The new patch set together with the coverity fix that was sent separately is included.
</pre>
<pre>
</pre>
<pre class="moz-signature" cols="72">--
Thank you,
Dmitri Pal
Sr. Engineering Manager for IdM portfolio
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
<a class="moz-txt-link-abbreviated" href="http://www.redhat.com/carveoutcosts/">www.redhat.com/carveoutcosts/</a>
</pre>
</body>
</html>