Add read-only option to ovs-dpctl and ovs-ofctl commands.

ovs-dpctl and ovs-ofctl lack a read-only option to prevent running of commands that perform read-write operations. Add it and the necessary scaffolding to each. Signed-off-by: Ryan Moats <[email protected]> Signed-off-by: Ben Pfaff <[email protected]>
evelikov · Aug 16, 2016 · 1f4a725 · 1f4a725
1 parent 239fa5b
commit 1f4a725
Show file tree

Hide file tree

Showing 23 changed files with 321 additions and 263 deletions.
diff --git a/lib/command-line.c b/lib/command-line.c
@@ -87,20 +87,10 @@ ovs_cmdl_print_options(const struct option options[])
     ds_destroy(&ds);
 }
 
-/* Runs the command designated by argv[0] within the command table specified by
- * 'commands', which must be terminated by a command whose 'name' member is a
- * null pointer.
- *
- * Command-line options should be stripped off, so that a typical invocation
- * looks like:
- *    struct ovs_cmdl_context ctx = {
- *        .argc = argc - optind,
- *        .argv = argv + optind,
- *    };
- *    ovs_cmdl_run_command(&ctx, my_commands);
- * */
-void
-ovs_cmdl_run_command(struct ovs_cmdl_context *ctx, const struct ovs_cmdl_command commands[])
+static void
+ovs_cmdl_run_command__(struct ovs_cmdl_context *ctx,
+                       const struct ovs_cmdl_command commands[],
+                       bool read_only)
 {
     const struct ovs_cmdl_command *p;
 
@@ -118,6 +108,10 @@ ovs_cmdl_run_command(struct ovs_cmdl_context *ctx, const struct ovs_cmdl_command
                 VLOG_FATAL("'%s' command takes at most %d arguments",
                            p->name, p->max_args);
             } else {
+                if (p->mode == OVS_RW && read_only) {
+                    VLOG_FATAL("'%s' command does not work in read only mode",
+                               p->name);
+                }
                 p->handler(ctx);
                 if (ferror(stdout)) {
                     VLOG_FATAL("write to stdout failed");
@@ -132,6 +126,32 @@ ovs_cmdl_run_command(struct ovs_cmdl_context *ctx, const struct ovs_cmdl_command
 
     VLOG_FATAL("unknown command '%s'; use --help for help", ctx->argv[0]);
 }
+
+/* Runs the command designated by argv[0] within the command table specified by
+ * 'commands', which must be terminated by a command whose 'name' member is a
+ * null pointer.
+ *
+ * Command-line options should be stripped off, so that a typical invocation
+ * looks like:
+ *    struct ovs_cmdl_context ctx = {
+ *        .argc = argc - optind,
+ *        .argv = argv + optind,
+ *    };
+ *    ovs_cmdl_run_command(&ctx, my_commands);
+ * */
+void
+ovs_cmdl_run_command(struct ovs_cmdl_context *ctx,
+                     const struct ovs_cmdl_command commands[])
+{
+    ovs_cmdl_run_command__(ctx, commands, false);
+}
+
+void
+ovs_cmdl_run_command_read_only(struct ovs_cmdl_context *ctx,
+                               const struct ovs_cmdl_command commands[])
+{
+    ovs_cmdl_run_command__(ctx, commands, true);
+}
 
 /* Process title. */
 

diff --git a/lib/command-line.h b/lib/command-line.h
@@ -41,12 +41,16 @@ struct ovs_cmdl_command {
     int min_args;
     int max_args;
     ovs_cmdl_handler handler;
+    enum { OVS_RO, OVS_RW } mode;    /* Does this command modify things? */
 };
 
 char *ovs_cmdl_long_options_to_short_options(const struct option *options);
 void ovs_cmdl_print_options(const struct option *options);
 void ovs_cmdl_print_commands(const struct ovs_cmdl_command *commands);
-void ovs_cmdl_run_command(struct ovs_cmdl_context *, const struct ovs_cmdl_command[]);
+void ovs_cmdl_run_command(struct ovs_cmdl_context *,
+                          const struct ovs_cmdl_command[]);
+void ovs_cmdl_run_command_read_only(struct ovs_cmdl_context *,
+                                    const struct ovs_cmdl_command[]);
 
 void ovs_cmdl_proctitle_init(int argc, char **argv);
 #if defined(__FreeBSD__) || defined(__NetBSD__)

diff --git a/lib/db-ctl-base.h b/lib/db-ctl-base.h
@@ -120,7 +120,7 @@ struct ctl_command_syntax {
      * empty string if the command does not support any options. */
     const char *options;
 
-    enum { RO, RW } mode;       /* Does this command modify the database? */
+    enum { RO, RW } mode;   /* Does this command modify the database? */
 };
 
 /* A command extracted from command-line input plus the structs for

diff --git a/lib/dpctl.c b/lib/dpctl.c
@@ -59,6 +59,7 @@ struct dpctl_command {
     int min_args;
     int max_args;
     dpctl_command_handler *handler;
+    enum { DP_RO, DP_RW} mode;
 };
 static const struct dpctl_command *get_all_dpctl_commands(void);
 static void dpctl_print(struct dpctl_params *dpctl_p, const char *fmt, ...)
@@ -1615,29 +1616,29 @@ dpctl_normalize_actions(int argc, const char *argv[],
 }
 
 static const struct dpctl_command all_commands[] = {
-    { "add-dp", "add-dp dp [iface...]", 1, INT_MAX, dpctl_add_dp },
-    { "del-dp", "del-dp dp", 1, 1, dpctl_del_dp },
-    { "add-if", "add-if dp iface...", 2, INT_MAX, dpctl_add_if },
-    { "del-if", "del-if dp iface...", 2, INT_MAX, dpctl_del_if },
-    { "set-if", "set-if dp iface...", 2, INT_MAX, dpctl_set_if },
-    { "dump-dps", "", 0, 0, dpctl_dump_dps },
-    { "show", "[dp...]", 0, INT_MAX, dpctl_show },
-    { "dump-flows", "[dp]", 0, 2, dpctl_dump_flows },
-    { "add-flow", "add-flow [dp] flow actions", 2, 3, dpctl_add_flow },
-    { "mod-flow", "mod-flow [dp] flow actions", 2, 3, dpctl_mod_flow },
-    { "get-flow", "get-flow [dp] ufid", 1, 2, dpctl_get_flow },
-    { "del-flow", "del-flow [dp] flow", 1, 2, dpctl_del_flow },
-    { "del-flows", "[dp]", 0, 1, dpctl_del_flows },
-    { "dump-conntrack", "[dp] [zone=N]", 0, 2, dpctl_dump_conntrack },
-    { "flush-conntrack", "[dp] [zone=N]", 0, 2, dpctl_flush_conntrack },
-    { "help", "", 0, INT_MAX, dpctl_help },
-    { "list-commands", "", 0, INT_MAX, dpctl_list_commands },
+    { "add-dp", "add-dp dp [iface...]", 1, INT_MAX, dpctl_add_dp, DP_RW },
+    { "del-dp", "del-dp dp", 1, 1, dpctl_del_dp, DP_RW },
+    { "add-if", "add-if dp iface...", 2, INT_MAX, dpctl_add_if, DP_RW },
+    { "del-if", "del-if dp iface...", 2, INT_MAX, dpctl_del_if, DP_RW },
+    { "set-if", "set-if dp iface...", 2, INT_MAX, dpctl_set_if, DP_RW },
+    { "dump-dps", "", 0, 0, dpctl_dump_dps, DP_RO },
+    { "show", "[dp...]", 0, INT_MAX, dpctl_show, DP_RO },
+    { "dump-flows", "[dp]", 0, 2, dpctl_dump_flows, DP_RO },
+    { "add-flow", "add-flow [dp] flow actions", 2, 3, dpctl_add_flow, DP_RW },
+    { "mod-flow", "mod-flow [dp] flow actions", 2, 3, dpctl_mod_flow, DP_RW },
+    { "get-flow", "get-flow [dp] ufid", 1, 2, dpctl_get_flow, DP_RO },
+    { "del-flow", "del-flow [dp] flow", 1, 2, dpctl_del_flow, DP_RW },
+    { "del-flows", "[dp]", 0, 1, dpctl_del_flows, DP_RW },
+    { "dump-conntrack", "[dp] [zone=N]", 0, 2, dpctl_dump_conntrack, DP_RO },
+    { "flush-conntrack", "[dp] [zone=N]", 0, 2, dpctl_flush_conntrack, DP_RW },
+    { "help", "", 0, INT_MAX, dpctl_help, DP_RO },
+    { "list-commands", "", 0, INT_MAX, dpctl_list_commands, DP_RO },
 
     /* Undocumented commands for testing. */
-    { "parse-actions", "actions", 1, INT_MAX, dpctl_parse_actions },
-    { "normalize-actions", "actions", 2, INT_MAX, dpctl_normalize_actions },
+    { "parse-actions", "actions", 1, INT_MAX, dpctl_parse_actions, DP_RO },
+    { "normalize-actions", "actions", 2, INT_MAX, dpctl_normalize_actions, DP_RO },
 
-    { NULL, NULL, 0, 0, NULL },
+    { NULL, NULL, 0, 0, NULL, DP_RO },
 };
 
 static const struct dpctl_command *get_all_dpctl_commands(void)
@@ -1672,6 +1673,12 @@ dpctl_run_command(int argc, const char *argv[], struct dpctl_params *dpctl_p)
                             p->name, p->max_args);
                 return EINVAL;
             } else {
+                if (p->mode == DP_RW && dpctl_p->read_only) {
+                    dpctl_error(dpctl_p, 0,
+                                "'%s' command does not work in read only mode",
+                                p->name);
+                    return EINVAL;
+                }
                 return p->handler(argc, argv, dpctl_p);
             }
         }

