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