[PATCH] Allow specifying bridge port STP state by name rather than number.

[PATCH] Allow specifying bridge port STP state by name rather than number.

[Alex Pilon]

The existing behaviour forces one to memorize the integer constants for
STP port states.

    # bridge link set dev dummy0 state 3

This patch makes it possible to use the lowercased port state name.

    # bridge link set dev dummy0 state forwarding

Invalid non-integer inputs now cause exit with status -1.

Signed-off-by: Alex Pilon <alp@alexpilon.ca>
---
 bridge/link.c     | 14 +++++++++++++-
 man/man8/bridge.8 |  4 +++-
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/bridge/link.c b/bridge/link.c
index c8555f8..a7bd85f 100644
--- a/bridge/link.c
+++ b/bridge/link.c
@@ -316,7 +316,19 @@ static int brlink_modify(int argc, char **argv)
 			priority = atoi(*argv);
 		} else if (strcmp(*argv, "state") == 0) {
 			NEXT_ARG();
-			state = atoi(*argv);
+			char *endptr;
+			size_t nstates = sizeof(port_states) / sizeof(*port_states);
+			state = strtol(*argv, &endptr, 10);
+			if (!(**argv != '\0' && *endptr == '\0')) {
+				for (state = 0; state < nstates; state++)
+					if (strcmp(port_states[state], *argv) == 0)
+						break;
+				if (state == nstates) {
+					fprintf(stderr,
+						"Error: invalid STP port state\n");
+					exit(-1);
+				}
+			}
 		} else if (strcmp(*argv, "hwmode") == 0) {
 			NEXT_ARG();
 			flags = BRIDGE_FLAGS_SELF;
diff --git a/man/man8/bridge.8 b/man/man8/bridge.8
index e344db2..68ad71e 100644
--- a/man/man8/bridge.8
+++ b/man/man8/bridge.8
@@ -207,7 +207,9 @@ droot port selectio algorithms.
 .TP
 .BI state " STATE "
 the operation state of the port.  This is primarily used by user space STP/RSTP
-implementation.  The following is a list of valid values:
+implementation.  One may enter a lowercased port state name, or one of the
+numbers below.  Negative inputs are ignored, and unrecognized names return an
+error.
 
 .B 0
 - port is DISABLED.  Make this port completely inactive.
-- 
2.3.0

Re: [PATCH] Allow specifying bridge port STP state by name rather than number.

[Jonathon Reinhart]

Please don't pass -1 to exit().  It is outside the acceptable range (0 to 255)
of exit status values:

https://www.gnu.org/software/libc/manual/html_node/Exit-Status.html

Most programs exit(1) when presented with invalid user input.

On Thu, Feb 19, 2015 at 2:27 PM, Alex Pilon <alp@alexpilon.ca> wrote:
> The existing behaviour forces one to memorize the integer constants for
> STP port states.
>
>     # bridge link set dev dummy0 state 3
>
> This patch makes it possible to use the lowercased port state name.
>
>     # bridge link set dev dummy0 state forwarding
>
> Invalid non-integer inputs now cause exit with status -1.
>
> Signed-off-by: Alex Pilon <alp@alexpilon.ca>
> ---
>  bridge/link.c     | 14 +++++++++++++-
>  man/man8/bridge.8 |  4 +++-
>  2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/bridge/link.c b/bridge/link.c
> index c8555f8..a7bd85f 100644
> --- a/bridge/link.c
> +++ b/bridge/link.c
> @@ -316,7 +316,19 @@ static int brlink_modify(int argc, char **argv)
>                         priority = atoi(*argv);
>                 } else if (strcmp(*argv, "state") == 0) {
>                         NEXT_ARG();
> -                       state = atoi(*argv);
> +                       char *endptr;
> +                       size_t nstates = sizeof(port_states) / sizeof(*port_states);
> +                       state = strtol(*argv, &endptr, 10);
> +                       if (!(**argv != '\0' && *endptr == '\0')) {
> +                               for (state = 0; state < nstates; state++)
> +                                       if (strcmp(port_states[state], *argv) == 0)
> +                                               break;
> +                               if (state == nstates) {
> +                                       fprintf(stderr,
> +                                               "Error: invalid STP port state\n");
> +                                       exit(-1);
> +                               }
> +                       }
>                 } else if (strcmp(*argv, "hwmode") == 0) {
>                         NEXT_ARG();
>                         flags = BRIDGE_FLAGS_SELF;
> diff --git a/man/man8/bridge.8 b/man/man8/bridge.8
> index e344db2..68ad71e 100644
> --- a/man/man8/bridge.8
> +++ b/man/man8/bridge.8
> @@ -207,7 +207,9 @@ droot port selectio algorithms.
>  .TP
>  .BI state " STATE "
>  the operation state of the port.  This is primarily used by user space STP/RSTP
> -implementation.  The following is a list of valid values:
> +implementation.  One may enter a lowercased port state name, or one of the
> +numbers below.  Negative inputs are ignored, and unrecognized names return an
> +error.
>
>  .B 0
>  - port is DISABLED.  Make this port completely inactive.
> --
> 2.3.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Allow specifying bridge port STP state by name rather than number.

[Alex Pilon]

On Thu, Feb 19, 2015 at 09:00:23PM -0500, Jonathon Reinhart wrote:
> Please don't pass -1 to exit().  It is outside the acceptable range (0 to 255)
> of exit status values:
>
> https://www.gnu.org/software/libc/manual/html_node/Exit-Status.html
>
> Most programs exit(1) when presented with invalid user input.

I was being consistent with nearby code, and the dominant style.  But if
that's the case, then there's a few hundred such mistakes that need
correcting.  Should I fix all of them, and if so, should that be a
single patch, or multiple?

There's also only a few instances of EXIT_{SUCCESS,FAILURE}, so I took
it wasn't the style to use that.

Regards,

Alex Pilon

Re: [PATCH] Allow specifying bridge port STP state by name rather than number.

[Jonathon Reinhart]

Sorry, I only looked at your patch, and not the surrounding code. In that
case, I would probably stick with the existing style.

I'm just a casual passer-by, but I would like to hear others' opinion on this,
as I find exit(-1) to always be incorrect. The problem is that, per the man
page:

> The exit() function causes normal process termination and the value
> of status & 0377 is returned to the parent (see wait(2)).

So when -1 is passed to exit(), the parent will see it as 255. Why not
return a value that could be found in the source by grepping for it?

On Thu, Feb 19, 2015 at 11:57 PM, Alex Pilon <alp@alexpilon.ca> wrote:
> On Thu, Feb 19, 2015 at 09:00:23PM -0500, Jonathon Reinhart wrote:
>> Please don't pass -1 to exit().  It is outside the acceptable range (0 to 255)
>> of exit status values:
>>
>> https://www.gnu.org/software/libc/manual/html_node/Exit-Status.html
>>
>> Most programs exit(1) when presented with invalid user input.
>
> I was being consistent with nearby code, and the dominant style.  But if
> that's the case, then there's a few hundred such mistakes that need
> correcting.  Should I fix all of them, and if so, should that be a
> single patch, or multiple?
>
> There's also only a few instances of EXIT_{SUCCESS,FAILURE}, so I took
> it wasn't the style to use that.
>
> Regards,
>
> Alex Pilon



-- 
Computers are incredibly fast, accurate and stupid. Human beings are
incredibly slow, inaccurate and brilliant. Together they are powerful
beyond imagination.
                                    A. Einstein