diff --git a/lib/dpctl.h b/lib/dpctl.h
@@ -33,6 +33,9 @@ struct dpctl_params {
     /* --may-create: Allow mod-flows command to create a new flow? */
     bool may_create;
 
+    /* --read-only: Do not run R/W commands? */
+    bool read_only;
+
     /* -m, --more: Increase output verbosity. */
     int verbosity;
 

diff --git a/ovsdb/ovsdb-tool.c b/ovsdb/ovsdb-tool.c
@@ -573,20 +573,20 @@ do_list_commands(struct ovs_cmdl_context *ctx OVS_UNUSED)
 }
 
 static const struct ovs_cmdl_command all_commands[] = {
-    { "create", "[db [schema]]", 0, 2, do_create },
-    { "compact", "[db [dst]]", 0, 2, do_compact },
-    { "convert", "[db [schema [dst]]]", 0, 3, do_convert },
-    { "needs-conversion", NULL, 0, 2, do_needs_conversion },
-    { "db-version", "[db]",  0, 1, do_db_version },
-    { "db-cksum", "[db]", 0, 1, do_db_cksum },
-    { "schema-version", "[schema]", 0, 1, do_schema_version },
-    { "schema-cksum", "[schema]", 0, 1, do_schema_cksum },
-    { "query", "[db] trns", 1, 2, do_query },
-    { "transact", "[db] trns", 1, 2, do_transact },
-    { "show-log", "[db]", 0, 1, do_show_log },
-    { "help", NULL, 0, INT_MAX, do_help },
-    { "list-commands", NULL, 0, INT_MAX, do_list_commands },
-    { NULL, NULL, 0, 0, NULL },
+    { "create", "[db [schema]]", 0, 2, do_create, OVS_RW },
+    { "compact", "[db [dst]]", 0, 2, do_compact, OVS_RW },
+    { "convert", "[db [schema [dst]]]", 0, 3, do_convert, OVS_RW },
+    { "needs-conversion", NULL, 0, 2, do_needs_conversion, OVS_RO },
+    { "db-version", "[db]",  0, 1, do_db_version, OVS_RO },
+    { "db-cksum", "[db]", 0, 1, do_db_cksum, OVS_RO },
+    { "schema-version", "[schema]", 0, 1, do_schema_version, OVS_RO },
+    { "schema-cksum", "[schema]", 0, 1, do_schema_cksum, OVS_RO },
+    { "query", "[db] trns", 1, 2, do_query, OVS_RO },
+    { "transact", "[db] trns", 1, 2, do_transact, OVS_RO },
+    { "show-log", "[db]", 0, 1, do_show_log, OVS_RO },
+    { "help", NULL, 0, INT_MAX, do_help, OVS_RO },
+    { "list-commands", NULL, 0, INT_MAX, do_list_commands, OVS_RO },
+    { NULL, NULL, 0, 0, NULL, OVS_RO },
 };
 
 static const struct ovs_cmdl_command *get_all_commands(void)

