On Thu, Feb 28, 2013 at 07:36:47PM -0800, Andy Zhou wrote:
> When error is encountered with add-flows command, this patch
> adds file name and line number in addition to the parser
> error message to the output.
>
> The file name and line number will not be added to
> the output of "ovs-ofctl add-flow", only parser error
> message will appear. Same as before.
>
> Signed-off-by: Andy Zhou <[email protected]>
This is a big step in the right direction. Thank you.
The comment on ds_preprocess_line() is wrong (the function doesn't
clear anything initially in 'ds').
The return value convention for ds_preprocess_line() is unusual for
this code base. Our most common return value conventions are either a
"bool" indicating success or failure, or an "int" with 0 for success
or a positive errno for failure. I guess a "bool" might be best here?
I see a few "char*" etc. in this patch, that should be written "char
*".
This patch changes several function to return NULL for success,
otherwise a "char *" indicating an error. That makes sense, but I
have two suggestions. First, I think that these functions should have
a comment explaining the convention, because it is not a widespread
convention in the code base. Second, I suggest tagging these
functions with WARN_UNUSED_RESULT (see lib/compiler.h), because it is
easy to accidentally forget to check the return value in a caller and
this attribute helps to avoid that mistake.
I think that this change is somewhat incomplete. For example,
parse_named_action() calls parse_output(), which calls
mf_parse_subfield(), which aborts if there is a parse error.
str_to_ofpacts() has a typo:
}else {
should be
} else {
In multiple places, this should not be written on a single line (and
coding style requires {}):
if (err_str) return err_str;
In parse_ofp_str(), I see two potential memory leaks on error paths.
First, the 'string' allocated at the top of the function. Second, the
'ofpacts' buffer allocated near the bottom of the function.
+ if (err_str) {
+ fprintf(stderr, "OFCTL-ERR [%s:%d] ", file_name, lineno);
+ parser_fatal_exit_with_reason(err_str);
+ }
I think I'd rather just use "%s:%d: " as the lead-in. I don't think
OFCTL-ERR adds much, and editors that know how to parse error messages
already understand "%s:%d ".
We usually write comments as complete sentences that end with periods
(or with exclamation points if they are very exciting comments):
+/* Display the err_str to stderr, then exit
+ *
+ * This function will free the memory allocated for err_str
+ */
Please write the return type on a separate line:
+void parser_fatal_exit_with_reason(char *err_str)
+{
+ fprintf(stderr, "%s\n", err_str);
+ free(err_str);
I don't think we use -1 as an exit status right now, EXIT_FAILURE (1)
is more conventional:
+ exit(-1);
+}
Thanks,
Ben.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
