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
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.
On Fri, 2012-03-16 at 01:22 -0400, Dmitri Pal wrote:
On 01/05/2011 01:19 PM, Stephen Gallagher wrote:
> On 01/03/2011 06:12 PM, Dmitri Pal wrote:
> > 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.
> > 0001--INI-Making-Coverity-happy.patch <- 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.
> 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.
I realized what the problem is. See the first patch.
> > 0002--INI-Adding-missing-function-declararion.patch <- this is the
> > patch that was rejected from the second set sent earlier. Fixed
> > according to review comment.
Dropped. I think Jakub fixed it.
> > 0003--BUILD-Allow-trace-per-component.patch <- This patch allows tracing
> > per component
> Nack. Requiring a file doesn't make sense. Just add --enable-trace-ini,
> --enable-trace-collection etc. flags.
Dropped for now
> > The following set of patches introduces the merging of sections during
> > the reading of the file:
> > 0004--INI-New-error-codes-and-messages.patch
> 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.
Attached as is. The ticket is open.
> > 0005--INI-New-merge-flags.patch
> 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.
As I mentioned earlier I will reconsider however for now it is unchanged.
> > 0006--INI-Add-new-vars-to-parse-structure.patch
> Ack, though it seems to me that a memset to zero would be simpler than
> manually setting every struct member to zero manually.
> > 0007--INI-Add-save_error-function.patch
> 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.
Modified. Const char * is passed in.
> > 0008--INI-Change-parse_error-to-use-save_error.patch
> Nack. This will need to be updated to correspond to the changes for
> patch 0007.
Updated to reflect the above patch.
> > 0009--INI-Preparing-for-merging-sections.patch
> > 0010--INI-Enhance-value-processing.patch
> > 0011--INI-Use-section-line-number.patch
> > 0012--INI-Refactor-section-processing.patch
> Nack. Please fix the formatting of the switch statement. There should be
> only one level of indent following the case tag.
> > 0013--INI-Return-error-in-DETECT-mode.patch
> > 0014--INI-New-test-files-for-section-merge.patch
> > 0015--INI-Test-DETECT-mode-and-use-new-file.patch
> > 0016--INI-Test-for-all-section-merge-modes.patch
> Nack. Please fix random tabs in the indentation. Otherwise it looks fine.
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