> On 03/26/2012 04:49 AM, Jan Zelený wrote: > > #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 > > I am re-sending the whole set. > > In #0015 I removed the trailing spaces Stephen noted in the other mail. > The rest 14 are the same. > > #0017 - remove the initialization of the filename. Removing other > initialization is unsafe. Agreed on IRC. > Opened a ticket to do trust malloc to return ENOMEM. > https://fedorahosted.org/sssd/ticket/1276 > I was told that malloc() by standard must return ENOMEM. Doe this > standard apply to all distros or just Linux?
Quoting malloc man page: The UNIX 98 standard requires malloc(), calloc(), and realloc() to set errno to ENOMEM upon failure. Ack > #0019 > Removed error variable and return EOK directly. Ack > #0020 > Changed comment. > Removed error variable and return EOK directly. > We agree on IRC that it is OK to keep function signature as is. Ack > #0022 > Unchanged. > Explanation: > Some time ago I started building a parallel interface in ini_configobj.h > to the existing public interface that is in ini_config.h So we do not > change the public existing one until the new one is ready and > stabilized. As one of the steps to prepare the new interface I renamed > the function in the new interface to be consistent with the naming > convention. I might provide some compatibility layer when I am ready to > switch from the old interface to the new interface but this is out of > scope of the current patch set. I think that my comment still stands. I understand your explanation but I still don't understand what's the point in keeping the function when you don't have its declaration in header file. ABI compatibility? This is the last question I have regarding the patch set, other than that it's ok. 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
