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.
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.
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.
On Mon, Sep 28, 2009 at 10:33:22AM -0400, 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.
works for me, ACK. Let's wait for Dmitri's comments.
bye, Sumit
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.
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.
On Mon, Sep 28, 2009 at 02:50:30PM -0400, 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.
Works for me and I think Dmitri's points are in.
ACK from me.
bye, Sumit
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.
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:
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.
- The ini_to_collection argument should be configuration source, not
"filename".
- 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; }
- 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.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 10/02/2009 12:52 PM, Dmitri Pal wrote:
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:
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.
- The ini_to_collection argument should be configuration source, not
"filename".
- 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; }
- 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.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Ack. Thanks for cleaning this up.
- -- Stephen Gallagher RHCE 804006346421761
Looking to carve out IT costs? www.redhat.com/carveoutcosts/
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 10/02/2009 02:47 PM, Stephen Gallagher wrote:
On 10/02/2009 12:52 PM, Dmitri Pal wrote:
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:
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.
- The ini_to_collection argument should be configuration source, not
"filename".
- 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; }
- 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.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Ack. Thanks for cleaning this up.
Pushed to master. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
- -- Stephen Gallagher RHCE 804006346421761
Looking to carve out IT costs? www.redhat.com/carveoutcosts/
sssd-devel@lists.fedorahosted.org