diff --git a/tests/ovstest.c b/tests/ovstest.c
@@ -33,7 +33,7 @@ static size_t allocated_commands = 0;
 static void
 add_command(struct ovs_cmdl_command *cmd)
 {
-    const struct ovs_cmdl_command nil = {NULL, NULL, 0, 0, NULL};
+    const struct ovs_cmdl_command nil = {NULL, NULL, 0, 0, NULL, OVS_RO};
 
     while (n_commands + 1 >= allocated_commands) {
         commands = x2nrealloc(commands, &allocated_commands,
@@ -86,7 +86,7 @@ help(struct ovs_cmdl_context *ctx OVS_UNUSED)
 static void
 add_top_level_commands(void)
 {
-    struct ovs_cmdl_command help_cmd = {"--help", NULL, 0, 0, help};
+    struct ovs_cmdl_command help_cmd = {"--help", NULL, 0, 0, help, OVS_RO };
 
     add_command(&help_cmd);
 }

diff --git a/tests/test-bitmap.c b/tests/test-bitmap.c
@@ -149,9 +149,9 @@ run_benchmarks(struct ovs_cmdl_context *ctx)
 }
 
 static const struct ovs_cmdl_command commands[] = {
-    {"check", NULL, 0, 0, run_tests},
-    {"benchmark", NULL, 1, 1, run_benchmarks},
-    {NULL, NULL, 0, 0, NULL},
+    {"check", NULL, 0, 0, run_tests, OVS_RO},
+    {"benchmark", NULL, 1, 1, run_benchmarks, OVS_RO},
+    {NULL, NULL, 0, 0, NULL, OVS_RO},
 };
 
 static void

diff --git a/tests/test-ccmap.c b/tests/test-ccmap.c
@@ -272,9 +272,9 @@ benchmark_ccmap(void)
 
 
 static const struct ovs_cmdl_command commands[] = {
-    {"check", NULL, 0, 1, run_tests},
-    {"benchmark", NULL, 3, 3, run_benchmarks},
-    {NULL, NULL, 0, 0, NULL},
+    {"check", NULL, 0, 1, run_tests, OVS_RO},
+    {"benchmark", NULL, 3, 3, run_benchmarks, OVS_RO},
+    {NULL, NULL, 0, 0, NULL, OVS_RO},
 };
 
 static void

diff --git a/tests/test-classifier.c b/tests/test-classifier.c
@@ -1838,23 +1838,23 @@ static void help(struct ovs_cmdl_context *ctx);
 
 static const struct ovs_cmdl_command commands[] = {
     /* Classifier tests. */
-    {"empty", NULL, 0, 0, test_empty},
-    {"destroy-null", NULL, 0, 0, test_destroy_null},
-    {"single-rule", NULL, 0, 0, test_single_rule},
-    {"rule-replacement", NULL, 0, 0, test_rule_replacement},
-    {"many-rules-in-one-list", NULL, 0, 1, test_many_rules_in_one_list},
-    {"many-rules-in-one-table", NULL, 0, 1, test_many_rules_in_one_table},
-    {"many-rules-in-two-tables", NULL, 0, 0, test_many_rules_in_two_tables},
-    {"many-rules-in-five-tables", NULL, 0, 0, test_many_rules_in_five_tables},
-    {"benchmark", NULL, 0, 5, run_benchmarks},
+    {"empty", NULL, 0, 0, test_empty, OVS_RO },
+    {"destroy-null", NULL, 0, 0, test_destroy_null, OVS_RO },
+    {"single-rule", NULL, 0, 0, test_single_rule, OVS_RO },
+    {"rule-replacement", NULL, 0, 0, test_rule_replacement, OVS_RO },
+    {"many-rules-in-one-list", NULL, 0, 1, test_many_rules_in_one_list, OVS_RO },
+    {"many-rules-in-one-table", NULL, 0, 1, test_many_rules_in_one_table, OVS_RO },
+    {"many-rules-in-two-tables", NULL, 0, 0, test_many_rules_in_two_tables, OVS_RO },
+    {"many-rules-in-five-tables", NULL, 0, 0, test_many_rules_in_five_tables, OVS_RO },
+    {"benchmark", NULL, 0, 5, run_benchmarks, OVS_RO },
 
     /* Miniflow and minimask tests. */
-    {"miniflow", NULL, 0, 0, test_miniflow},
-    {"minimask_has_extra", NULL, 0, 0, test_minimask_has_extra},
-    {"minimask_combine", NULL, 0, 0, test_minimask_combine},
+    {"miniflow", NULL, 0, 0, test_miniflow, OVS_RO },
+    {"minimask_has_extra", NULL, 0, 0, test_minimask_has_extra, OVS_RO },
+    {"minimask_combine", NULL, 0, 0, test_minimask_combine, OVS_RO },
 
-    {"--help", NULL, 0, 0, help},
-    {NULL, NULL, 0, 0, NULL},
+    {"--help", NULL, 0, 0, help, OVS_RO },
+    {NULL, NULL, 0, 0, NULL, OVS_RO },
 };
 
 static void

diff --git a/tests/test-cmap.c b/tests/test-cmap.c
@@ -636,9 +636,9 @@ benchmark_hmap(void)
 }
 
 static const struct ovs_cmdl_command commands[] = {
-    {"check", NULL, 0, 1, run_tests},
-    {"benchmark", NULL, 3, 4, run_benchmarks},
-    {NULL, NULL, 0, 0, NULL},
+    {"check", NULL, 0, 1, run_tests, OVS_RO},
+    {"benchmark", NULL, 3, 4, run_benchmarks, OVS_RO},
+    {NULL, NULL, 0, 0, NULL, OVS_RO},
 };
 
 static void

diff --git a/tests/test-conntrack.c b/tests/test-conntrack.c
@@ -260,13 +260,13 @@ static const struct ovs_cmdl_command commands[] = {
      * is '1', each packet in a batch will have a different source and
      * destination port */
     {"benchmark", "n_threads n_pkts batch_size [change_connection]", 3, 4,
-     test_benchmark},
+     test_benchmark, OVS_RO},
     /* Reads packets from 'file' and sends them to the connection tracker,
      * 'batch_size' (1 by default) per call, with the commit flag set.
      * Prints the ct_state of each packet. */
-    {"pcap", "file [batch_size]", 1, 2, test_pcap},
+    {"pcap", "file [batch_size]", 1, 2, test_pcap, OVS_RO},
 
-    {NULL, NULL, 0, 0, NULL},
+    {NULL, NULL, 0, 0, NULL, OVS_RO},
 };
 
 static void

