Hello,
please see attached simple patch.
Please note that this patch depends on patch from thread:
RESPONDERS: refactor create_pipe_fd()
Thanks!
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)
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.
Michal
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?
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.
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.
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@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.
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@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@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@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..
On Tue, Oct 28, 2014 at 05:50:19AM +0100, Jakub Hrozek wrote:
From 39277d9408527dd364baba1effb76c4d87789ee7 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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@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..
* master: cbcb834028794a4c658a85965516113f8c0760c1 458f5245dd5130d12666cce6faf8ef1ec7f80169
sssd-devel@lists.fedorahosted.org