| Message ID | 20150219192726.GA17950@alexpilon.ca |
|---|---|
| State | Accepted, archived |
| Delegated to: | stephen hemminger |
| Headers | show |
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 -- 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
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 -- 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
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
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.
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(-)