diff --git a/tests/test-heap.c b/tests/test-heap.c
@@ -461,16 +461,16 @@ test_heap_raw_delete(struct ovs_cmdl_context *ctx OVS_UNUSED)
 
 static const struct ovs_cmdl_command commands[] = {
     { "insert-delete-same-order", NULL, 0, 0,
-      test_heap_insert_delete_same_order, },
+      test_heap_insert_delete_same_order, OVS_RO },
     { "insert-delete-reverse-order", NULL, 0, 0,
-      test_heap_insert_delete_reverse_order, },
+      test_heap_insert_delete_reverse_order, OVS_RO },
     { "insert-delete-every-order", NULL, 0, 0,
-      test_heap_insert_delete_every_order, },
+      test_heap_insert_delete_every_order, OVS_RO },
     { "insert-delete-same-order-with-dups", NULL, 0, 0,
-      test_heap_insert_delete_same_order_with_dups, },
-    { "raw-insert", NULL, 0, 0, test_heap_raw_insert, },
-    { "raw-delete", NULL, 0, 0, test_heap_raw_delete, },
-    { NULL, NULL, 0, 0, NULL, },
+      test_heap_insert_delete_same_order_with_dups, OVS_RO },
+    { "raw-insert", NULL, 0, 0, test_heap_raw_insert, OVS_RO },
+    { "raw-delete", NULL, 0, 0, test_heap_raw_delete, OVS_RO },
+    { NULL, NULL, 0, 0, NULL, OVS_RO },
 };
 
 static void

