Some notes in passing, please forgive any ignorance
On Thu, Jun 16, 2011 at 5:08 PM, Andrew Evans <[email protected]> wrote:
> On Thu, 2011-06-16 at 14:12 -0700, Ben Pfaff wrote:
> > On Wed, Jun 15, 2011 at 06:55:38PM -0700, Andrew Evans wrote:
> > > When an error is encountered while parsing flows from a file, ovs-ofctl
> doesn't
> > > print the flow it can't parse, so it's not always obvious which flow is
> causing
> > > the error. Print the flow before the error message to make it clear.
> >
> > Could you write a helper function to do the job of this:
> >
> > if (verbose) {
> > fprintf(stderr, "%s:\n", str_);
> > }
> > ovs_fatal(0, "must specify an action");
>
> Ok, this looks a bit tidier:
>
> ---
> lib/ofp-parse.c | 45 +++++++++++++++++++++++++++++++++------------
> lib/ofp-parse.h | 5 +++--
> utilities/ovs-ofctl.c | 6 +++---
> 3 files changed, 39 insertions(+), 17 deletions(-)
>
> diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
> index a4679a3..f0b943d 100644
> --- a/lib/ofp-parse.c
> +++ b/lib/ofp-parse.c
> @@ -614,6 +614,19 @@ struct field {
> flow_wildcards_t wildcard; /* FWW_* bit. */
> };
>
> +static void
> +ofp_fatal(bool verbose, const char *flow, const char *format, ...)
> +{
> + va_list args;
> +
> + if (verbose) {
> + fprintf(stderr, "%s:\n", flow);
> + }
> +
> + va_start(args, format);
> + ovs_fatal_valist(0, format, args);
> +}
> +
> static bool
> parse_field_name(const char *name, const struct field **f_out)
> {
> @@ -777,12 +790,14 @@ parse_reg_value(struct cls_rule *rule, int reg_idx,
> const char *value)
> }
> }
>
> -/* Convert 'string' (as described in the Flow Syntax section of the
> ovs-ofctl
> - * man page) into 'pf'. If 'actions' is specified, an action must be in
> +/* Convert 'str_' (as described in the Flow Syntax section of the
> ovs-ofctl
> + * man page) into 'fm'. If 'actions' is specified, an action must be in
> * 'string' and may be expanded or reallocated. */
> void
> -parse_ofp_str(struct flow_mod *fm, struct ofpbuf *actions, char *string)
> +parse_ofp_str(struct flow_mod *fm, struct ofpbuf *actions, const char
> *str_,
> + bool verbose)
> {
> + char *string = xstrdup(str_);
> char *save_ptr = NULL;
> char *name;
>
> @@ -798,13 +813,13 @@ parse_ofp_str(struct flow_mod *fm, struct ofpbuf
> *actions, char *string)
> if (actions) {
> char *act_str = strstr(string, "action");
> if (!act_str) {
> - ovs_fatal(0, "must specify an action");
> + ofp_fatal(verbose, str_, "must specify an action");
> }
> *act_str = '\0';
>
> act_str = strchr(act_str + 1, '=');
> if (!act_str) {
> - ovs_fatal(0, "must specify an action");
> + ofp_fatal(verbose, str_, "must specify an action");
> }
>
> act_str++;
> @@ -831,7 +846,7 @@ parse_ofp_str(struct flow_mod *fm, struct ofpbuf
> *actions, char *string)
>
> value = strtok_r(NULL, ", \t\r\n", &save_ptr);
> if (!value) {
> - ovs_fatal(0, "field %s missing value", name);
> + ofp_fatal(verbose, str_, "field %s missing value", name);
> }
>
> if (!strcmp(name, "table")) {
> @@ -875,7 +890,10 @@ parse_ofp_str(struct flow_mod *fm, struct ofpbuf
> *actions, char *string)
> && isdigit((unsigned char) name[3])) {
> unsigned int reg_idx = atoi(name + 3);
> if (reg_idx >= FLOW_N_REGS) {
> - ovs_fatal(0, "only %d registers supported",
> FLOW_N_REGS);
> + if (verbose) {
> + fprintf(stderr, "%s:\n", str_);
> + }
> + ovs_fatal(verbose, str_, "only %d registers
> supported", FLOW_N_REGS);
>
Is this on purpose?
> }
> parse_reg_value(&fm->cr, reg_idx, value);
> } else if (!strcmp(name, "duration")
> @@ -885,10 +903,12 @@ parse_ofp_str(struct flow_mod *fm, struct ofpbuf
> *actions, char *string)
> * "ovs-ofctl dump-flows" back into commands that parse
> * flows. */
> } else {
> - ovs_fatal(0, "unknown keyword %s", name);
> + ovs_fatal(verbose, str_, "unknown keyword %s", name);
>
Should this be ofp_fatal?
> }
> }
> }
> +
> + free(string);
>
I assume we don't care about missing this if you hit a fatal.
> }
>
> /* Parses 'string' as an OFPT_FLOW_MOD or NXT_FLOW_MOD with command
> 'command'
> @@ -899,7 +919,8 @@ parse_ofp_str(struct flow_mod *fm, struct ofpbuf
> *actions, char *string)
> * flow. */
> void
> parse_ofp_flow_mod_str(struct list *packets, enum nx_flow_format
> *cur_format,
> - bool *flow_mod_table_id, char *string, uint16_t
> command)
> + bool *flow_mod_table_id, char *string, uint16_t
> command,
> + bool verbose)
> {
> bool is_del = command == OFPFC_DELETE || command ==
> OFPFC_DELETE_STRICT;
> enum nx_flow_format min_format, next_format;
> @@ -909,7 +930,7 @@ parse_ofp_flow_mod_str(struct list *packets, enum
> nx_flow_format *cur_format,
> struct flow_mod fm;
>
> ofpbuf_init(&actions, 64);
> - parse_ofp_str(&fm, is_del ? NULL : &actions, string);
> + parse_ofp_str(&fm, is_del ? NULL : &actions, string, verbose);
> fm.command = command;
>
> min_format = ofputil_min_flow_format(&fm.cr);
> @@ -953,7 +974,7 @@ parse_ofp_flow_mod_file(struct list *packets,
> ok = ds_get_preprocessed_line(&s, stream) == 0;
> if (ok) {
> parse_ofp_flow_mod_str(packets, cur, flow_mod_table_id,
> - ds_cstr(&s), command);
> + ds_cstr(&s), command, true);
> }
> ds_destroy(&s);
>
> @@ -966,7 +987,7 @@ parse_ofp_flow_stats_request_str(struct
> flow_stats_request *fsr,
> {
> struct flow_mod fm;
>
> - parse_ofp_str(&fm, NULL, string);
> + parse_ofp_str(&fm, NULL, string, false);
> fsr->aggregate = aggregate;
> fsr->match = fm.cr;
> fsr->out_port = fm.out_port;
> diff --git a/lib/ofp-parse.h b/lib/ofp-parse.h
> index 8209298..d55159a 100644
> --- a/lib/ofp-parse.h
> +++ b/lib/ofp-parse.h
> @@ -29,11 +29,12 @@ struct flow_stats_request;
> struct list;
> struct ofpbuf;
>
> -void parse_ofp_str(struct flow_mod *, struct ofpbuf *actions, char
> *string);
> +void parse_ofp_str(struct flow_mod *, struct ofpbuf *actions, const char
> *str_,
> + bool verbose);
>
> void parse_ofp_flow_mod_str(struct list *packets,
> enum nx_flow_format *cur, bool
> *flow_mod_table_id,
> - char *string, uint16_t command);
> + char *string, uint16_t command, bool verbose);
> bool parse_ofp_flow_mod_file(struct list *packets,
> enum nx_flow_format *cur, bool
> *flow_mod_table_id,
> FILE *, uint16_t command);
> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index 6c7e3ac..7d3b7fb 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -658,7 +658,7 @@ do_flow_mod__(int argc, char *argv[], uint16_t command)
> flow_mod_table_id = false;
>
> parse_ofp_flow_mod_str(&requests, &flow_format, &flow_mod_table_id,
> - argc > 2 ? argv[2] : "", command);
> + argc > 2 ? argv[2] : "", command, false);
> check_final_format_for_flow_mod(flow_format);
>
> open_vconn(argv[1], &vconn);
> @@ -1035,7 +1035,7 @@ read_flows_from_file(const char *filename, struct
> classifier *cls, int index)
> struct flow_mod fm;
>
> ofpbuf_init(&actions, 64);
> - parse_ofp_str(&fm, &actions, ds_cstr(&s));
> + parse_ofp_str(&fm, &actions, ds_cstr(&s), true);
>
> version = xmalloc(sizeof *version);
> version->cookie = fm.cookie;
> @@ -1308,7 +1308,7 @@ do_parse_flow(int argc OVS_UNUSED, char *argv[])
>
> list_init(&packets);
> parse_ofp_flow_mod_str(&packets, &flow_format, &flow_mod_table_id,
> - argv[1], OFPFC_ADD);
> + argv[1], OFPFC_ADD, false);
> print_packet_list(&packets);
> }
>
>
>
-Reid
>
> _______________________________________________
> dev mailing list
> [email protected]
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
