-----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: >>> >>> 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. >
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/ -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/ iEYEARECAAYFAkrKBMkACgkQeiVVYja6o6MJ/QCfdrIlsWdtDH4vznASzFvCwkhl N+IAn02UuAI/K7lzCmEdO2EVV54UBNr/ =c2We -----END PGP SIGNATURE----- _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
