#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
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
