[PATCH] tap: include errno in error when tap_ns_tun() fails

[PATCH] tap: include errno in error when tap_ns_tun() fails

[Paul Holzinger]

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@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);
 
-- 
@@ -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

Re: [PATCH] tap: include errno in error when tap_ns_tun() fails

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1701 bytes --]

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@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] tap: include errno in error when tap_ns_tun() fails

[Paul Holzinger]

On 02/08/2023 03:39, David Gibson wrote:
> 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@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.

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.

>
>
>>   
>>   	pasta_ns_conf(c);
>>   

Re: [PATCH] tap: include errno in error when tap_ns_tun() fails

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2329 bytes --]

On Wed, Aug 02, 2023 at 11:51:02AM +0200, Paul Holzinger wrote:
> 
> On 02/08/2023 03:39, David Gibson wrote:
> > 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@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.
> 
> Thanks, I only did a quick test with chmod 600 /dev/net/tun and got the
> expected EACCES so I assumed it worked fine

Huh, interesting, maybe it does work.  Unless some earlier syscall
happened to set errno to EACCES.

> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]