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/
From d1bbb7589893b03e71e72367395a474c5c635d14 Mon Sep 17 00:00:00 2001
From: Dmitri Pal <d...@redhat.com>
Date: Sun, 26 Dec 2010 21:45:39 -0500
Subject: [PATCH 22/28] [INI] Rename error print function
All config file processing functions start with "ini_config".
The only function that does not comply is
ini_print_errors. We can't rename it since
it is a part of the current active interface.
I marked that function needs to be removed when we
remove old interface and created a copy with
the correct name. I also updated unit test accordingly.
---
ini/ini_configobj.h | 5 +----
ini/ini_parse_ut.c | 8 ++++----
ini/ini_print.c | 3 +--
3 files changed, 6 insertions(+), 10 deletions(-)
diff --git a/ini/ini_configobj.h b/ini/ini_configobj.h
index 913b91e..7089b62 100644
--- a/ini/ini_configobj.h
+++ b/ini/ini_configobj.h
@@ -300,10 +300,7 @@ int ini_config_copy(struct ini_cfgobj *ini_config,
struct ini_cfgobj **ini_new);
/* Function to print errors from the list */
-void ini_print_errors(FILE *file, char **error_list);
-
-
-
+void ini_config_print_errors(FILE *file, char **error_list);
/* Merge two configurations together creating a new one */
int ini_config_merge(struct ini_cfgobj *first,
diff --git a/ini/ini_parse_ut.c b/ini/ini_parse_ut.c
index b291b22..ca77112 100644
--- a/ini/ini_parse_ut.c
+++ b/ini/ini_parse_ut.c
@@ -85,7 +85,7 @@ int test_one_file(const char *in_filename,
INIOUT(printf("Errors detected while parsing: %s\n",
ini_config_get_filename(file_ctx)));
ini_config_get_errors(file_ctx, &error_list);
- INIOUT(ini_print_errors(stdout, error_list));
+ INIOUT(ini_config_print_errors(stdout, error_list));
ini_config_free_errors(error_list);
}
/* We do not return here intentionally */
@@ -371,7 +371,7 @@ int merge_values_test(void)
INIOUT(printf("Errors detected while parsing: %s\n",
ini_config_get_filename(file_ctx)));
ini_config_get_errors(file_ctx, &error_list);
- INIOUT(ini_print_errors(stdout, error_list));
+ INIOUT(ini_config_print_errors(stdout, error_list));
ini_config_free_errors(error_list);
}
@@ -548,7 +548,7 @@ int merge_section_test(void)
INIOUT(printf("Errors detected while parsing: %s\n",
ini_config_get_filename(file_ctx)));
ini_config_get_errors(file_ctx, &error_list);
- INIOUT(ini_print_errors(stdout, error_list));
+ INIOUT(ini_config_print_errors(stdout, error_list));
ini_config_free_errors(error_list);
}
@@ -706,7 +706,7 @@ int startup_test(void)
INIOUT(printf("Errors detected while parsing: %s\n",
ini_config_get_filename(file_ctx)));
ini_config_get_errors(file_ctx, &error_list);
- INIOUT(ini_print_errors(stdout, error_list));
+ INIOUT(ini_config_print_errors(stdout, error_list));
ini_config_free_errors(error_list);
}
/* We do not return here intentionally */
diff --git a/ini/ini_print.c b/ini/ini_print.c
index 1dcfa54..1128b70 100644
--- a/ini/ini_print.c
+++ b/ini/ini_print.c
@@ -459,9 +459,8 @@ void print_config_parsing_errors(FILE *file,
TRACE_FLOW_STRING("print_config_parsing_errors", "Exit");
}
-
/* Function to print errors from the list */
-void ini_print_errors(FILE *file, char **error_list)
+void ini_config_print_errors(FILE *file, char **error_list)
{
unsigned count = 0;
--
1.7.1
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
