-----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(a)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(a)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(a)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(a)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(a)lists.fedorahosted.org
-----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-----