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