[SSSD] [INI] Patches for ding-libs: Merging config sections, handling metadata, remaining Coverity issues...

Jan Zelený jzeleny at redhat.com
Tue Mar 27 07:27:36 UTC 2012


> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120327/e82eef43/attachment.sig>


More information about the sssd-devel mailing list