[SSSD] [PATCH] Add config_from_fd() to ini_config

Dmitri Pal dpal at redhat.com
Mon Sep 28 17:46:57 UTC 2009


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 at 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 at 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/




More information about the sssd-devel mailing list