On Fri, Oct 24, 2014 at 04:11:12PM +0200, Pavel Reichl wrote:
On 10/23/2014 07:25 PM, Michal Židek wrote:
>On 10/23/2014 07:17 PM, Jakub Hrozek wrote:
>>On Thu, Oct 23, 2014 at 07:12:44PM +0200, Michal Židek wrote:
>>>On 10/23/2014 07:02 PM, Jakub Hrozek wrote:
>>>>On Thu, Oct 23, 2014 at 05:30:56PM +0200, Pavel Reichl wrote:
>>>>>- ret = create_pipe_fd(ctx->sock_name, &ctx->fd, 0111);
>>>>>+ umask(0111);
>>>>>+ ret = create_pipe_fd(ctx->sock_name, &ctx->fd);
>>>>>+ umask(DEF_RSP_UMASK);
>>>>> assert_int_equal(ret, EOK);
>>>>> assert_int_not_equal(ctx->fd, -1);
>>>>> check_sock_properties(ctx, 0666);
>>>>>
>>>>> /* Make sure we can overwrite an existing socket */
>>>>>- ret = create_pipe_fd(ctx->sock_name, &ctx->fd, 0000);
>>>>>+ umask(0000);
>>>>>+ ret = create_pipe_fd(ctx->sock_name, &ctx->fd);
>>>>>+ umask(DEF_RSP_UMASK);
>>>>
>>>>I don't like a pattern like this that requires the caller to set and
>>>>reset the caller. I would prefer if we still passed either the
>>>>required
>>>>mode or umask to the create_pipe_fd(), then create_pipe_fd() would
>>>>first
>>>>save the old umask, set the new one, create the pipes and then reset
>>>>the
>>>>old one in the "done" handler.
>>>>
>>>>The default umask should be set in a very generic function like
>>>>server_setup (maybe we already do that, not sure)
>>>
>>>It can not be done in server_setup (it is too late). So we can
>>>have a new "setup" like function or we just call umask after
>>>the process is created (in main) with some #defined value.
>>
>>If the new function called just umask then I don't see a reason for the
>>wrapper -- maybe for future extensibility?
>
>I think calling umask will be just fine. No new function needed,
>I should not have mentioned it probably.
>
>>
>>But I'm fine with both these options, I just don't like us setting an
>>umask
>>in create_pipe_fd without resetting in back in the same function. A
>>utility function should have no side effects.
>
>
>_______________________________________________
>sssd-devel mailing list
>sssd-devel(a)lists.fedorahosted.org
>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
OK, please see updated version.
I was wondering if we should create some function like
responder_common_init() which would be called in each responder in the
beginning of main() to provide common functionality like setting umask.
Thanks.
From 39277d9408527dd364baba1effb76c4d87789ee7 Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)redhat.com>
Date: Fri, 24 Oct 2014 10:06:55 +0100
Subject: [PATCH 1/2] RESPONDERS: Don't hard-code umask value in utility
ACK
From eb11d17b94f217f7dbfda5f264d46585cd8fbef2 Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)redhat.com>
Date: Fri, 24 Oct 2014 12:42:50 +0100
Subject: [PATCH 2/2] RESPONDERS: Set default value for umask
Resolves:
https://fedorahosted.org/sssd/ticket/
ACK
Although we don't strictly need to set restrictive umask in other
responder than PAM, it's not harmful either and might actually come
handy when/if we add more content..