<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
  <head>
    <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#ffffff" text="#000000">
    On 03/19/2012 04:39 PM, Stephen Gallagher wrote:
    <blockquote
      cite="mid:1332189593.12919.9.camel@sgallagh520.sgallagh.bos.redhat.com"
      type="cite">
      <pre wrap="">Patch 0001: Ack

Patch 0002: Ack

Patch 0003: Ack

Patch 0004: Ack, but it would be more efficient (maintenance-wise) to
just memset() the struct instead of setting the individual elements to
zero.

Patch 0005: Ack

Patch 0006: Ack

Patch 0007: Ack

Patch 0008: Ack

Patch 0009: Ack

Patch 0010: Ack

Patch 0011: Ack

Patch 0012: Ack

Patch 0013: Ack

Patch 0014: Ack

Patch 0015: Nack (minor). I can fix this when I push, but please don't
include trailing spaces after the case directives.

</pre>
    </blockquote>
    <br>
    Thanks for review. I do not like memset().  It just hides things. If
    it is not initialized coverity and valgrind would not complain and
    we will see the issue. <br>
    The plugin for gedit that I use does not show the spaces the way the
    old plugin did. I need to find a good plugin again.<br>
    I will take a note to find a better plugin.<br>
    <br>
    Please push and I will just rebase. <br>
    <br>
    <blockquote
      cite="mid:1332189593.12919.9.camel@sgallagh520.sgallagh.bos.redhat.com"
      type="cite">
      <pre wrap="">
On Fri, 2012-03-16 at 01:22 -0400, Dmitri Pal wrote:
</pre>
      <blockquote type="cite">
        <pre wrap="">On 01/05/2011 01:19 PM, Stephen Gallagher wrote:
</pre>
        <blockquote type="cite">
          <pre wrap="">On 01/03/2011 06:12 PM, Dmitri Pal wrote:
</pre>
          <blockquote type="cite">
            <pre wrap="">Please see the attached patches. I tried to split the patches logically
into manageable sets.
Unfortunately I made a minor mistake and I am afraid I will do something
wrong to fix it.
I merged two wrong patches. Fortunately it was three liner with 1 liner
so it is not a big of the deal but I am really scared that I will do
something wrong and loose the work I have done.
So I hope it is Ok to send it as is.
</pre>
          </blockquote>
          <pre wrap="">
</pre>
          <blockquote type="cite">
            <pre wrap="">0001--INI-Making-Coverity-happy.patch  &lt;- this is the patch I submitted
earlier that I merged by mistake. I was supposed to merge it with patch
25 but picked the wrong one instead.
Patch 25 addresses the real issue found by Coverity as mentioned in
Stephen's review mail but it did not apply cleanly since it relies on
some code from the patches in the middle.
</pre>
          </blockquote>
          <pre wrap="">

I split this patch. I'm attaching the missing initializer patch. That
gets an ack.

However, the 100078/79 fix is not correct. I did some digging and found
the real problem. If you look at line 264 and 279 of ini_valueobj_ut.c,
it's possible for other_create_test() to return EOK without having
modified the vo variable. As a result, you will be passing the
uninitialized value to modify_test(). So Coverity is right that there's
a possibility of it passing a freed variable here.

So the Coverity fix gets a nack.

</pre>
        </blockquote>
        <pre wrap="">
I realized what the problem is. See the first patch.

</pre>
        <blockquote type="cite">
          <pre wrap="">
</pre>
          <blockquote type="cite">
            <pre wrap="">0002--INI-Adding-missing-function-declararion.patch  &lt;- this is the
patch that was rejected from the second set sent earlier. Fixed
according to review comment.
</pre>
          </blockquote>
        </blockquote>
        <pre wrap="">
Dropped. I think Jakub fixed it.
</pre>
        <blockquote type="cite">
          <pre wrap="">

Ack.


</pre>
          <blockquote type="cite">
            <pre wrap="">0003--BUILD-Allow-trace-per-component.patch &lt;- This patch allows tracing
per component
</pre>
          </blockquote>
          <pre wrap="">

Nack. Requiring a file doesn't make sense. Just add --enable-trace-ini,
--enable-trace-collection etc. flags.

</pre>
        </blockquote>
        <pre wrap="">
Dropped for now

</pre>
        <blockquote type="cite">
          <blockquote type="cite">
            <pre wrap="">The following set of patches introduces the merging of sections during
the reading of the file:
0004--INI-New-error-codes-and-messages.patch
</pre>
          </blockquote>
          <pre wrap="">
It would be better to use an enum here instead of #defines, then your
last entry for ERR_MAXPARSE will always be one higher than your previous
error message.

See sdap_basic_opt in src/providers/ldap/sdap.h in the SSSD source code
for an example.

That said, there's nothing WRONG with this patch, so ack.

</pre>
        </blockquote>
        <pre wrap="">
Attached as is. The ticket is open.


</pre>
        <blockquote type="cite">
          <blockquote type="cite">
            <pre wrap="">0005--INI-New-merge-flags.patch
</pre>
          </blockquote>
          <pre wrap="">I don't much like the idea of having flags that have overlapping bits
without an obvious reason (0x0020 and 0x0030, for example), but since
those are pre-existing, I'll leave them alone. Ack.
</pre>
        </blockquote>
        <pre wrap="">
As I mentioned earlier I will reconsider however for now it is unchanged.

</pre>
        <blockquote type="cite">
          <pre wrap="">
