On Fri, Aug 26, 2016 at 04:15:49PM -0700, Andy Zhou wrote:
> Currently, 'sync-exclude-tables' command line options are simply stored
> in a string. Change the implementation to store it in an shash instead
> to improve modularity.
>
> One additional benefit of this change is that errors can be detected
> and reported to user earlier. Adde a 'dryrun' option to
> set_blacklist_tables() API to make this feature available to the
> command line option parsing and unixctl command parsing.
>
> Signed-off-by: Andy Zhou <az...@ovn.org>
Seems useful enough.
I have a few minor suggested improvements, see below.
Acked-by: Ben Pfaff <b...@ovn.org>
--8<--------------------------cut here-------------------------->8--
diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
index b1d7fc5..ed067d4 100644
--- a/ovsdb/ovsdb-server.c
+++ b/ovsdb/ovsdb-server.c
@@ -1,4 +1,4 @@
-/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
+/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2016 Nicira, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -1110,9 +1110,10 @@ ovsdb_server_set_sync_excluded_tables(struct
unixctl_conn *conn,
if (is_backup_server) {
replication_init();
}
- set_blacklist_tables(argv[1], false);
+ err = set_blacklist_tables(argv[1], false);
}
unixctl_command_reply(conn, err);
+ free(err);
}
static void
@@ -1463,8 +1464,7 @@ parse_options(int *argcp, char **argvp[],
case OPT_SYNC_EXCLUDE: {
char *err = set_blacklist_tables(optarg, false);
if (err) {
- printf("%s\n", err);
- exit(EXIT_FAILURE);
+ ovs_fatal(0, "%s", err);
}
break;
}
diff --git a/ovsdb/replication.c b/ovsdb/replication.c
index ebd348c..11384a1 100644
--- a/ovsdb/replication.c
+++ b/ovsdb/replication.c
@@ -145,13 +145,14 @@ get_active_ovsdb_server(void)
return active_ovsdb_server;
}
-/* Parse 'blacklist' to rebuild 'blacklist_tables'. The current
- * black list tables will be wiped out, regardless if 'blacklist'
- * can be parsed or not.
+/* Parse 'blacklist' to rebuild 'blacklist_tables'. If 'dryrun' is false, the
+ * current black list tables will be wiped out, regardless of whether
+ * 'blacklist' can be parsed. If 'dryrun' is true, only parses 'blacklist' and
+ * reports any errors, without modifying the blacklist.
*
- * On error, Returns the error string which the caller is
- * responsible for freeing. Return NULL otherwise. */
-char *
+ * On error, returns the error string, which the caller is
+ * responsible for freeing. Returns NULL otherwise. */
+char * OVS_WARN_UNUSED_RESULT
set_blacklist_tables(const char *blacklist, bool dryrun)
{
struct sset set = SSET_INITIALIZER(&set);
@@ -183,13 +184,13 @@ set_blacklist_tables(const char *blacklist, bool dryrun)
done:
sset_destroy(&set);
if (err && !dryrun) {
- /* On error, destory the partially built 'blacklist_tables'. */
+ /* On error, destroy the partially built 'blacklist_tables'. */
blacklist_tables_clear();
}
return err;
}
-char *
+char * OVS_WARN_UNUSED_RESULT
get_blacklist_tables(void)
{
struct shash_node *node;
@@ -201,9 +202,7 @@ get_blacklist_tables(void)
struct sset *tables = node->data;
SSET_FOR_EACH (table, tables) {
- struct ds ds= DS_EMPTY_INITIALIZER;
- ds_put_format(&ds, "%s:%s", database, table);
- sset_add(&set, ds_steal_cstr(&ds));
+ sset_add_and_free(&set, xasprintf("%s:%s", database, table));
}
}
@@ -212,7 +211,7 @@ get_blacklist_tables(void)
* that used to implement the hmap data structure. This is
* only useful for writting unit tests. */
const char **sorted = sset_sort(&set);
- struct ds ds= DS_EMPTY_INITIALIZER;
+ struct ds ds = DS_EMPTY_INITIALIZER;
size_t i;
for (i = 0; i < sset_count(&set); i++) {
ds_put_format(&ds, "%s,", sorted[i]);
@@ -256,7 +255,7 @@ static bool
blacklist_tables_find(const char *database, const char *table)
{
struct sset *tables = shash_find_data(&blacklist_tables, database);
- return tables ? sset_contains(tables, table) : false;
+ return tables && sset_contains(tables, table);
}
void
diff --git a/ovsdb/replication.h b/ovsdb/replication.h
index 1de9c8b..cc79199 100644
--- a/ovsdb/replication.h
+++ b/ovsdb/replication.h
@@ -21,7 +21,6 @@
#include <stdbool.h>
#include "openvswitch/shash.h"
-
struct db {
/* Initialized in main(). */
char *filename;
@@ -42,8 +41,9 @@ void replication_usage(void);
/* Unixctl APIs */
void set_active_ovsdb_server(const char *remote_server);
const char *get_active_ovsdb_server(void);
-char *set_blacklist_tables(const char *blacklist, bool dryrun);
-char *get_blacklist_tables(void);
+char *set_blacklist_tables(const char *blacklist, bool dryrun)
+ OVS_WARN_UNUSED_RESULT;
+char *get_blacklist_tables(void) OVS_WARN_UNUSED_RESULT;
void disconnect_active_server(void);
const struct db *find_db(const struct shash *all_dbs, const char *db_name);
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
