[ovs-dev] [PATCH] ofp-parse: Do not exit() upon a parse error.
Ben Pfaff
blp at nicira.com
Mon Jul 8 17:13:40 UTC 2013
On Tue, Jul 02, 2013 at 04:51:45PM -0700, Alex Wang wrote:
> diff --git a/lib/meta-flow.c b/lib/meta-flow.c
> > index 0677202..e66987c 100644
> > --- a/lib/meta-flow.c
> > +++ b/lib/meta-flow.c
> > @@ -2699,21 +2699,22 @@ mf_parse_subfield__(struct mf_subfield *sf, const
> > char **sp)
> > /* Parses a subfield from the beginning of 's' into 'sf'. Returns the
> > first
> > * byte in 's' following the parsed string.
> > *
> > - * Exits with an error message if 's' has incorrect syntax.
> > + * Returns NULL if successful, otherwise a malloc()'d string describing
> > the
> > + * error. The caller is responsible for freeing the returned string.
> > *
> >
>
> The comment " Returns the first byte in 's' following the parsed string."
> should be removed.
Oops, thank you, fixed.
> > void
> > diff --git a/lib/meta-flow.h b/lib/meta-flow.h
> > index a85a193..bc402dc 100644
> > --- a/lib/meta-flow.h
> > +++ b/lib/meta-flow.h
> > @@ -360,8 +360,10 @@ uint64_t mf_get_subfield(const struct mf_subfield *,
> > const struct flow *);
> >
> >
> > void mf_format_subfield(const struct mf_subfield *, struct ds *);
> > -char *mf_parse_subfield__(struct mf_subfield *sf, const char **s);
> > -const char *mf_parse_subfield(struct mf_subfield *, const char *);
> > +char *mf_parse_subfield__(struct mf_subfield *sf, const char **s)
> > + WARN_UNUSED_RESULT;
> > +char *mf_parse_subfield(struct mf_subfield *, const char *s)
> > + WARN_UNUSED_RESULT;
> >
> >
>
> The function attribute "WARN_UNUSED_RESULT" should be added to
> "mf_parse_subfield__()" in "meta-flow.c"
Thanks, added.
> diff --git a/lib/nx-match.c b/lib/nx-match.c
> index 8bdd8ec..3a6d7cc 100644
> --- a/lib/nx-match.c
> +++ b/lib/nx-match.c
>
> > +char * WARN_UNUSED_RESULT
> > nxm_parse_stack_action(struct ofpact_stack *stack_action, const char *s)
> > {
> > - s = mf_parse_subfield(&stack_action->subfield, s);
> > + char *error;
> > +
> > + error = mf_parse_subfield__(&stack_action->subfield, &s);
> > + if (error) {
> > + return error;
> > + }
> > +
> > if (*s != '\0') {
> > - ovs_fatal(0, "%s: trailing garbage following push or pop", s);
> > + return xasprintf("%s: trailing garbage following push or pop", s);
> > }
> > +
> > + return NULL;
> > }
>
> Is the check "trailing garbage" error message very important? If not, we
> could just use "mf_parse_subfield()" and remove that check.
It's nice to have, so I'd prefer to keep it.
> diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
> > index da36f88..ced6bcd 100644
> > --- a/lib/ofp-parse.c
> > +++ b/lib/ofp-parse.c
> > @@ -41,123 +41,196 @@
> >
> > VLOG_DEFINE_THIS_MODULE(ofp_parse);
> >
> > -static void ofp_fatal(const char *flow, bool verbose, const char *format,
> > ...)
> > - NO_RETURN;
> >
> > -static uint16_t
> > +str_to_u8(const char *str, const char *name, uint8_t *valuep)
>
> Would you like to swap the "str_to_u16()" and "str_to_u8()"? so that it
> goes in increasing order of bits of uint.
Good idea. Done.
> > -static void
> > +/* Parses 'arg' as the argument to an "enqueue" action, and appends such
> > an
> > + * action to 'ofpacts'.
> > + *
> > + * Returns NULL if successful, otherwise a malloc()'d string describing
> > the
> > + * error. The caller is responsible for freeing the returned string. */
> > +static char * WARN_UNUSED_RESULT
> > parse_enqueue(char *arg, struct ofpbuf *ofpacts)
> > {
> > char *sp = NULL;
> > char *port = strtok_r(arg, ":q", &sp);
> > char *queue = strtok_r(NULL, "", &sp);
> > struct ofpact_enqueue *enqueue;
> > + char *error;
> >
> > if (port == NULL || queue == NULL) {
> > - ovs_fatal(0, "\"enqueue\" syntax is \"enqueue:PORT:QUEUE\"");
> > + return xstrdup("\"enqueue\" syntax is \"enqueue:PORT:QUEUE\"");
> > }
> >
> > enqueue = ofpact_put_ENQUEUE(ofpacts);
> > - enqueue->port = u16_to_ofp(str_to_u32(port));
> > - enqueue->queue = str_to_u32(queue);
> > + if (!ofputil_port_from_string(port, &enqueue->port)) {
> > + return xasprintf("%s: enqueue to unknown port", port);
> > + }
> > + if (!error) {
> > + error = str_to_u32(queue, &enqueue->queue);
> > + }
> > + return error;
> > }
> >
>
>
> The pointer error is not initialized, should remove the if statement and
> return just "str_to_u32(queue, &enqueue->queue)".
Ouch, good catch. I wish the compiler would report obvious idiocies
like this.
> > -static void
> > +/* Parses 'arg' as the argument to instruction 'type', and appends such an
> > + * instruction to 'ofpacts'.
> > + *
> > + * Returns NULL if successful, otherwise a malloc()'d string describing
> > the
> > + * error. The caller is responsible for freeing the returned string. */
> > +static char * WARN_UNUSED_RESULT
> > parse_named_instruction(enum ovs_instruction_type type,
> > char *arg, struct ofpbuf *ofpacts)
> > {
> > enum ofperr error;
> > + char *error_s;
> >
> > switch (type) {
> > case OVSINST_OFPIT11_APPLY_ACTIONS:
> > @@ -698,28 +922,33 @@ parse_named_instruction(enum ovs_instruction_type
> > type,
> >
> > case OVSINST_OFPIT11_WRITE_ACTIONS:
> > /* XXX */
> > - ovs_fatal(0, "instruction write-actions is not supported yet");
> > - break;
> > + return xstrdup("instruction write-actions is not supported yet");
> >
> > case OVSINST_OFPIT11_CLEAR_ACTIONS:
> > ofpact_put_CLEAR_ACTIONS(ofpacts);
> > break;
> >
> > case OVSINST_OFPIT13_METER:
> > - ofpact_put_METER(ofpacts)->meter_id = str_to_u32(arg);
> > + error_s = str_to_u32(arg, &ofpact_put_METER(ofpacts)->meter_id);
> > break;
> >
> > case OVSINST_OFPIT11_WRITE_METADATA:
> > - parse_metadata(ofpacts, arg);
> > + error_s = parse_metadata(ofpacts, arg);
> > + if (error_s) {
> > + return error_s;
> > + }
> > break;
> >
> > case OVSINST_OFPIT11_GOTO_TABLE: {
> > struct ofpact_goto_table *ogt = ofpact_put_GOTO_TABLE(ofpacts);
> > char *table_s = strsep(&arg, ",");
> > if (!table_s || !table_s[0]) {
> > - ovs_fatal(0, "instruction goto-table needs table id");
> > + return xstrdup("instruction goto-table needs table id");
> > + }
> > + error_s = str_to_u8(table_s, "table", &ogt->table_id);
> > + if (error_s) {
> > + return error_s;
> > }
> > - ogt->table_id = str_to_u8(table_s, "table");
> > break;
> > }
> > }
>
> 1. Could you explain why the last case "OVSINST_OFPIT11_GOTO_TABLE" is
> enclosed by curly bracket?
It has a local variable, so the braces are necessary.
> 2. For the case "OVSINST_OFPIT13_METER", there is no check for error.
Oops, I've now added:
if (error_s) {
return error_s;
}
> 3. Could we move the error check outside the switch statement? Will shorten
> the code.
Yes, good point. Done.
> > @@ -913,13 +1140,13 @@ parse_ofp_str(struct ofputil_flow_mod *fm, int
> > command, const char *str_,
> > if (fields & F_ACTIONS) {
> > act_str = strstr(string, "action");
> > if (!act_str) {
> > - ofp_fatal(str_, verbose, "must specify an action");
> > + return xasprintf("must specify an action");
> > }
> > *act_str = '\0';
> >
> > act_str = strchr(act_str + 1, '=');
> > if (!act_str) {
> > - ofp_fatal(str_, verbose, "must specify an action");
> > + return xasprintf("must specify an action");
> > }
>
> Could you use "xstrdup()" here? since previously, if there is no
> formatting, we always use "xstrdup()".
Fixed, thanks.
> > @@ -948,44 +1176,50 @@ parse_ofp_str(struct ofputil_flow_mod *fm, int
> > command, const char *str_,
> >
> > value = strtok_r(NULL, ", \t\r\n", &save_ptr);
> > if (!value) {
> > - ofp_fatal(str_, verbose, "field %s missing value", name);
> > + return xasprintf("field %s missing value", name);
> > }
> >
> > if (!strcmp(name, "table")) {
> > - fm->table_id = str_to_u8(value, name);
> > + error = str_to_u8(value, "table", &fm->table_id);
> > } else if (!strcmp(name, "out_port")) {
> > if (!ofputil_port_from_string(value, &fm->out_port)) {
> > - ofp_fatal(str_, verbose, "%s is not a valid OpenFlow
> > port",
> > - name);
> > + error = xasprintf("%s is not a valid OpenFlow port",
> > + value);
> > }
> > } else if (fields & F_PRIORITY && !strcmp(name, "priority")) {
> > - fm->priority = str_to_u16(value, name);
> > + uint16_t priority;
> > +
> > + error = str_to_u16(value, name, &priority);
> > + fm->priority = priority;
> > } else if (fields & F_TIMEOUT && !strcmp(name,
> > "idle_timeout")) {
> > - fm->idle_timeout = str_to_u16(value, name);
> > + error = str_to_u16(value, name, &fm->idle_timeout);
> > } else if (fields & F_TIMEOUT && !strcmp(name,
> > "hard_timeout")) {
> > - fm->hard_timeout = str_to_u16(value, name);
> > + error = str_to_u16(value, name, &fm->hard_timeout);
> > } else if (!strcmp(name, "cookie")) {
> > char *mask = strchr(value, '/');
> >
> > if (mask) {
> > /* A mask means we're searching for a cookie. */
> > if (command == OFPFC_ADD) {
> > - ofp_fatal(str_, verbose, "flow additions cannot
> > use "
> > - "a cookie mask");
> > + return xstrdup("flow additions cannot use "
> > + "a cookie mask");
> > }
> > *mask = '\0';
> > - fm->cookie = htonll(str_to_u64(value));
> > - fm->cookie_mask = htonll(str_to_u64(mask+1));
> > + error = str_to_be64(value, &fm->cookie);
> > + if (error) {
> > + return error;
> > + }
> > + error = str_to_be64(mask + 1, &fm->cookie_mask);
> > } else {
> > /* No mask means that the cookie is being set. */
> > if (command != OFPFC_ADD && command != OFPFC_MODIFY
> > - && command != OFPFC_MODIFY_STRICT) {
> > - ofp_fatal(str_, verbose, "cannot set cookie");
> > + && command != OFPFC_MODIFY_STRICT) {
> > + return xstrdup("cannot set cookie");
> > }
> > - fm->new_cookie = htonll(str_to_u64(value));
> > + error = str_to_be64(value, &fm->new_cookie);
> > }
> > } else if (mf_from_name(name)) {
> > - parse_field(mf_from_name(name), value, &fm->match);
> > + error = parse_field(mf_from_name(name), value,
> > &fm->match);
> > } else if (!strcmp(name, "duration")
> > || !strcmp(name, "n_packets")
> > || !strcmp(name, "n_bytes")
> > @@ -995,12 +1229,16 @@ parse_ofp_str(struct ofputil_flow_mod *fm, int
> > command, const char *str_,
> > * "ovs-ofctl dump-flows" back into commands that parse
> > * flows. */
> > } else {
> > - ofp_fatal(str_, verbose, "unknown keyword %s", name);
> > + error = xasprintf("unknown keyword %s", name);
> > + }
> > +
> > + if (error) {
> > + return error;
> > }
> > }
> > }
>
> Just want to ask and make sure that there will not be multiple hits in the
> if-else-if statement above, right?
Multiple hits? What do you mean?
> +char * WARN_UNUSED_RESULT
> > parse_ofp_flow_mod_file(const char *file_name, uint16_t command,
> > struct ofputil_flow_mod **fms, size_t *n_fms)
> > {
> > size_t allocated_fms;
> > + int line_number;
> > FILE *stream;
> > struct ds s;
> >
> > + *fms = NULL;
> > + *n_fms = 0;
> > +
> > stream = !strcmp(file_name, "-") ? stdin : fopen(file_name, "r");
> > if (stream == NULL) {
> > - ovs_fatal(errno, "%s: open", file_name);
> > + return xasprintf("%s: open failed (%s)", file_name,
> > strerror(errno));
> > }
> >
> > allocated_fms = *n_fms;
> > ds_init(&s);
> > - while (!ds_get_preprocessed_line(&s, stream)) {
> > + line_number = 0;
> > + while (!ds_get_preprocessed_line(&s, stream, &line_number)) {
> > + char *error;
> > +
> > if (*n_fms >= allocated_fms) {
> > *fms = x2nrealloc(*fms, &allocated_fms, sizeof **fms);
> > }
> > - parse_ofp_flow_mod_str(&(*fms)[*n_fms], ds_cstr(&s), command,
> > false);
> > + error = parse_ofp_flow_mod_str(&(*fms)[*n_fms], ds_cstr(&s),
> > command);
> > + if (error) {
> > + size_t i;
> > +
> > + for (i = 0; i < *n_fms; i++) {
> > + free((*fms)[i].ofpacts);
> > + }
> > + free(*fms);
> > + *fms = NULL;
> > + *n_fms = 0;
> > +
> > + return xasprintf("%s:%d: %s", file_name, line_number, error);
> > + }
> > *n_fms += 1;
> > }
> > ds_destroy(&s);
> >
>
>
> Should ds_destroy(&s) before return error message.
Good catch, thank you. Added.
> > diff --git a/tests/learn.at b/tests/learn.at
> > index ec1c347..ce810b4 100644
> > --- a/tests/learn.at
> > +++ b/tests/learn.at
> > @@ -75,11 +75,13 @@ AT_CHECK([[ovs-ofctl parse-flow
> > 'actions=learn(load:5->NXM_OF_IP_DST[])']],
> > [1], [], [stderr])
> > AT_CHECK([sed -e 's/.*|meta_flow|WARN|//' < stderr], [0],
> > [[destination field ip_dst lacks correct prerequisites
> > +ovs-ofctl: actions are invalid with specified match (OFPBAC_BAD_ARGUMENT)
> > ]], [[]])
> > AT_CHECK([[ovs-ofctl parse-flow
> > 'actions=learn(load:NXM_OF_IP_DST[]->NXM_NX_REG1[])']],
> > [1], [], [stderr])
> > AT_CHECK([sed -e 's/.*|meta_flow|WARN|//' < stderr], [0],
> > [[source field ip_dst lacks correct prerequisites
> > +ovs-ofctl: actions are invalid with specified match (OFPBAC_BAD_ARGUMENT)
> > ]])
> > AT_CLEANUP
>
>
> Actually, I'm surprised by how few tests are modified. I'll do a scan
> again, after this patch is updated.
This indicates that we have poor coverage of "negative" tests (the
ones that provoke error messages). It would be good to fix that.
Thanks for the review. I'll send out a v2 soon.
More information about the dev
mailing list