diff --git a/tests/test-jsonrpc.c b/tests/test-jsonrpc.c
@@ -331,11 +331,11 @@ do_help(struct ovs_cmdl_context *ctx OVS_UNUSED)
 }
 
 static struct ovs_cmdl_command all_commands[] = {
-    { "listen", NULL, 1, 1, do_listen },
-    { "request", NULL, 3, 3, do_request },
-    { "notify", NULL, 3, 3, do_notify },
-    { "help", NULL, 0, INT_MAX, do_help },
-    { NULL, NULL, 0, 0, NULL },
+    { "listen", NULL, 1, 1, do_listen, OVS_RO },
+    { "request", NULL, 3, 3, do_request, OVS_RO },
+    { "notify", NULL, 3, 3, do_notify, OVS_RO },
+    { "help", NULL, 0, INT_MAX, do_help, OVS_RO },
+    { NULL, NULL, 0, 0, NULL, OVS_RO },
 };
 
 static struct ovs_cmdl_command *

diff --git a/tests/test-netlink-conntrack.c b/tests/test-netlink-conntrack.c
@@ -161,14 +161,14 @@ static const struct ovs_cmdl_command commands[] = {
     /* Linux netlink connection tracker interface test. */
 
     /* Prints all the entries in the connection table and exits. */
-    {"dump", "[zone=zone]", 0, 1, test_nl_ct_dump},
+    {"dump", "[zone=zone]", 0, 1, test_nl_ct_dump, OVS_RO},
     /* Listens to all the connection tracking events and prints them to
      * standard output until killed. */
-    {"monitor", "", 0, 0, test_nl_ct_monitor},
+    {"monitor", "", 0, 0, test_nl_ct_monitor, OVS_RO},
     /* Flushes all the entries from all the tables.. */
-    {"flush", "[zone=zone]", 0, 1, test_nl_ct_flush},
+    {"flush", "[zone=zone]", 0, 1, test_nl_ct_flush, OVS_RO},
 
-    {NULL, NULL, 0, 0, NULL},
+    {NULL, NULL, 0, 0, NULL, OVS_RO},
 };
 
 static void