Looks good to me. Acked-by: Kyle Mestery <[email protected]>
On Aug 29, 2012, at 7:09 PM, Ben Pfaff wrote: > OVSDB has always had the ability to mark a column as "immutable", so that > its value cannot be changed in a given row after that row is initially > inserted. However, we discovered recently that ovsdb-server has never > enforced this constraint. This commit implements enforcement. > > Reported-by: Paul Ingram <[email protected]> > Signed-off-by: Ben Pfaff <[email protected]> > --- > NEWS | 2 ++ > lib/ovsdb-idl-provider.h | 3 ++- > ovsdb/execution.c | 18 +++++++++++++++++- > ovsdb/mutation.c | 8 +++++++- > ovsdb/ovsdb-idlc.in | 5 +++++ > tests/ovs-vsctl.at | 12 ++++++++++++ > tests/ovsdb-execution.at | 43 +++++++++++++++++++++++++++++++++++++++++++ > tests/ovsdb-mutation.at | 12 ++++++++++++ > utilities/ovs-vsctl.c | 22 ++++++++++++++++++++-- > 9 files changed, 120 insertions(+), 5 deletions(-) > > diff --git a/NEWS b/NEWS > index 872f8d0d..cbc5c58 100644 > --- a/NEWS > +++ b/NEWS > @@ -18,6 +18,8 @@ post-v1.8.0 > are true, but because we do not know of any users for this > feature it seems better on balance to remove it. (The ovs-pki-cgi > program was not included in distribution packaging.) > + - ovsdb-server now enforces the immutability of immutable columns. This > + was not enforced in earlier versions due to an oversight. > - Stable bond mode is deprecated and will be removed no earlier than > February 2013. Please email [email protected] with concerns. > - The autopath action is deprecated and will be removed no earlier than > diff --git a/lib/ovsdb-idl-provider.h b/lib/ovsdb-idl-provider.h > index 546acbb..7ea5ce6 100644 > --- a/lib/ovsdb-idl-provider.h > +++ b/lib/ovsdb-idl-provider.h > @@ -1,4 +1,4 @@ > -/* Copyright (c) 2009, 2010, 2011 Nicira, Inc. > +/* Copyright (c) 2009, 2010, 2011, 2012 Nicira, Inc. > * > * Licensed under the Apache License, Version 2.0 (the "License"); > * you may not use this file except in compliance with the License. > @@ -41,6 +41,7 @@ struct ovsdb_idl_row { > struct ovsdb_idl_column { > char *name; > struct ovsdb_type type; > + bool mutable; > void (*parse)(struct ovsdb_idl_row *, const struct ovsdb_datum *); > void (*unparse)(struct ovsdb_idl_row *); > }; > diff --git a/ovsdb/execution.c b/ovsdb/execution.c > index 1aff0c5..300c247 100644 > --- a/ovsdb/execution.c > +++ b/ovsdb/execution.c > @@ -1,4 +1,4 @@ > -/* Copyright (c) 2009, 2010, 2011 Nicira, Inc. > +/* Copyright (c) 2009, 2010, 2011, 2012 Nicira, Inc. > * > * Licensed under the Apache License, Version 2.0 (the "License"); > * you may not use this file except in compliance with the License. > @@ -435,6 +435,22 @@ ovsdb_execute_update(struct ovsdb_execution *x, struct > ovsdb_parser *parser, > error = parse_row(row_json, table, x->symtab, &row, &columns); > } > if (!error) { > + size_t i; > + > + for (i = 0; i < columns.n_columns; i++) { > + const struct ovsdb_column *column = columns.columns[i]; > + > + if (!column->mutable) { > + error = ovsdb_syntax_error(parser->json, > + "constraint violation", > + "Cannot update immutable column > %s " > + "in table %s.", > + column->name, > table->schema->name); > + break; > + } > + } > + } > + if (!error) { > error = ovsdb_condition_from_json(table->schema, where, x->symtab, > &condition); > } > diff --git a/ovsdb/mutation.c b/ovsdb/mutation.c > index 0dcd16f..5fd983a 100644 > --- a/ovsdb/mutation.c > +++ b/ovsdb/mutation.c > @@ -1,4 +1,4 @@ > -/* Copyright (c) 2009, 2010, 2011 Nicira, Inc. > +/* Copyright (c) 2009, 2010, 2011, 2012 Nicira, Inc. > * > * Licensed under the Apache License, Version 2.0 (the "License"); > * you may not use this file except in compliance with the License. > @@ -95,6 +95,12 @@ ovsdb_mutation_from_json(const struct ovsdb_table_schema > *ts, > "No column %s in table %s.", > column_name, ts->name); > } > + if (!m->column->mutable) { > + return ovsdb_syntax_error(json, "constraint violation", > + "Cannot mutate immutable column %s in " > + "table %s.", column_name, ts->name); > + } > + > ovsdb_type_clone(&m->type, &m->column->type); > > mutator_name = json_string(array->elems[1]); > diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in > index 0b1933b..478109a 100755 > --- a/ovsdb/ovsdb-idlc.in > +++ b/ovsdb/ovsdb-idlc.in > @@ -577,11 +577,16 @@ static void\n%s_columns_init(void) > for columnName, column in sorted(table.columns.iteritems()): > cs = "%s_col_%s" % (structName, columnName) > d = {'cs': cs, 'c': columnName, 's': structName} > + if column.mutable: > + mutable = "true" > + else: > + mutable = "false" > print > print " /* Initialize %(cs)s. */" % d > print " c = &%(cs)s;" % d > print " c->name = \"%(c)s\";" % d > print column.type.cInitType(" ", "c->type") > + print " c->mutable = %s;" % mutable > print " c->parse = %(s)s_parse_%(c)s;" % d > print " c->unparse = %(s)s_unparse_%(c)s;" % d > print "}" > diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at > index 84cc272..e9036191 100644 > --- a/tests/ovs-vsctl.at > +++ b/tests/ovs-vsctl.at > @@ -736,6 +736,18 @@ AT_CHECK([RUN_OVS_VSCTL([clear netflow `cat > netflow-uuid` targets])], > AT_CHECK([RUN_OVS_VSCTL([destroy b br2])], > [1], [], [ovs-vsctl: no row "br2" in table Bridge > ], [OVS_VSCTL_CLEANUP]) > +AT_CHECK([RUN_OVS_VSCTL([add i br1 name x])], > + [1], [], [ovs-vsctl: cannot modify read-only column name in table Interface > +], [OVS_VSCTL_CLEANUP]) > +AT_CHECK([RUN_OVS_VSCTL([set port br1 name br2])], > + [1], [], [ovs-vsctl: cannot modify read-only column name in table Port > +], [OVS_VSCTL_CLEANUP]) > +AT_CHECK([RUN_OVS_VSCTL([remove b br1 name br1])], > + [1], [], [ovs-vsctl: cannot modify read-only column name in table Bridge > +], [OVS_VSCTL_CLEANUP]) > +AT_CHECK([RUN_OVS_VSCTL([clear b br1 name])], > + [1], [], [ovs-vsctl: cannot modify read-only column name in table Bridge > +], [OVS_VSCTL_CLEANUP]) > OVS_VSCTL_CLEANUP > AT_CLEANUP > > diff --git a/tests/ovsdb-execution.at b/tests/ovsdb-execution.at > index 480f610..6a3b5d1 100644 > --- a/tests/ovsdb-execution.at > +++ b/tests/ovsdb-execution.at > @@ -111,6 +111,15 @@ gc_schema () { > "isRoot": false}}} > EOF > } > + > +immutable_schema () { > + cat <<'EOF' > +{"name": "immutable", > + "tables": { > + "a": { > + "columns": {"i": {"type": "integer", "mutable": false}}}}} > +EOF > +} > ] > m4_divert_pop([PREPARE_TESTS]) > > @@ -908,6 +917,40 @@ OVSDB_CHECK_EXECUTION([weak references], > [{"rows":[{"_uuid":["uuid","<3>"],"b":2,"b2a":["set",[]]},{"_uuid":["uuid","<4>"],"b":3,"b2a":["set",[]]}]}] > ]]) > > +OVSDB_CHECK_EXECUTION([immutable columns], > + [immutable_schema], > + [[[["immutable", > + {"op": "insert", > + "table": "a", > + "row": {"i": 5}, > + "uuid-name": "row1"}]]], > + [[["immutable", > + {"op": "update", > + "table": "a", > + "row": {"i": 10}, > + "where": []}]]], > + [[["immutable", > + {"op": "update", > + "table": "a", > + "row": {"i": 5}, > + "where": []}]]], > + [[["immutable", > + {"op": "mutate", > + "table": "a", > + "where": [], > + "mutations": [["i", "-=", 5]]}]]], > + [[["immutable", > + {"op": "mutate", > + "table": "a", > + "where": [], > + "mutations": [["i", "*=", 1]]}]]]], > + [[[{"uuid":["uuid","<0>"]}] > +[{"details":"Cannot update immutable column i in table > a.","error":"constraint > violation","syntax":"{\"op\":\"update\",\"row\":{\"i\":10},\"table\":\"a\",\"where\":[]}"}] > +[{"details":"Cannot update immutable column i in table > a.","error":"constraint > violation","syntax":"{\"op\":\"update\",\"row\":{\"i\":5},\"table\":\"a\",\"where\":[]}"}] > +[{"details":"Cannot mutate immutable column i in table > a.","error":"constraint violation","syntax":"[\"i\",\"-=\",5]"}] > +[{"details":"Cannot mutate immutable column i in table > a.","error":"constraint violation","syntax":"[\"i\",\"*=\",1]"}] > +]]) > + > OVSDB_CHECK_EXECUTION([garbage collection], > [gc_schema], > [dnl Check that inserting a row without any references is a no-op. > diff --git a/tests/ovsdb-mutation.at b/tests/ovsdb-mutation.at > index 3d75317..fc898b5 100644 > --- a/tests/ovsdb-mutation.at > +++ b/tests/ovsdb-mutation.at > @@ -99,6 +99,18 @@ test-ovsdb: syntax > "["u","delete",["uuid","9179ca6d-6d65-400a-b455-3ad92783a099" > ]]) > AT_CLEANUP > > +AT_SETUP([disallowed mutations on immutable columns]) > +AT_KEYWORDS([ovsdb negative mutation]) > +AT_CHECK([[test-ovsdb parse-mutations \ > + '{"columns": > + {"i": {"type": "integer", "mutable": false}}}' \ > + '[["i", "+=", 1]]' > +]], > + [1], [], > + [[test-ovsdb: syntax "["i","+=",1]": constraint violation: Cannot mutate > immutable column i in table mytable. > +]]) > +AT_CLEANUP > + > OVSDB_CHECK_POSITIVE([mutations on sets], > [[parse-mutations \ > '{"columns": > diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c > index 0a7c2c6..e6bd63b 100644 > --- a/utilities/ovs-vsctl.c > +++ b/utilities/ovs-vsctl.c > @@ -2763,7 +2763,7 @@ error: > return error; > } > > -static void > +static const struct ovsdb_idl_column * > pre_parse_column_key_value(struct vsctl_context *ctx, > const char *arg, > const struct vsctl_table_class *table) > @@ -2780,6 +2780,18 @@ pre_parse_column_key_value(struct vsctl_context *ctx, > > pre_get_column(ctx, table, column_name, &column); > free(column_name); > + > + return column; > +} > + > +static void > +check_mutable(const struct vsctl_table_class *table, > + const struct ovsdb_idl_column *column) > +{ > + if (!column->mutable) { > + vsctl_fatal("cannot modify read-only column %s in table %s", > + column->name, table->class->name); > + } > } > > static void > @@ -3112,7 +3124,10 @@ pre_cmd_set(struct vsctl_context *ctx) > > table = pre_get_table(ctx, table_name); > for (i = 3; i < ctx->argc; i++) { > - pre_parse_column_key_value(ctx, ctx->argv[i], table); > + const struct ovsdb_idl_column *column; > + > + column = pre_parse_column_key_value(ctx, ctx->argv[i], table); > + check_mutable(table, column); > } > } > > @@ -3195,6 +3210,7 @@ pre_cmd_add(struct vsctl_context *ctx) > > table = pre_get_table(ctx, table_name); > pre_get_column(ctx, table, column_name, &column); > + check_mutable(table, column); > } > > static void > @@ -3251,6 +3267,7 @@ pre_cmd_remove(struct vsctl_context *ctx) > > table = pre_get_table(ctx, table_name); > pre_get_column(ctx, table, column_name, &column); > + check_mutable(table, column); > } > > static void > @@ -3316,6 +3333,7 @@ pre_cmd_clear(struct vsctl_context *ctx) > const struct ovsdb_idl_column *column; > > pre_get_column(ctx, table, ctx->argv[i], &column); > + check_mutable(table, column); > } > } > > -- > 1.7.2.5 > > _______________________________________________ > dev mailing list > [email protected] > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
