#0016:
Ack
#0017:
I'm not sure what's the point in checking errno when malloc fails. IIRC the
errno will be always ENOMEM. The same applies to the strndup several lines
below.
The initialization block (new_ctx->.... = NULL) is redundant, you set
everything necessary later and in case of any error, uninitalized values won't
make it out of the function anyway.
#0018:
Ack
#0019:
The return error at the end is misleading, please use return EOK instead
(similar issue is in other patches as well, please change it when you see it).
#0020:
Please change the comment to:
Determines if two file contexts are different by comparing:
I don't like the prototype of ini_config_changed(). Is it necessary to return
special error code? I would perform the check and in case of wrong input, I'd
fall back to the safe option - return true (as in the config file has changed).
In this case no special check is necessary and the code will be more readable.
#0021:
Ack
#0022:
I'm confused. In the patch comment you write that we can't remove it from the
old interface but yet you remove it from the header file. I'd say it should
remain there (and be marked) as well.
#0023:
Ack
#0024:
Ack
On 01/24/2011 12:27 PM, Sumit Bose wrote:
The numeration changed. But the ste is the same. The patches in this
mail are dependent on the patches sent in the previous email on Friday.
>> Patches related porting of the meta data from old way of doing things to
>> the new way of doing things:
>> 0017--INI-Separate-close-and-destroy.patch
>
> You should set file_ctx->file to NULL after fclose(file_ctx->file) to
> make the if(file_ctx->file) checks work in ini_config_file_close() and
> ini_config_file_destroy().
Yes it was fixed in a different patch. Now merged together.
> There are tab indents in merge_values_test() and merge_section_test().
Fixed.
>> 0018--INI-Function-to-reopen-file.patch
Unchanged
>> 0019--INI-Metadata-collection-is-gone.patch
>
> You remove metadata from struct ini_cfgfile without removing all
> references to metadata in the same patch. You should make clear that
> more patches are needed to create a buildable version of libini or
> remove all references in this patch.
This is not true. The buildable version is still preserved.
All the code that I am building is not executed yet.
It is a parallel interface. There are currently two interfaces:
ini_config.h and ini_configobj.h
The external code is still suing the ini_config.h.
The new interface that the UT are executing is pointing to the interface
growing in ini_configobj.h.
This metadata functions of the existing ini_config interface are not
affected.
After some thinking I realized that there is too much overhead for doing
the metadata handling the way I did it in the first implementation. The
new interface corrects this but does not affect the old code yet. So the
metadata collection is gone from the new interface not from the old one.
The code compiles fine after this patch and the three patches right
after it take advantage of this change. Patches 18-21 should be
considered together as a block of related patches.
> I wonder is the following is a change of defaults. With the patch the
> new file_ctx->file_stats are only set if INI_META_STATS is set while
> previously file-ctx>metadata was set unconditionally.
Fixed by memset.
>> 0020--INI-Check-access-function.patch
This is now 19.
> I wonder if it is necessary to return EINVAL if flags == 0. I would say
> in this case no checks are requested and EOK could be returned?
I disagree with this one. With flags 0 I think it is a noop and might
lead to the wrong assumptions while no operation was actually performed.
IMO passing flags=0 is a codding error in this case and thus should be
reported as such.
> I would prefer to copy file_ctx->file_stats.st_mode instead of modifying
> it, e.g. you might want to know the file type later on.
Good catch. Agree. Fixed.
>> 0021--INI-Avoid-double-free.patch <- patch related to 17 (missed check)
>
> oops, so you can ignore my comment to 00017, let's see if there is also
> a patch for ini_config_file_destroy(). "I might squash this patch into
> one of the previous ones." Yes, please.
This one is merged now.
>> 0022--INI-Function-to-check-for-changes.patch
This is now 20 and unchanged.
>> 0023--INI-Tests-for-access-and-changes.patch
This is now 21.
> Why do you need sleep(1) ?
Removed.
> The man page of system() does not mention that system() sets errno,
> please check the return code instead.
Changed to use execlp().
>> 0024--INI-Rename-error-print-function.patch <- rename error printing
>> function for consistency with new interface
This is now 22.
> Maybe ini_print_errors() should print a deprecated warning to make
> migrations easier.
As I mentioned above there is no impact on the existing interface.
As you notice I made a clone of the existing function with a different
name and made a comment to clean the old function when the interface is
deprecated.
The changes will be made later when we switch to new interface, so it is
premature to print a warning as this function is still used.
>> 0025--INI-Initialize-variables-in-loops.patch <- Coverity issue
>> addressed. Related to patch 0001.
This is 23 and merged with another initialization one liner patch.
>> 0026--INI-Exposing-functions.patch <- Make some internal functions
>> reusable
This is 24 and unchanged.
>> There is also patch 27. It is a piece of new functionality. It is a
>> preview. Please see the comment before reviewing it.
>> Do I need to split it into multiple patches or it is Ok as is? It is
>> pretty big but all changes are in one file and logically related.
>> The UNIT test is missing so I am not claiming it actually works as
>> expected.
>
> I didn't had a look at 0027 so far.
I am going to hold to patch 27 yet. I have other higher priority issues
to address.
Please review and nack/push this ASAP.
I need to clean ding libs and elapi for the audit effort ASAP.
There are too many patches piling to be able to move forward
effectively. I am the bottleneck now and i need to bring all this into a
usable state within a week or so. Please help!
One last thing. Should I switch to the format used in freeipa?
https://fedorahosted.org/freeipa/wiki/PatchFormat
It seems that SSSD/ding-libs does not follow this format. Should it? Not
insisting, just asking.
I'm already using it. I don't mind one way or the other.
Thanks
Jan