On Fri, Jan 29, 2016 at 03:25:56PM +0100, Lukas Slebodnik wrote: > On (29/01/16 13:51), Pavel Reichl wrote: > > > > > >On 01/29/2016 01:41 PM, Lukas Slebodnik wrote: > >>https://fedorahosted.org/sssd/ticket/2931 > >>--- > >> src/providers/krb5/krb5_child.c | 17 +++++++++++++++++ > >> 1 file changed, 17 insertions(+) > >> > >>diff --git a/src/providers/krb5/krb5_child.c > >>b/src/providers/krb5/krb5_child.c > >>index > >>12eb9e2093d2bdd7d67e8d029fec1455488aa67c..88bcaddc419c1e6dc5d9a0c69b50c45a45c95efc > >> 100644 > >>--- a/src/providers/krb5/krb5_child.c > >>+++ b/src/providers/krb5/krb5_child.c > >>@@ -2675,6 +2675,23 @@ int main(int argc, const char *argv[]) > >> goto done; > >> } > >> > >>+ ret = open("/etc/krb5.conf", O_RDONLY); > >>+ if (ret == EOK) > > > >I thought that open() returns file descriptor on success and and -1 in case > >of error. Was I wrong? > > > That's a fast codding style change swith->if after testing and before sending > patch.
It's better to test file access with open if you need to actually use the file to avoid TOCTOU-style races, but here I guess it doesn't matter. Nonetheless, I would prefer to use a variable named 'fd' and not 'ret' and close the fd right away, even though krb5_child is short-lived and would close its descriptors later. I also wonder if we should wrap the code in a short function. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
