[PATCH iproute2 0/3] ip vrf: minor error message cleanups

[PATCH iproute2 0/3] ip vrf: minor error message cleanups

[David Ahern]

David Ahern (3):
  ip vrf: Fix error message when running exec as non-root user
  ip vrf: Improve error message for non-root user
  ip vrf: Clean up bpf related error messages

 ip/ipvrf.c |  6 +++++-
 lib/fs.c   | 16 ++++++++++++----
 2 files changed, 17 insertions(+), 5 deletions(-)

-- 
2.1.4

[PATCH iproute2 1/3] ip vrf: Fix run-on error message on mkdir failure

[David Ahern]

Andy reported a missing newline if a non-root user attempts to run
'ip vrf exec':

$ ./ip/ip vrf exec default /bin/echo asdf
mkdir failed for /var/run/cgroup2: Permission deniedFailed to setup vrf cgroup2 directory

Reported-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
 lib/fs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/fs.c b/lib/fs.c
index 39cc96dccca9..644bb486ae8e 100644
--- a/lib/fs.c
+++ b/lib/fs.c
@@ -121,7 +121,7 @@ int make_path(const char *path, mode_t mode)
 
 			if (mkdir(dir, mode) != 0) {
 				fprintf(stderr,
-					"mkdir failed for %s: %s",
+					"mkdir failed for %s: %s\n",
 					dir, strerror(errno));
 				goto out;
 			}
-- 
2.1.4

[PATCH iproute2 2/3] ip vrf: Improve cgroup2 error messages

[David Ahern]

Currently, if a non-root user attempts to run ip vrf exec a non-helpful
error is returned:

$ ip vrf exec mgmt bash
Failed to mount cgroup2. Are CGROUPS enabled in your kernel?

Only show the CGROUPS kernel hint for the ENODEV error and for the
rest show the strerror for the errno. So now:

$ ip/ip vrf exec mgmt bash
Failed to mount cgroup2: Operation not permitted

Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
 lib/fs.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/lib/fs.c b/lib/fs.c
index 644bb486ae8e..12a4657a0bc9 100644
--- a/lib/fs.c
+++ b/lib/fs.c
@@ -80,13 +80,21 @@ char *find_cgroup2_mount(void)
 
 	if (mount("none", mnt, CGROUP2_FS_NAME, 0, NULL)) {
 		/* EBUSY means already mounted */
-		if (errno != EBUSY) {
+		if (errno == EBUSY)
+			goto out;
+
+		if (errno == ENODEV) {
 			fprintf(stderr,
 				"Failed to mount cgroup2. Are CGROUPS enabled in your kernel?\n");
-			free(mnt);
-			return NULL;
+		} else {
+			fprintf(stderr,
+				"Failed to mount cgroup2: %s\n",
+				strerror(errno));
 		}
+		free(mnt);
+		return NULL;
 	}
+out:
 	return mnt;
 }
 
-- 
2.1.4

Re: [PATCH iproute2 2/3] ip vrf: Improve cgroup2 error messages

[Sergei Shtylyov]

Hello!

On 1/6/2017 3:22 AM, David Ahern wrote:

> Currently, if a non-root user attempts to run ip vrf exec a non-helpful
> error is returned:
>
> $ ip vrf exec mgmt bash
> Failed to mount cgroup2. Are CGROUPS enabled in your kernel?
>
> Only show the CGROUPS kernel hint for the ENODEV error and for the
> rest show the strerror for the errno. So now:
>
> $ ip/ip vrf exec mgmt bash
> Failed to mount cgroup2: Operation not permitted
>
> Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
> ---
>  lib/fs.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/lib/fs.c b/lib/fs.c
> index 644bb486ae8e..12a4657a0bc9 100644
> --- a/lib/fs.c
> +++ b/lib/fs.c
> @@ -80,13 +80,21 @@ char *find_cgroup2_mount(void)
>
>  	if (mount("none", mnt, CGROUP2_FS_NAME, 0, NULL)) {
>  		/* EBUSY means already mounted */
> -		if (errno != EBUSY) {
> +		if (errno == EBUSY)
> +			goto out;
> +
> +		if (errno == ENODEV) {
>  			fprintf(stderr,
>  				"Failed to mount cgroup2. Are CGROUPS enabled in your kernel?\n");
> -			free(mnt);
> -			return NULL;
> +		} else {
> +			fprintf(stderr,
> +				"Failed to mount cgroup2: %s\n",
> +				strerror(errno));
>  		}

    How about a *switch* instead?

> +		free(mnt);
> +		return NULL;
>  	}
> +out:
>  	return mnt;
>  }
>

MBR, Sergei

Re: [PATCH iproute2 2/3] ip vrf: Improve cgroup2 error messages

[David Ahern]

>> @@ -80,13 +80,21 @@ char *find_cgroup2_mount(void)
>>
>>      if (mount("none", mnt, CGROUP2_FS_NAME, 0, NULL)) {
>>          /* EBUSY means already mounted */
>> -        if (errno != EBUSY) {
>> +        if (errno == EBUSY)
>> +            goto out;
>> +
>> +        if (errno == ENODEV) {
>>              fprintf(stderr,
>>                  "Failed to mount cgroup2. Are CGROUPS enabled in your kernel?\n");
>> -            free(mnt);
>> -            return NULL;
>> +        } else {
>> +            fprintf(stderr,
>> +                "Failed to mount cgroup2: %s\n",
>> +                strerror(errno));
>>          }
> 
>    How about a *switch* instead?

I did consider it. Did not make the code simpler or easier to read.

[PATCH iproute2 3/3] ip vrf: Improve bpf error messages

[David Ahern]

Next up a non-root user gets various bpf related error messages:

$ ip vrf exec mgmt bash
Failed to load BPF prog: 'Operation not permitted'
Kernel compiled with CGROUP_BPF enabled?

Catch the EPERM error and do not show the kernel config option.

Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
 ip/ipvrf.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/ip/ipvrf.c b/ip/ipvrf.c
index dc8364a43a57..8bd99d6251f2 100644
--- a/ip/ipvrf.c
+++ b/ip/ipvrf.c
@@ -181,7 +181,11 @@ static int vrf_configure_cgroup(const char *path, int ifindex)
 	if (prog_fd < 0) {
 		fprintf(stderr, "Failed to load BPF prog: '%s'\n",
 			strerror(errno));
-		fprintf(stderr, "Kernel compiled with CGROUP_BPF enabled?\n");
+
+		if (errno != EPERM) {
+			fprintf(stderr,
+				"Kernel compiled with CGROUP_BPF enabled?\n");
+		}
 		goto out;
 	}
 
-- 
2.1.4

Re: [PATCH iproute2 0/3] ip vrf: minor error message cleanups

[Stephen Hemminger]

On Thu,  5 Jan 2017 16:22:20 -0800
David Ahern <dsa@cumulusnetworks.com> wrote:

> David Ahern (3):
>   ip vrf: Fix error message when running exec as non-root user
>   ip vrf: Improve error message for non-root user
>   ip vrf: Clean up bpf related error messages
> 
>  ip/ipvrf.c |  6 +++++-
>  lib/fs.c   | 16 ++++++++++++----
>  2 files changed, 17 insertions(+), 5 deletions(-)
> 

Looks good, applied.