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

Stephen Gallagher sgallagh at redhat.com
Mon Sep 28 18:50:30 UTC 2009


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

New patch attached that should address Dmitri's concerns.

-- 
Stephen Gallagher
RHCE 804006346421761

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-Add-config_from_fd-to-ini_config.patch
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20090928/a5f588d6/attachment.ksh>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 260 bytes
Desc: OpenPGP digital signature
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20090928/a5f588d6/attachment.sig>


More information about the sssd-devel mailing list