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.
--
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
