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. -- Stephen Gallagher RHCE 804006346421761 Looking to carve out IT costs? www.redhat.com/carveoutcosts/
From c66bb515635c3ed8439d85d9c0724e56db59c2e2 Mon Sep 17 00:00:00 2001 From: Stephen Gallagher <sgall...@redhat.com> Date: Mon, 28 Sep 2009 09:36:00 -0400 Subject: [PATCH 1/2] Add config_from_fd() to ini_config --- common/ini/ini_config.c | 129 +++++++++++++++++++++++++++++++++++++++----- common/ini/ini_config.h | 23 +++++++- common/ini/ini_config_ut.c | 74 +++++++++++++++++++++++++ 3 files changed, 211 insertions(+), 15 deletions(-) diff --git a/common/ini/ini_config.c b/common/ini/ini_config.c index ffc5960..16cabd1 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> @@ -153,13 +154,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 *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,8 +177,6 @@ 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); @@ -423,9 +422,17 @@ void free_ini_config_lines(struct collection_item *lines) TRACE_FLOW_STRING("free_ini_config_lines", "Exit"); } +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); + /* 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 +441,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 +450,94 @@ 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 = 0; + FILE *config_file = NULL; + + TRACE_FLOW_STRING("config_from_fd", "Entry"); + + config_file = fopen(config_filename, "r"); + if(!config_file) { + error = errno; + TRACE_FLOW_NUMBER("config_from_file_with_lines. Returns", 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; + FILE *config_file; + + TRACE_FLOW_STRING("config_from_fd_with_lines", "Entry"); + + config_file = fdopen(fd, "r"); + if (!config_file) { + error = errno; + TRACE_FLOW_NUMBER("config_from_fd_with_lines. Returns", 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; +} + +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 +596,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); @@ -538,6 +624,7 @@ int config_for_app(const char *application, struct collection_item **error_set) { int error = EOK; + FILE *file; char *file_name; struct collection_item *error_list_common = NULL; struct collection_item *error_list_specific = NULL; @@ -605,8 +692,18 @@ 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); + file = fopen(config_file, "r"); + if (!file) { + error = errno; + if(created) { + col_destroy_collection(*ini_config); + *ini_config = NULL; + } + } + else { + error = ini_to_collection(file, config_file, *ini_config, + error_level, pass_common, NULL); + } tried++; /* ENOENT and EOK are Ok */ if (error) { @@ -624,7 +721,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); @@ -665,7 +762,13 @@ int config_for_app(const char *application, TRACE_INFO_STRING("Opening file:", file_name); /* Read master file */ - error = ini_to_collection(file_name, *ini_config, + file = fopen(file_name, "r"); + if (!file) { + error = errno; + TRACE_ERROR_NUMBER("Failed to read specific application file", error); + return error; + } + error = ini_to_collection(file, file_name, *ini_config, error_level, pass_specific, NULL); tried++; free(file_name); 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.6.2.5
signature.asc
Description: OpenPGP digital signature
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel