Re: [PATCH] Fallback to direct access if lacking pemission for linux namespaces

[Andrew Burgess <aburgess@redhat.com>]

Tyson Whitehead via Gdb-patches <gdb-patches@sourceware.org> writes:

> The case where gdb and the target had the same path always used to
> work. Now it fails if gdb and the target are in different namespaces
> and gdb doesn't have permission to enter the target namespace.

So I just read the bug report, and I think you did a better job there of
explaining the background of why you wrote this patch.  You should
include the history in the commit message, instead of just saying that
something used to work and now fails.

> This commit causes gdb to fall back to trying direct access should
> it lack permission to enter the namespace. This way it does not
> break a case that used to work or require capabilites not required.

Out of interest, did you consider doing:

  (gdb) set sysroot

as a mechanism to solve your problem?  This feels like a better
solution to me, as this better describes your local setup.

The problem I see with your proposal is that in the case where the
namespace path _isn't_ visible to GDB, and if for some reason GDB isn't
able to setns(), then the errors the user sees will be for GDB trying to
open the namepsace path as if it were local -- which might give the
impression that GDB is doing the wrong thing.

If we are going to fall back to attempting local access (as a last
ditched effort), then we should, at the very least, emit a warning to
let the user know that we couldn't setns() and that we're trying local
access as a last resort.

>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30113
> ---
>  gdb/nat/linux-namespaces.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/gdb/nat/linux-namespaces.c b/gdb/nat/linux-namespaces.c
> index 4b1fee18425..8922717be10 100644
> --- a/gdb/nat/linux-namespaces.c
> +++ b/gdb/nat/linux-namespaces.c
> @@ -935,6 +935,13 @@ linux_mntns_access_fs (pid_t pid)
>  	  if (error == ENOSYS)
>  	    error = ENOTSUP;
>  
> +	  /* EPERM indicates the required capabilites are not available
> +	     for this. Fall back to the old direct gdb behaviour in
> +	     order to not break cases where this used to work as the
> +	     path could still be the same in both namespaces. */

The reference to "old direct gdb behaviour" here should be removed.
It's OK to have comments like "This code exists for backwards
compatibility with old GDB behaviour...", but that isn't the case here.

The comment should justify the new code in terms of the desired
behaviour of GDB, so something like:

  /* EPERM indicates the required capabilites to setns() are not
     available.  It is possible that the file is directly visible to
     GDB, so, rather than just failing at this point, lets at least try
     direct access.  */
  if (error == EPERM)
    {
      warning (_("blah blah something about trying direct access"));
      return MNSH_FS_DIRECT;
    }

Though I would recommend trying 'set sysroot' and seeing if that solves
the access problems you are seeing.

Thanks,
Andrew

> +	  if (error == EPERM)
> +	    return MNSH_FS_DIRECT;
> +
>  	  errno = error;
>  	  return MNSH_FS_ERROR;
>  	}
> -- 
> 2.40.1