On Mon, Apr 06, 2015 at 02:11:24PM -0700, Andy Zhou wrote:
> I'm determined not to let the terrible style of pseudo-parsing we have in
> OVS leak into OVN. Here's the first step.
>
> Signed-off-by: Ben Pfaff <[email protected]>
>
> Looks good in general. A few minor comments in line:
> Acked-by: Andy Zhou <[email protected]>
Thanks.
> > +static void
> > +lex_token_format_masked_integer(const struct lex_token *token, struct ds
> > *s)
> > +{
> > + enum lex_format format = lex_token_get_format(token);
> > +
> > + lex_token_format_value(&token->value, format, s);
> > + ds_put_char(s, '/');
> > +
> > + const union mf_subvalue *mask = &token->mask;
> > + if (format == LEX_F_IPV4 && ip_is_cidr(mask->ipv4)) {
> > + ds_put_format(s, "%d", ip_count_cidr_bits(mask->ipv4));
> > + } else if (token->format == LEX_F_IPV6 && ipv6_is_cidr(&mask->ipv6)) {
> > + ds_put_format(s, "%d", ipv6_count_cidr_bits(&mask->ipv6));
> > + } else {
> > + lex_token_format_value(&token->mask, format, s);
> > + }
> > +}
> > +
> > +
> There is an extra blank line above.
Thanks, removed.
> > +static const char *
> > +lex_parse_integer(const char *p, struct lex_token *token)
> > +{
> > + memset(&token->value, 0, sizeof token->value);
> > + p = lex_parse_integer__(p, token);
> > + if (token->type == LEX_T_INTEGER && *p == '/') {
> > + struct lex_token mask;
> > +
> > + lex_token_init(&mask);
> > + memset(&mask.value, 0, sizeof mask.value);
> > + p = lex_parse_integer__(p + 1, &mask);
> > + if (mask.type == LEX_T_INTEGER) {
> > + token->type = LEX_T_MASKED_INTEGER;
> > +
> > + uint32_t prefix_bits = ntohll(mask.value.integer);
> > + if (token->format == mask.format) {
> > + /* Same format value and mask is always OK. */
> > + token->mask = mask.value;
> > + } else if (token->format == LEX_F_IPV4
> > + && mask.format == LEX_F_DECIMAL
> > + && prefix_bits <= 32) {
> > + /* IPv4 address with decimal mask is a CIDR prefix. */
> > + token->mask.integer = htonll(ntohl(be32_prefix_mask(
> > + prefix_bits)));
> > + } else if (token->format == LEX_F_IPV6
> > + && mask.format == LEX_F_DECIMAL
> > + && prefix_bits <= 128) {
> > + /* IPv6 address with decimal mask is a CIDR prefix. */
> > + token->mask.ipv6 = ipv6_create_mask(prefix_bits);
> > + } else if (token->format == LEX_F_DECIMAL
> > + && mask.format == LEX_F_HEXADECIMAL
> > + && token->value.integer == 0) {
> > + /* Special case for e.g. 0/0x1234. */
> > + token->format = LEX_F_HEXADECIMAL;
> > + token->mask = mask.value;
> > + } else {
> > + lex_error(token, "Value and mask have incompatible
> > formats.");
> > + return p;
> > + }
> > +
> > + for (int i = 0; i < ARRAY_SIZE(token->mask.be32); i++) {
> > + ovs_be32 v = token->value.be32[i];
> > + ovs_be32 m = token->mask.be32[i];
> > +
> > + if (v & ~m) {
> > + lex_error(token, "Value contains unmasked 1-bits.");
> > + break;
> > + }
> > + }
> > +
> > + return p;
> > + } else {
> > + lex_token_swap(&mask, token);
>
> Which case was this 'else' for? It would be nice to add a comment here.
Thanks, I added a comment and assertino:
@@ -392,6 +391,10 @@ lex_parse_integer(const char *p, struct lex_token *token)
return p;
} else {
+ /* 'mask' isn't an integer so it must be an error message. Swap
+ * the integer for the error message so that we return the error
+ * message (and destroy the pre-mask token just below). */
+ ovs_assert(token->type == LEX_T_ERROR);
lex_token_swap(&mask, token);
}
lex_token_destroy(&mask);
> > + }
> > + lex_token_destroy(&mask);
> > + }
> > + return p;
> > +}
> > +
> > +static const char *
> > +lex_parse_string(const char *p, struct lex_token *token)
> > +{
> > + const char *start = ++p;
> > + for (;;) {
> > + switch (*p) {
> > + case '\0':
> > + lex_error(token, "Input ends inside quoted string.");
> > + return p;
> > +
> > + case '"':
> > + token->type = (json_string_unescape(start, p - start,
> > &token->s)
> > + ? LEX_T_STRING : LEX_T_ERROR);
> > + return p + 1;
> > +
> > + case '\\':
> > + p++;
> > + if (*p) {
> > + p++;
> > + }
> > + break;
> > +
> > + default:
> > + p++;
> > + break;
> > + }
> > + }
> > +
> extra line above or intentional?
> > +}
Extra. Removed, thanks.
> > +/* Initializes 'token' and parses the first token from the beginning of
> > + * null-terminated string 'p' into 'token'. Stores a pointer to the start
> > of
> > + * the token (after skipping white space and comments, if any) into
> > '*startp'.
> > + * Returns the character position at which to begin parsing the next
> > token. */
> > +const char *
> > +lex_token_parse(struct lex_token *token, const char *p, const char
> > **startp)
> > +{
> > + lex_token_init(token);
> > +
> > +next:
> > + *startp = p;
> > + switch (*p) {
> > + case '\0':
> > + token->type = LEX_T_END;
> > + return p;
> > +
> > + case ' ': case '\t': case '\n': case '\r':
> > + p++;
> > + goto next;
> > +
> > + case '/':
> > + p++;
> > + if (*p == '/') {
> > + do {
> > + p++;
> > + } while (*p != '\0' && *p != '\n');
> > + goto next;
> > + } else if (*p == '*') {
> > + p++;
> > + for (;;) {
> > + if (*p == '*' && p[1] == '/') {
> > + p += 2;
> > + goto next;
> > + } else if (*p == '\0' || *p == '\n') {
> > + lex_error(token, "`/*' without matching `*/'.");
> > + return p;
> It seems "/* and */" can not be used for multi line comment. May be
> worth a comment to point this out.
True. (I wanted it to be possible to tokenize a line at a time
without dependencies between lines.) I added some documentation:
diff --git a/ovn/ovn.xml b/ovn/ovn.xml
index 0906272..62e93ed 100644
--- a/ovn/ovn.xml
+++ b/ovn/ovn.xml
@@ -375,6 +375,13 @@
</p>
<p>
+ <em>Comments</em> may be introduced by <code>//</code>, which extends
+ to the next new-line. Comments within a line may be bracketed by
+ <code>/*</code> and <code>*/</code>. Multiline comments are not
+ supported.
+ </p>
+
+ <p>
(The above is pretty ambitious. It probably makes sense to initially
implement only a subset of this specification. The full specification
is written out mainly to get an idea of what a fully general matching
It sounds like Russell or Justin would like to review this series
also, so for now I'll just update my "ovn" branch in my github
"reviews" repository.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
