On 03/26/2012 10:55 AM, Dmitri Pal wrote:
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?

#0019
Removed error variable and return EOK directly.

#0020
Changed comment.
Removed error variable and return EOK directly.
We agree on IRC that it is OK to keep function signature as is.

#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.


Updated patch is attached.

The rest are same.

_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel


-- 
Thank you,
Dmitri Pal

Sr. Engineering Manager IPA project,
Red Hat Inc.


-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/