</pre>
          <blockquote type="cite">
            <pre wrap="">0006--INI-Add-new-vars-to-parse-structure.patch
</pre>
          </blockquote>
          <pre wrap="">Ack, though it seems to me that a memset to zero would be simpler than
manually setting every struct member to zero manually.
</pre>
        </blockquote>
        <pre wrap="">
Unchanged


</pre>
        <blockquote type="cite">
          <pre wrap="">
</pre>
          <blockquote type="cite">
            <pre wrap="">0007--INI-Add-save_error-function.patch
</pre>
          </blockquote>
          <pre wrap="">Nack. It doesn't make sense to pass in an index value to an array local
only to the function. That's not clean. It would be better to either
pass in the const char * for the message, or at worst pass in an enum
type that you would use to look up the matching error message.
</pre>
        </blockquote>
        <pre wrap="">
Modified. Const char * is passed in.

</pre>
        <blockquote type="cite">
          <pre wrap="">
</pre>
          <blockquote type="cite">
            <pre wrap="">0008--INI-Change-parse_error-to-use-save_error.patch
</pre>
          </blockquote>
          <pre wrap="">Nack. This will need to be updated to correspond to the changes for
patch 0007.
</pre>
        </blockquote>
        <pre wrap="">
Updated to reflect the above patch.

</pre>
        <blockquote type="cite">
          <pre wrap="">
</pre>
          <blockquote type="cite">
            <pre wrap="">0009--INI-Preparing-for-merging-sections.patch
</pre>
          </blockquote>
          <pre wrap="">Ack.

</pre>
        </blockquote>
        <pre wrap="">
Unchanged

</pre>
        <blockquote type="cite">
          <blockquote type="cite">
            <pre wrap="">0010--INI-Enhance-value-processing.patch
</pre>
          </blockquote>
          <pre wrap="">Ack.

</pre>
        </blockquote>
        <pre wrap="">
Unchanged

</pre>
        <blockquote type="cite">
          <blockquote type="cite">
            <pre wrap="">0011--INI-Use-section-line-number.patch
</pre>
          </blockquote>
          <pre wrap="">Ack.

</pre>
        </blockquote>
        <pre wrap="">
Unchanged

</pre>
        <blockquote type="cite">
          <blockquote type="cite">
            <pre wrap="">0012--INI-Refactor-section-processing.patch
</pre>
          </blockquote>
          <pre wrap="">Nack. Please fix the formatting of the switch statement. There should be
only one level of indent following the case tag.

</pre>
        </blockquote>
        <pre wrap="">
Done

</pre>
        <blockquote type="cite">
          <blockquote type="cite">
            <pre wrap="">0013--INI-Return-error-in-DETECT-mode.patch
</pre>
          </blockquote>
          <pre wrap="">Ack

</pre>
        </blockquote>
        <pre wrap="">
Unchanged

</pre>
        <blockquote type="cite">
          <blockquote type="cite">
            <pre wrap="">0014--INI-New-test-files-for-section-merge.patch
</pre>
          </blockquote>
          <pre wrap="">Ack

</pre>
        </blockquote>
        <pre wrap="">
Unchanged

</pre>
        <blockquote type="cite">
          <blockquote type="cite">
            <pre wrap="">0015--INI-Test-DETECT-mode-and-use-new-file.patch
</pre>
          </blockquote>
          <pre wrap="">Ack

</pre>
        </blockquote>
        <pre wrap="">Unchanged
</pre>
        <blockquote type="cite">
          <blockquote type="cite">
            <pre wrap="">0016--INI-Test-for-all-section-merge-modes.patch
</pre>
          </blockquote>
          <pre wrap="">Nack. Please fix random tabs in the indentation. Otherwise it looks fine.

</pre>
        </blockquote>
        <pre wrap="">Fixed.



Added another patch, in the new numeration it is patch 15. It is a
cosmetic change to meet the coding standard.

The rest are deferred in this round.

------------------------------

I also have a question.
I use a struct that is not aligned by 8 byte boundary on a 64 bit machine.

pointer = 8
unsigned = 4
unsigned = 4
unsigned = 4

total length is 20 which is padded to 24

I allocate memory for it using sizeof of the structure. Everything is
fine - no leak. I then set the values of the structure one by one.
Also in some cases for debugging purposes I print the contents of the
allocated memory as binary memory byte by byte.
When I do this with the memory used by this structure valgrind complains
that I am printing uninitialized memory. And indeed the padding is not
initialized by me.
So I can:
1) memset memory after allocating it (which I do not like to do
especially if I can set all the elements to the non zero values anyways
except for padding)
2) Add artificial padding to the structure or a dummy unsigned member
(seems a bit ugly)
3) Ignore the warning (seems wrong)
4) Ask your advice (...?)


_______________________________________________
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://fedorahosted.org/mailman/listinfo/sssd-devel">https://fedorahosted.org/mailman/listinfo/sssd-devel</a>
</pre>
      </blockquote>
      <pre wrap="">


</pre>
      <pre wrap="">
<fieldset class="mimeAttachmentHeader"></fieldset>
_______________________________________________
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://fedorahosted.org/mailman/listinfo/sssd-devel">https://fedorahosted.org/mailman/listinfo/sssd-devel</a>
</pre>
    </blockquote>
    <br>
    <br>
    <pre class="moz-signature" cols="72">-- 
Thank you,
Dmitri Pal

Sr. Engineering Manager IPA project,
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>