Re: [PATCH 2/2] CLEANUP: namespace: remove uneeded ns check in my_socketat
On Thu, Feb 13, 2020 at 08:38:38AM +0100, William Dauchy wrote: (...) > > This allows to remove the !ns || default_namespace logic from > > the function's epilogue. What do you think ? > > Indeed, that's clearer in my opinion. I guess I let you handle that > as you wrote the new proposition. So I tried and it's a real mess :-) We must not put socket-specific stuff in the .h, or it will require tons of #ifdef around includes. I may retry differently later, by having my_socketat() inlined and _my_socketat() in the .c for cases where a namespace is defined. This way we won't need any socket-specific stuff in the .h. Willy
Re: [PATCH 2/2] CLEANUP: namespace: remove uneeded ns check in my_socketat
On Thu, Feb 13, 2020 at 07:48:03AM +0100, Willy Tarreau wrote:
> For me it's different, it's not related to the fact that it's used
> later but to the fact that we only need to undo the namespace change
> if it was changed in the first call. Indeed "ns" is not null only
> when the caller wants to switch the namespace to create a socket. In
> fact as long as there's no namespace configured on the servers, or
> if we're trying to connect to a dispatch or transparent address, ns
> will be NULL and we'll save two setns calls (i.e. almost always the
> case).
indeed, the code was not very clear in that regard, thank you for the
clarification. I overlooked the case we only go through this function to
get a file descriptor without namespace.
> In fact I think we could simplify the logic a bit and merge the code
> into the inline function present in common/namespace.h. This would also
> require to export default_namespace:
>
>
> static inline int my_socketat(const struct netns_entry *ns, int domain, int
> type, int protocol)
> {
> #ifdef USE_NS
> int sock;
>
> if (likely(!ns || default_namespace < 0))
> goto no_ns;
>
> if (setns(ns->fd, CLONE_NEWNET) == -1)
> return -1;
>
> sock = socket(domain, type, protocol);
>
> if (setns(default_namespace, CLONE_NEWNET) == -1) {
> close(sock);
> sock = -1;
> }
> return sock;
> no_ns:
> #endif
> return socket(domain, type, protocol);
> }
>
> This allows to remove the !ns || default_namespace logic from
> the function's epilogue. What do you think ?
Indeed, that's clearer in my opinion. I guess I let you handle that
as you wrote the new proposition.
Thank you,
--
William
Re: [PATCH 2/2] CLEANUP: namespace: remove uneeded ns check in my_socketat
On Wed, Feb 12, 2020 at 09:42:06PM +0100, William Dauchy wrote:
> On Thu, Feb 13, 2020 at 01:31:51AM +0500, ??? wrote:
> > we "use" it.
> > depending on true/false we either return -1 or not
>
> I guess it is present in the first condition to be able to access
> `ns->fd` safely in setns; but the second condition does not acces `ns`
> later.
For me it's different, it's not related to the fact that it's used
later but to the fact that we only need to undo the namespace change
if it was changed in the first call. Indeed "ns" is not null only
when the caller wants to switch the namespace to create a socket. In
fact as long as there's no namespace configured on the servers, or
if we're trying to connect to a dispatch or transparent address, ns
will be NULL and we'll save two setns calls (i.e. almost always the
case).
In fact I think we could simplify the logic a bit and merge the code
into the inline function present in common/namespace.h. This would also
require to export default_namespace:
static inline int my_socketat(const struct netns_entry *ns, int domain, int
type, int protocol)
{
#ifdef USE_NS
int sock;
if (likely(!ns || default_namespace < 0))
goto no_ns;
if (setns(ns->fd, CLONE_NEWNET) == -1)
return -1;
sock = socket(domain, type, protocol);
if (setns(default_namespace, CLONE_NEWNET) == -1) {
close(sock);
sock = -1;
}
return sock;
no_ns:
#endif
return socket(domain, type, protocol);
}
This allows to remove the !ns || default_namespace logic from
the function's epilogue. What do you think ?
Willy
Re: [PATCH 2/2] CLEANUP: namespace: remove uneeded ns check in my_socketat
On Thu, Feb 13, 2020 at 01:31:51AM +0500, Илья Шипицин wrote: > we "use" it. > depending on true/false we either return -1 or not I guess it is present in the first condition to be able to access `ns->fd` safely in setns; but the second condition does not acces `ns` later. -- William
Re: [PATCH 2/2] CLEANUP: namespace: remove uneeded ns check in my_socketat
чт, 13 февр. 2020 г. в 01:26, William Dauchy :
> we check ns variable but we don't use it later
>
we "use" it.
depending on true/false we either return -1 or not
>
> Signed-off-by: William Dauchy
> ---
> src/namespace.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/namespace.c b/src/namespace.c
> index 89a968e36..3536629bc 100644
> --- a/src/namespace.c
> +++ b/src/namespace.c
> @@ -120,7 +120,7 @@ int my_socketat(const struct netns_entry *ns, int
> domain, int type, int protocol
>
> sock = socket(domain, type, protocol);
>
> - if (default_namespace >= 0 && ns && setns(default_namespace,
> CLONE_NEWNET) == -1) {
> + if (default_namespace >= 0 && setns(default_namespace,
> CLONE_NEWNET) == -1) {
> if (sock >= 0)
> close(sock);
> return -1;
> --
> 2.25.0
>
>
>
[PATCH 2/2] CLEANUP: namespace: remove uneeded ns check in my_socketat
we check ns variable but we don't use it later
Signed-off-by: William Dauchy
---
src/namespace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/namespace.c b/src/namespace.c
index 89a968e36..3536629bc 100644
--- a/src/namespace.c
+++ b/src/namespace.c
@@ -120,7 +120,7 @@ int my_socketat(const struct netns_entry *ns, int domain,
int type, int protocol
sock = socket(domain, type, protocol);
- if (default_namespace >= 0 && ns && setns(default_namespace,
CLONE_NEWNET) == -1) {
+ if (default_namespace >= 0 && setns(default_namespace, CLONE_NEWNET) ==
-1) {
if (sock >= 0)
close(sock);
return -1;
--
2.25.0
