* [PATCH] Allow specifying bridge port STP state by name rather than number.
@ 2015-02-19 19:27 Alex Pilon
2015-02-20 2:00 ` Jonathon Reinhart
0 siblings, 1 reply; 4+ messages in thread
From: Alex Pilon @ 2015-02-19 19:27 UTC (permalink / raw)
To: netdev
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] Allow specifying bridge port STP state by name rather than number.
2015-02-19 19:27 [PATCH] Allow specifying bridge port STP state by name rather than number Alex Pilon
@ 2015-02-20 2:00 ` Jonathon Reinhart
2015-02-20 4:57 ` Alex Pilon
0 siblings, 1 reply; 4+ messages in thread
From: Jonathon Reinhart @ 2015-02-20 2:00 UTC (permalink / raw)
To: Alex Pilon; +Cc: netdev
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Allow specifying bridge port STP state by name rather than number.
2015-02-20 2:00 ` Jonathon Reinhart
@ 2015-02-20 4:57 ` Alex Pilon
2015-02-20 14:40 ` Jonathon Reinhart
0 siblings, 1 reply; 4+ messages in thread
From: Alex Pilon @ 2015-02-20 4:57 UTC (permalink / raw)
To: Jonathon Reinhart; +Cc: netdev
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Allow specifying bridge port STP state by name rather than number.
2015-02-20 4:57 ` Alex Pilon
@ 2015-02-20 14:40 ` Jonathon Reinhart
0 siblings, 0 replies; 4+ messages in thread
From: Jonathon Reinhart @ 2015-02-20 14:40 UTC (permalink / raw)
To: Alex Pilon; +Cc: netdev
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-02-20 14:41 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-02-19 19:27 [PATCH] Allow specifying bridge port STP state by name rather than number Alex Pilon
2015-02-20 2:00 ` Jonathon Reinhart
2015-02-20 4:57 ` Alex Pilon
2015-02-20 14:40 ` Jonathon Reinhart
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.