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. -- Thank you, Dmitri Pal Engineering Manager IPA project, Red Hat Inc. ------------------------------- Looking to carve out IT costs? www.redhat.com/carveoutcosts/ _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel