Dmitri Pal wrote:
> Stephen Gallagher wrote:
>
>> On 09/28/2009 01:46 PM, Dmitri Pal wrote:
>>
>>
>>> Stephen Gallagher wrote:
>>>
>>>
>>>> On 09/28/2009 09:55 AM, Simo Sorce wrote:
>>>>
>>>>
>>>>
>>>>> On Mon, 2009-09-28 at 09:38 -0400, Stephen Gallagher wrote:
>>>>>
>>>>>
>>>>>
>>>>>> The SSSD needs a config_from_fd() variant of the config_from_file()
>>>>>> call
>>>>>> so that we can preopen a config file and perform some verification on
>>>>>> it
>>>>>> before parsing it. The config_from_fd() call is used to avoid race
>>>>>> conditions between testing the file and reading it in.
>>>>>>
>>>>>> Note: the *_from_fd() functions still require the config file name for
>>>>>> internal information. This does not imply that it is used to open the
>>>>>> file.
>>>>>>
>>>>>>
>>>>>>
>>>>> I think it is better not to require a file name, and, internally, just
>>>>> use something like "dummy" or a random string like the process pid etc..
>>>>>
>>>>> This way there is no risk that someone may accidentally change the code
>>>>> later to re-open the file or something like that, if that is done it
>>>>> will immediately break when it tries to open "dummy" (hopefully :-)
>>>>>
>>>>> Simo.
>>>>>
>>>>> _______________________________________________
>>>>> sssd-devel mailing list
>>>>> sssd-devel@lists.fedorahosted.org
>>>>> https://fedorahosted.org/mailman/listinfo/sssd-devel
>>>>>
>>>>>
>>>>>
>>>> New version does not require the file name for the _from_fd() functions.
>>>> It will create a string "dummy_<fd>" to use for the config file name
>>>> internally.
>>>>
>>>>
>>>>
>>>>
>>>> ------------------------------------------------------------------------
>>>>
>>>> _______________________________________________
>>>> sssd-devel mailing list
>>>> sssd-devel@lists.fedorahosted.org
>>>> https://fedorahosted.org/mailman/listinfo/sssd-devel
>>>>
>>>>
>>> Nack:
>>>
>>> Had an IRC conversation with Steven:
>>>
>>> <dpal> sgallagh, why do you need "dummy" thing
>>> <dpal> sgallagh, I would have done it differently
>>> <dpal> I mean the internal layers
>>> <sgallagh> dpal: I added the "dummy" thing so that there was always a
>>> unique identifier at the top level of the collection
>>> <sgallagh> dpal: If you would like to recommend an alternative approach,
>>> please nack the patch and provide suggestions. I'm all ears :)
>>> <dpal> sgallagh, then this is something that the app should pass in
>>> together with fd
>>> <dpal> if the app opened the file it should say how to name it
>>> <sgallagh> dpal: Look at the history, I did that at first and it was nacked.
>>> <sgallagh> s/history/email thread/
>>> <dpal> I have seen Simo's comment but I do not think we are talking
>>> about same thing
>>> <sgallagh> dpal: Perhaps you can clarify, then?
>>> <dpal> The filename is used in the low level function in only one place
>>> besides the opening the file - to name the collection
>>> <sgallagh> right
>>> <sgallagh> So I just gave it the name "dummy_fd" if it was opened as an
>>> fd instead of directly opening a file path.
>>> <dpal> It is done in the error list only
>>> <dpal> It is done for error reporting purpose
>>> <dpal> You do not want to return error list back to caller with text
>>> "errors parsing dymmy_123"
>>> <dpal> sgallagh, the only value of having the file name in the error
>>> list is to report back the error in the file.
>>> * sgallagh nods
>>> <dpal> sgallagh, If the caller handles the file it should name it or
>>> there should be no name in this case
>>> <sgallagh> Well, that seems like a convincing-enough argument to negate
>>> simo's nack, honestly.
>>> <dpal> and a generic name instead
>>> <sgallagh> Neither of us were really sure if the name was ever used, or
>>> if it was just there to be available to the caller
>>> <sgallagh> dpal: Do you want me to just change it from "dummy_<fd>" to
>>> "file descriptor <fd>" instead?
>>> <dpal> sgallagh, more of then I would always pass two things into
>>> ini_to_collection
>>> <sgallagh> dpal: Or put back the interface to let it be specified
>>> <dpal> sgallagh, let me finish
>>> <dpal> sgallagh, move the fopen/fdopen out of the ini_to_collection
>>> <dpal> Pass in the ready to use FILE *file and the string for naming the
>>> "source"
>>> <dpal> Then wrap the new implementation of the function with function
>>> that just open the file and sends down the filename as a source string.
>>> <dpal> The new "other" function would instead do the fdopen using passed
>>> in file descriptor and will send down passed in string to name the source
>>> <sgallagh> dpal: That still doesn't explain the fd case. If we're
>>> calling config_from_fd(), are you saying that we need to also pass the
>>> filename?
>>> <dpal> sgallagh, I say you need to name the source in some way so that
>>> you can report the error
>>> <dpal> sgallagh, it was logical to use filename as name of the source in
>>> case of file
>>> <dpal> It is not clear to me what would be the best name of the source
>>> if you are using a FD
>>> <sgallagh> dpal: I think I'd prefer to leave the interface as-is and
>>> change the "dummy_fd" to the more readable "file descriptor fd"
>>> <sgallagh> dpal: I'll make the changes under the hood as you've suggested
>>> <dpal> This does not help the caller
>>> <sgallagh> dpal: what do you mean?
>>> <dpal> He does not know what this fd means
>>> <sgallagh> dpal: He has to. He's the one who passed it in
>>> <dpal> sgallagh, I am talking about admin who reads the output and does
>>> not have a clue what fd 23 meant.
>>> <sgallagh> dpal: Ok, put this all in a nack email and I'll put it back
>>> the way I had it before, where you have to pass the filename as well as
>>> the fd into config_from_fd(). Ok?
>>> <sgallagh> I'll make the other internal changes as well
>>> <dpal> If you tell him "configuration port" he will understand that the
>>> configuration was read on the port.
>>> <dpal> Also I would suggest not to name it as file name in this case but
>>> rather "configuration_source" or something like to make it more generic
>>> for fd and file cases.
>>>
>>>
>>>
>>>
>> New patch attached that should address Dmitri's concerns.
>>
>>
>> ------------------------------------------------------------------------
>>
>> _______________________________________________
>> sssd-devel mailing list
>> sssd-devel@lists.fedorahosted.org
>> https://fedorahosted.org/mailman/listinfo/sssd-devel
>>
> Sorry. Nack again:
>
> 1) In ini_to_collection()
>
> if (file == NULL) {
> error = errno;
> TRACE_ERROR_NUMBER("Failed to open file - but this is OK", error);
> return ENOENT;
> }
>
> The errno is irrelevant and the error string needs to change.
>
> 2) The ini_to_collection argument should be configuration source, not
> "filename".
>
> 3) Trace stament here should be TRACE_ERROR... instead of TRACE_FLOW...
> if(!config_file) {
> error = errno;
> TRACE_FLOW_NUMBER("config_from_file_with_lines. Returns", error);
> return error;
> }
> I usually have some more descriptive message in case of error. Like
> "Failed to open file" or similar.
> Trace FLOW is used for normal entry and exit into the function. In case
> of errors it should be TRACE_ERROR...
> Same comment regarding this piece of code:
>
> config_file = fdopen(fd, "r");
> if (!config_file) {
> error = errno;
> TRACE_FLOW_NUMBER("config_from_fd_with_lines. Returns", error);
> return error;
> }
>
> 4) Code inside config_for_app() function is really broken.
> It was relying on the fact that in the original implementation the
> failure to open the file would translate into ENOENT error code.
> As you might see in this case in error processing the corresponding
> action was taken. Now the error handling can't rely on this and the
> whole logic about
> trying one file and then another and then checking if we failed with
> ENOENT with both files is broken.
>
> The simplest way to fix this issue is to have a wrapper that would do
> something like config_from_file_with_lines() does but would open the
> file and call
> ini_to_collection directly. It should return ENOENT if the file opening
> failed. In this case in config_for_app() you would just replace direct call
> to ini_to_collection() with this function. It would be easier than to
> rewrite and retest the logic of this function.
> It took me some time to write it and make it work the way I wanted and
> make sure it works in different scenarios.
>
>
Refined patch attached.
--
Thank you,
Dmitri Pal
Engineering Manager IPA project,
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
>From 8e3fd20bd6cea8f56b68f7a57c252b408a9baa9c Mon Sep 17 00:00:00 2001
From: Stephen Gallagher <sgall...@redhat.com>
Date: Mon, 28 Sep 2009 09:36:00 -0400
Subject: [PATCH] INI Add config_from_fd() to ini_config
Patch adds ability to read
configuration using already open
file descriptor.
Started by Steve G and refined a bit by me.
---
common/ini/ini_config.c | 164 ++++++++++++++++++++++++++++++++++++++------
common/ini/ini_config.h | 23 ++++++-
common/ini/ini_config_ut.c | 74 ++++++++++++++++++++
3 files changed, 239 insertions(+), 22 deletions(-)
diff --git a/common/ini/ini_config.c b/common/ini/ini_config.c
index ffc5960..bd4fa79 100644
--- a/common/ini/ini_config.c
+++ b/common/ini/ini_config.c
@@ -27,6 +27,7 @@
#include <ctype.h>
#include <stdlib.h>
#include <locale.h>
+#include <fcntl.h>
#include "config.h"
/* For error text */
#include <libintl.h>
@@ -75,6 +76,17 @@
#define INI_ERROR "errors"
#define INI_ERROR_NAME "errname"
+/* Internally used functions */
+static int config_with_lines(const char *application,
+ FILE *config_file,
+ const char *config_source,
+ struct collection_item **ini_config,
+ int error_level,
+ struct collection_item **error_list,
+ struct collection_item **lines);
+
+
+
/* Different error string functions can be passed as callbacks */
typedef const char * (*error_fn)(int error);
@@ -153,13 +165,13 @@ int read_line(FILE *file,
/***************************************************************************/
/* Function to read single ini file and pupulate
* the provided collection with subcollcetions from the file */
-static int ini_to_collection(const char *filename,
+static int ini_to_collection(FILE *file,
+ const char *config_filename,
struct collection_item *ini_config,
int error_level,
struct collection_item **error_list,
struct collection_item **lines)
{
- FILE *file;
int error;
int status;
int section_count = 0;
@@ -176,12 +188,9 @@ static int ini_to_collection(const char *filename,
TRACE_FLOW_STRING("ini_to_collection", "Entry");
- /* Open file for reading */
- file = fopen(filename, "r");
if (file == NULL) {
- error = errno;
- TRACE_ERROR_NUMBER("Failed to open file - but this is OK", error);
- return ENOENT;
+ TRACE_ERROR_NUMBER("No file handle", EINVAL);
+ return EINVAL;
}
/* Open the collection of errors */
@@ -194,7 +203,7 @@ static int ini_to_collection(const char *filename,
return error;
}
/* Add file name as the first item */
- error = col_add_str_property(*error_list, NULL, INI_ERROR_NAME, filename, 0);
+ error = col_add_str_property(*error_list, NULL, INI_ERROR_NAME, config_filename, 0);
if (error) {
TRACE_ERROR_NUMBER("Failed to and name to collection", error);
fclose(file);
@@ -423,9 +432,10 @@ void free_ini_config_lines(struct collection_item *lines)
TRACE_FLOW_STRING("free_ini_config_lines", "Exit");
}
+
/* Read configuration information from a file */
int config_from_file(const char *application,
- const char *config_file,
+ const char *config_filename,
struct collection_item **ini_config,
int error_level,
struct collection_item **error_list)
@@ -434,7 +444,7 @@ int config_from_file(const char *application,
TRACE_FLOW_STRING("config_from_file", "Entry");
error = config_from_file_with_lines(application,
- config_file,
+ config_filename,
ini_config,
error_level,
error_list,
@@ -443,16 +453,97 @@ int config_from_file(const char *application,
return error;
}
+/* Read configuration information from a file descriptor */
+int config_from_fd(const char *application,
+ int fd,
+ const char *config_source,
+ struct collection_item **ini_config,
+ int error_level,
+ struct collection_item **error_list)
+{
+ int error;
+
+ TRACE_FLOW_STRING("config_from_fd", "Entry");
+ error = config_from_fd_with_lines(application, fd, config_source,
+ ini_config, error_level,
+ error_list, NULL);
+ TRACE_FLOW_NUMBER("config_from_fd. Returns", error);
+ return error;
+}
+
/* Function to read the ini file and have a collection
* of which item appers on which line
*/
int config_from_file_with_lines(const char *application,
- const char *config_file,
+ const char *config_filename,
struct collection_item **ini_config,
int error_level,
struct collection_item **error_list,
struct collection_item **lines)
{
+ int error = EOK;
+ FILE *config_file = NULL;
+
+ TRACE_FLOW_STRING("config_from_file_with_lines", "Entry");
+
+ config_file = fopen(config_filename, "r");
+ if(!config_file) {
+ error = errno;
+ TRACE_ERROR_NUMBER("Failed to open file", error);
+ return error;
+ }
+
+ error = config_with_lines(application, config_file,
+ config_filename, ini_config,
+ error_level, error_list,
+ lines);
+ TRACE_FLOW_NUMBER("config_from_file_with_lines. Returns", error);
+ return error;
+}
+
+/* Function to read the ini file and have a collection
+ * of which item appers on which line
+ */
+int config_from_fd_with_lines(const char *application,
+ int fd,
+ const char *config_source,
+ struct collection_item **ini_config,
+ int error_level,
+ struct collection_item **error_list,
+ struct collection_item **lines)
+{
+ int error = EOK;
+ FILE *config_file;
+
+ TRACE_FLOW_STRING("config_from_fd_with_lines", "Entry");
+
+ config_file = fdopen(fd, "r");
+ if (!config_file) {
+ error = errno;
+ TRACE_ERROR_NUMBER("Failed to dup file", error);
+ return error;
+ }
+
+ error = config_with_lines(application, config_file,
+ config_source, ini_config,
+ error_level, error_list,
+ lines);
+ TRACE_FLOW_NUMBER("config_from_fd_with_lines. Returns", error);
+
+ return error;
+}
+
+/* Low level function that prepares the collection
+ * and calls parser.
+ */
+static int config_with_lines(const char *application,
+ FILE *config_file,
+ const char *config_source,
+ struct collection_item **ini_config,
+ int error_level,
+ struct collection_item **error_list,
+ struct collection_item **lines)
+{
int error;
int created = 0;
int created_lines = 0;
@@ -511,8 +602,9 @@ int config_from_file_with_lines(const char *application,
}
/* Do the actual work */
- error = ini_to_collection(config_file, *ini_config,
- error_level, error_list, lines);
+ error = ini_to_collection(config_file, config_source,
+ *ini_config, error_level,
+ error_list, lines);
/* In case of error when we created collection - delete it */
if (error && created) {
col_destroy_collection(*ini_config);
@@ -528,6 +620,39 @@ int config_from_file_with_lines(const char *application,
return error;
}
+/* Special wrapper around the inernal parser
+ * to open the file first.
+ * Used in conf_for_app function.
+ */
+static int ini_to_col_from_file(const char *config_filename,
+ struct collection_item *ini_config,
+ int error_level,
+ struct collection_item **error_list,
+ struct collection_item **lines)
+{
+ int error = EOK;
+ FILE *config_file = NULL;
+
+ TRACE_FLOW_STRING("ini_to_col_from_file", "Entry");
+
+ config_file = fopen(config_filename, "r");
+ if(!config_file) {
+ error = errno;
+ TRACE_ERROR_NUMBER("ini_to_col_from_file. Returns", error);
+ return ENOENT;
+ }
+
+ error = ini_to_collection(config_file,
+ config_filename,
+ ini_config,
+ error_level,
+ error_list,
+ lines);
+ TRACE_FLOW_NUMBER("ini_to_col_from_file. Returns", error);
+ return error;
+}
+
+
/* Read default config file and then overwrite it with a specific one
* from the directory */
int config_for_app(const char *application,
@@ -605,8 +730,8 @@ int config_for_app(const char *application,
/* Read master file */
if (config_file != NULL) {
TRACE_INFO_STRING("Reading master file:", config_file);
- error = ini_to_collection(config_file, *ini_config,
- error_level, pass_common, NULL);
+ error = ini_to_col_from_file(config_file, *ini_config,
+ error_level, pass_common, NULL);
tried++;
/* ENOENT and EOK are Ok */
if (error) {
@@ -624,7 +749,7 @@ int config_for_app(const char *application,
}
/* Add error results if any to the overarching error collection */
if ((pass_common != NULL) && (*pass_common != NULL)) {
- TRACE_INFO_STRING("Process erros resulting from file:", config_file);
+ TRACE_INFO_STRING("Process errors resulting from file:", config_file);
error = col_add_collection_to_collection(*error_set, NULL, NULL,
*pass_common,
COL_ADD_MODE_EMBED);
@@ -663,10 +788,9 @@ int config_for_app(const char *application,
sprintf(file_name, "%s%s%s.conf", config_dir, SLASH, application);
TRACE_INFO_STRING("Opening file:", file_name);
-
- /* Read master file */
- error = ini_to_collection(file_name, *ini_config,
- error_level, pass_specific, NULL);
+ /* Read specific file */
+ error = ini_to_col_from_file(file_name, *ini_config,
+ error_level, pass_specific, NULL);
tried++;
free(file_name);
/* ENOENT and EOK are Ok */
diff --git a/common/ini/ini_config.h b/common/ini/ini_config.h
index 1fee48a..5ae4981 100644
--- a/common/ini/ini_config.h
+++ b/common/ini/ini_config.h
@@ -97,23 +97,42 @@ const char *validation_error_str(int parsing_error);
/* Read configuration information from a file */
int config_from_file(const char *application, /* Name of the application - will be used as name of the collection */
- const char *config_file, /* Name of the config file - if NULL the collection will be empty */
+ const char *config_filename, /* Name of the config file - if NULL the collection will be empty */
struct collection_item **ini_config, /* If *ini_config is NULL a new ini object will be allocated, */
/* otherwise the one that is pointed to will be updated. */
int error_level, /* Error level - break for errors, warnings or best effort (don't break) */
struct collection_item **error_list); /* List of errors for a file */
+/* Read configuration information from a file descriptor */
+int config_from_fd(const char *application, /* Name of the application - will be used as name of the collection */
+ int fd, /* Previously opened file descriptor for the config file */
+ const char *config_source, /* Name of the file being parsed, for use when printing the error list */
+ struct collection_item **ini_config, /* If *ini_config is NULL a new ini object will be allocated*/
+ int error_level, /* Error level - break for errors, warnings or best effort (don't break) */
+ struct collection_item **error_list); /* List of errors for a file */
+
/* Read configuration information from a file with extra collection of line numbers */
int config_from_file_with_lines(
const char *application, /* Name of the application - will be used as name of the collection */
- const char *config_file, /* Name of the config file - if NULL the collection will be empty */
+ const char *config_filename, /* Name of the config file - if NULL the collection will be empty */
struct collection_item **ini_config, /* If *ini_config is NULL a new ini object will be allocated, */
/* otherwise the one that is pointed to will be updated. */
int error_level, /* Error level - break for errors, warnings or best effort (don't break) */
struct collection_item **error_list, /* List of errors for a file */
struct collection_item **lines); /* Collection of pairs where key is the key and value is line number */
+/* Read configuration information from a file descriptor with extra collection of line numbers */
+int config_from_fd_with_lines(
+ const char *application, /* Name of the application - will be used as name of the collection */
+ int fd, /* Previously opened file descriptor for the config file */
+ const char *config_source, /* Name of the file being parsed, for use when printing the error list */
+ struct collection_item **ini_config, /* If *ini_config is NULL a new ini object will be allocated, */
+ /* otherwise the one that is pointed to will be updated. */
+ int error_level, /* Error level - break for errors, warnings or best effort (don't break) */
+ struct collection_item **error_list, /* List of errors for a file */
+ struct collection_item **lines); /* Collection of pairs where key is the key and value is line number */
+
/* Read default config file and then overwrite it with a specific one from the directory */
int config_for_app(const char *application, /* Name of the application that will be used to get config for */
diff --git a/common/ini/ini_config_ut.c b/common/ini/ini_config_ut.c
index 6fbade6..52e89cb 100644
--- a/common/ini/ini_config_ut.c
+++ b/common/ini/ini_config_ut.c
@@ -23,6 +23,7 @@
#include <stdio.h>
#include <errno.h>
#include <unistd.h>
+#include <fcntl.h>
#define TRACE_HOME
#include "ini_config.h"
#include "collection.h"
@@ -133,6 +134,78 @@ int single_file(void)
return 0;
}
+int single_fd(void)
+{
+ int error;
+ struct collection_item *ini_config = NULL;
+ struct collection_item *error_set = NULL;
+ struct collection_item *lines = NULL;
+
+ int fd = open("./ini.conf", O_RDONLY);
+ if (fd < 0) {
+ error = errno;
+ printf("Attempt to read configuration returned error: %d\n", error);
+ return error;
+ }
+
+ error = config_from_fd("test", fd, "./ini.conf", &ini_config,
+ INI_STOP_ON_NONE, &error_set);
+ if (error) {
+ printf("Attempt to read configuration returned error: %d\n",error);
+ return error;
+ }
+
+ col_debug_collection(ini_config, COL_TRAVERSE_DEFAULT);
+ col_print_collection(ini_config);
+ col_print_collection(error_set);
+
+ printf("\n\n----------------------\n");
+ /* Output parsing errors (if any) */
+ print_file_parsing_errors(stdout, error_set);
+ printf("----------------------\n\n\n");
+
+
+ free_ini_config(ini_config);
+ free_ini_config_errors(error_set);
+ close(fd);
+
+ ini_config = NULL;
+ error_set = NULL;
+
+ printf("TEST WITH LINES\n");
+
+ fd = open("./ini.conf", O_RDONLY);
+ if (fd < 0) {
+ error = errno;
+ printf("Attempt to read configuration returned error: %d\n", error);
+ return error;
+ }
+ error = config_from_fd_with_lines("test", fd,
+ "./ini.conf",
+ &ini_config,
+ INI_STOP_ON_NONE,
+ &error_set, &lines);
+ if (error) {
+ printf("Attempt to read configuration returned error: %d\n",error);
+ return error;
+ }
+
+ col_debug_collection(ini_config, COL_TRAVERSE_DEFAULT);
+ col_debug_collection(lines, COL_TRAVERSE_DEFAULT);
+
+ printf("\n\n----------------------\n");
+ /* Output parsing errors (if any) */
+ print_file_parsing_errors(stdout, error_set);
+ printf("----------------------\n\n\n");
+
+
+ free_ini_config(ini_config);
+ free_ini_config_errors(error_set);
+ free_ini_config_lines(lines);
+
+ return 0;
+}
+
int negative_test(void)
{
int error;
@@ -828,6 +901,7 @@ int main(int argc, char *argv[])
if ((error = basic_test()) ||
(error = single_file()) ||
+ (error = single_fd()) ||
(error = real_test(NULL)) ||
/* This should result in merged configuration */
(error = real_test("./ini.conf")) ||
--
1.5.5.6
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
