-----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: >> >> 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. > > > > > ------------------------------------------------------------------------ > > _______________________________________________ > 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 SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/ iEYEARECAAYFAkrGStEACgkQeiVVYja6o6POJwCfeZkCf9Yf2RS/ZFhpEVfxp4rj zSYAn1KhL78VynzkQsAnizdMPGBhXOsc =zscp -----END PGP SIGNATURE----- _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
