Hey Ben,
Have comments inline,
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.
> 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"
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.
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.
> -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)".
> -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?
2. For the case "OVSINST_OFPIT13_METER", there is no check for error.
3. Could we move the error check outside the switch statement? Will shorten
the code.
> @@ -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()".
> @@ -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?
+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.
> 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.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
