On Thu, Dec 21, 2017 at 04:18:05PM -0800, Justin Pettit wrote: > > > > On Dec 8, 2017, at 12:53 PM, Ben Pfaff <[email protected]> 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 <[email protected]> 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(&reply); 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 @@ _uuid name 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
