It is important to know why a syscall failed so pasta should include the errno in the error message. This is still not perfect as we do not know which of functions (open, ioctl, if_nametoindex) failed but it should at least include more important context. This change was inspiered by a podman issue[1]. [1] https://github.com/containers/podman/issues/19428 Signed-off-by: Paul Holzinger <pholzing(a)redhat.com> --- tap.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tap.c b/tap.c index a6a73d3..c212616 100644 --- a/tap.c +++ b/tap.c @@ -1205,7 +1205,8 @@ static void tap_sock_tun_init(struct ctx *c) NS_CALL(tap_ns_tun, c); if (tun_ns_fd == -1) - die("Failed to open tun socket in namespace"); + die("Failed to open tun socket in namespace: %s", + strerror(errno)); pasta_ns_conf(c); -- 2.41.0
On Tue, Aug 01, 2023 at 01:50:17PM +0200, Paul Holzinger wrote:It is important to know why a syscall failed so pasta should include the errno in the error message. This is still not perfect as we do not know which of functions (open, ioctl, if_nametoindex) failed but it should at least include more important context.Uh.. we certainly want this, but I don't think this implementation will quite do it.This change was inspiered by a podman issue[1]. [1] https://github.com/containers/podman/issues/19428 Signed-off-by: Paul Holzinger <pholzing(a)redhat.com> --- tap.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tap.c b/tap.c index a6a73d3..c212616 100644 --- a/tap.c +++ b/tap.c @@ -1205,7 +1205,8 @@ static void tap_sock_tun_init(struct ctx *c) NS_CALL(tap_ns_tun, c);NS_CALL means we're running the function in an ephemeral thread/process/thingy..if (tun_ns_fd == -1) - die("Failed to open tun socket in namespace"); + die("Failed to open tun socket in namespace: %s", + strerror(errno));..which means we can't rely on it actually setting errno in the original process. The ephemeral thread does share memory, but modern errno is a weird per-thread thingy, so I'm not entirely sure what will happen here, but I'm certainly not confident about it working as we'd like. I'll have a crack at a more robust approach today.pasta_ns_conf(c);-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On 02/08/2023 03:39, David Gibson wrote:On Tue, Aug 01, 2023 at 01:50:17PM +0200, Paul Holzinger wrote:Thanks, I only did a quick test with chmod 600 /dev/net/tun and got the expected EACCES so I assumed it worked fine but anyway your series looks much better.It is important to know why a syscall failed so pasta should include the errno in the error message. This is still not perfect as we do not know which of functions (open, ioctl, if_nametoindex) failed but it should at least include more important context.Uh.. we certainly want this, but I don't think this implementation will quite do it.This change was inspiered by a podman issue[1]. [1] https://github.com/containers/podman/issues/19428 Signed-off-by: Paul Holzinger <pholzing(a)redhat.com> --- tap.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tap.c b/tap.c index a6a73d3..c212616 100644 --- a/tap.c +++ b/tap.c @@ -1205,7 +1205,8 @@ static void tap_sock_tun_init(struct ctx *c) NS_CALL(tap_ns_tun, c);NS_CALL means we're running the function in an ephemeral thread/process/thingy..if (tun_ns_fd == -1) - die("Failed to open tun socket in namespace"); + die("Failed to open tun socket in namespace: %s", + strerror(errno));..which means we can't rely on it actually setting errno in the original process. The ephemeral thread does share memory, but modern errno is a weird per-thread thingy, so I'm not entirely sure what will happen here, but I'm certainly not confident about it working as we'd like. I'll have a crack at a more robust approach today.> > pasta_ns_conf(c); >
On Wed, Aug 02, 2023 at 11:51:02AM +0200, Paul Holzinger wrote:On 02/08/2023 03:39, David Gibson wrote:Huh, interesting, maybe it does work. Unless some earlier syscall happened to set errno to EACCES.On Tue, Aug 01, 2023 at 01:50:17PM +0200, Paul Holzinger wrote:Thanks, I only did a quick test with chmod 600 /dev/net/tun and got the expected EACCES so I assumed it worked fineIt is important to know why a syscall failed so pasta should include the errno in the error message. This is still not perfect as we do not know which of functions (open, ioctl, if_nametoindex) failed but it should at least include more important context.Uh.. we certainly want this, but I don't think this implementation will quite do it.This change was inspiered by a podman issue[1]. [1] https://github.com/containers/podman/issues/19428 Signed-off-by: Paul Holzinger <pholzing(a)redhat.com> --- tap.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tap.c b/tap.c index a6a73d3..c212616 100644 --- a/tap.c +++ b/tap.c @@ -1205,7 +1205,8 @@ static void tap_sock_tun_init(struct ctx *c) NS_CALL(tap_ns_tun, c);NS_CALL means we're running the function in an ephemeral thread/process/thingy..if (tun_ns_fd == -1) - die("Failed to open tun socket in namespace"); + die("Failed to open tun socket in namespace: %s", + strerror(errno));..which means we can't rely on it actually setting errno in the original process. The ephemeral thread does share memory, but modern errno is a weird per-thread thingy, so I'm not entirely sure what will happen here, but I'm certainly not confident about it working as we'd like. I'll have a crack at a more robust approach today.but anyway your series looks much better.Yeah, I realized as I was working on it that that approach has the additional advantage of clearly showing which operation failed. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson