Re: [ovs-dev] [PATCH] ovsdb-server: Forbid user-specified databases with reserved names.

[Justin Pettit]

> On Dec 22, 2017, at 12:49 PM, Ben Pfaff  wrote:
> 
> On Thu, Dec 21, 2017 at 04:18:05PM -0800, Justin Pettit wrote:
>> 
>> 
>>> On Dec 8, 2017, at 12:53 PM, Ben Pfaff  wrote:
>>> 
>>> +if (argc > 2 && argv[1][0] == '_') {
>>> +unixctl_command_reply_error(conn, "cannot compact built-in 
>>> databases");
>>> +return;
>>> +}
>> 
>> This error condition seems a little odd to me.  I think it will only provide 
>> this warning if multiple databases are specified, and only complain if the 
>> first one is built-in.
> 
> You're right, there's a bug here, and I didn't test it.  Shame on me.
> 
> (However, the command supports at most one named database on the command
> line, which is part of the reason the test is somewhat odd.)

Oh, yeah, I see that now when I look at the command registration.  I was thrown 
off since I think the man page indicates multiple are supported:

   ovsdb-server/compact [db]...
  Compacts each database db in-place.  If no db is specified, com‐
  pacts every database in-place. 

I sent out a patch to update the man page.

Thanks for the incremental patch; it looks good to me.

--Justin


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovsdb-server: Forbid user-specified databases with reserved names.

[Ben Pfaff]

On Thu, Dec 21, 2017 at 04:18:05PM -0800, Justin Pettit wrote:
> 
> 
> > On Dec 8, 2017, at 12:53 PM, Ben Pfaff  wrote:
> > 
> > +if (argc > 2 && argv[1][0] == '_') {
> > +unixctl_command_reply_error(conn, "cannot compact built-in 
> > databases");
> > +return;
> > +}
> 
> This error condition seems a little odd to me.  I think it will only provide 
> this warning if multiple databases are specified, and only complain if the 
> first one is built-in.

You're right, there's a bug here, and I didn't test it.  Shame on me.

(However, the command supports at most one named database on the command
line, which is part of the reason the test is somewhat odd.)

> Acked-by: Justin Pettit 

I fixed the bug and added a test by folding in the following, and
applied this to master.   Thank you for the review and pointing out the
bug.

--8<--cut here-->8--

diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
index ff2ada76df4b..7f2d19ef568b 100644
--- a/ovsdb/ovsdb-server.c
+++ b/ovsdb/ovsdb-server.c
@@ -1136,13 +1136,14 @@ static void
 ovsdb_server_compact(struct unixctl_conn *conn, int argc,
  const char *argv[], void *dbs_)
 {
+const char *db_name = argc < 2 ? NULL : argv[1];
 struct shash *all_dbs = dbs_;
 struct ds reply;
 struct db *db;
 struct shash_node *node;
 int n = 0;
 
-if (argc > 2 && argv[1][0] == '_') {
+if (db_name && db_name[0] == '_') {
 unixctl_command_reply_error(conn, "cannot compact built-in databases");
 return;
 }
@@ -1150,9 +1151,9 @@ ovsdb_server_compact(struct unixctl_conn *conn, int argc,
 ds_init();
 SHASH_FOR_EACH(node, all_dbs) {
 db = node->data;
-if (argc < 2
-? node->name[0] != '_'
-: !strcmp(argv[1], node->name)) {
+if (db_name
+? !strcmp(node->name, db_name)
+: node->name[0] != '_') {
 struct ovsdb_error *error;
 
 VLOG_INFO("compacting %s database by user request", node->name);
diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at
index b830be0c1f4d..968356781604 100644
--- a/tests/ovsdb-server.at
+++ b/tests/ovsdb-server.at
@@ -689,6 +689,11 @@ _uuidname  number
 dnl Now compact the database in-place.
 AT_CHECK([[ovs-appctl -t ovsdb-server ovsdb-server/compact]],
   [0], [], [ignore], [test ! -e pid || kill `cat pid`])
+dnl Negative test.
+AT_CHECK([[ovs-appctl -t ovsdb-server ovsdb-server/compact _Server]],
+  [2], [], [cannot compact built-in databases
+ovs-appctl: ovsdb-server: server returned an error
+])
 dnl Make sure that "db" is still a symlink to dir/db instead of getting
 dnl replaced by a regular file, ditto for .db.~lock~.
 if test "$IS_WIN32" = "no"; then
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovsdb-server: Forbid user-specified databases with reserved names.

[Justin Pettit]

> On Dec 8, 2017, at 12:53 PM, Ben Pfaff  wrote:
> 
> +if (argc > 2 && argv[1][0] == '_') {
> +unixctl_command_reply_error(conn, "cannot compact built-in 
> databases");
> +return;
> +}

This error condition seems a little odd to me.  I think it will only provide 
this warning if multiple databases are specified, and only complain if the 
first one is built-in.

Acked-by: Justin Pettit 

--Justin


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] ovsdb-server: Forbid user-specified databases with reserved names.

[Ben Pfaff]

Names that begin with "_" are reserved, but ovsdb-server didn't previously
enforce this.

At the same time, make ovsdb-client ignore databases with reserved names
for the purpose of selecting a default database to work on.

This is in preparation for ovsdb-server starting to serve a new database,
full of meta-information, called _Server.

Signed-off-by: Ben Pfaff 
---
 ovsdb/execution.c| 17 -
 ovsdb/ovsdb-client.c | 19 ++-
 ovsdb/ovsdb-server.c | 51 ++-
 3 files changed, 64 insertions(+), 23 deletions(-)

diff --git a/ovsdb/execution.c b/ovsdb/execution.c
index 016b9c68767c..1794fcf66e43 100644
--- a/ovsdb/execution.c
+++ b/ovsdb/execution.c
@@ -175,11 +175,18 @@ ovsdb_execute(struct ovsdb *db, const struct 
ovsdb_session *session,
 error = parse_error;
 }
 /* Create read-only violation error if there is one. */
-if (!error && read_only && !ro) {
-error = ovsdb_error("not allowed",
-"%s operation not allowed when "
-"database server is in read only mode",
-op_name);
+if (!ro && !error) {
+if (read_only) {
+error = ovsdb_error("not allowed",
+"%s operation not allowed when "
+"database server is in read only mode",
+op_name);
+} else if (db->schema->name[0] == '_') {
+error = ovsdb_error("not allowed",
+"%s operation not allowed on "
+"table in reserved database %s",
+op_name, db->schema->name);
+}
 }
 if (error) {
 json_destroy(result);
diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c
index 7d90183843ee..e4f2ce2ba0b5 100644
--- a/ovsdb/ovsdb-client.c
+++ b/ovsdb/ovsdb-client.c
@@ -131,14 +131,23 @@ main(int argc, char *argv[])
 if (argc - optind > command->min_args
 && svec_contains(, argv[optind])) {
 database = xstrdup(argv[optind++]);
-} else if (dbs.n == 1) {
-database = xstrdup(dbs.names[0]);
 } else if (svec_contains(, "Open_vSwitch")) {
 database = xstrdup("Open_vSwitch");
 } else {
-jsonrpc_close(rpc);
-ovs_fatal(0, "no default database for `%s' command, please "
-  "specify a database name", command->name);
+size_t n = 0;
+const char *best = NULL;
+for (size_t i = 0; i < dbs.n; i++) {
+if (dbs.names[i][0] != '_') {
+best = dbs.names[i];
+n++;
+}
+}
+if (n != 1) {
+jsonrpc_close(rpc);
+ovs_fatal(0, "no default database for `%s' command, please "
+  "specify a database name", command->name);
+}
+database = xstrdup(best);
 }
 svec_destroy();
 } else {
diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
index 1e0c20486e38..19cea99d3741 100644
--- a/ovsdb/ovsdb-server.c
+++ b/ovsdb/ovsdb-server.c
@@ -146,7 +146,9 @@ ovsdb_replication_init(const char *sync_from, const char 
*exclude,
 struct shash_node *node;
 SHASH_FOR_EACH (node, all_dbs) {
 struct db *db = node->data;
-replication_add_local_db(node->name, db->db);
+if (node->name[0] != '_' && db->db) {
+replication_add_local_db(node->name, db->db);
+}
 }
 }
 
@@ -518,6 +520,9 @@ open_db(struct server_config *config, const char *filename)
 db_error = ovsdb_file_open(db->filename, false, >db, >file);
 if (db_error) {
 error = ovsdb_error_to_string(db_error);
+} else if (db->db->schema->name[0] == '_') {
+error = xasprintf("%s: names beginning with \"_\" are reserved",
+  db->db->schema->name);
 } else if (!ovsdb_jsonrpc_server_add_db(config->jsonrpc, db->db)) {
 error = xasprintf("%s: duplicate database name", db->db->schema->name);
 } else {
@@ -1147,10 +1152,17 @@ ovsdb_server_compact(struct unixctl_conn *conn, int 
argc,
 struct shash_node *node;
 int n = 0;
 
+if (argc > 2 && argv[1][0] == '_') {
+unixctl_command_reply_error(conn, "cannot compact built-in databases");
+return;
+}
+
 ds_init();
 SHASH_FOR_EACH(node, all_dbs) {
 db = node->data;
-if (argc < 2 || !strcmp(argv[1], node->name)) {
+if (argc < 2
+? node->name[0] != '_'
+: !strcmp(argv[1], node->name)) {
 struct ovsdb_error *error;
 
 VLOG_INFO("compacting %s database by user request", node->name);
@@ -1286,21 +1298,12 @@