[ovs-dev] [PATCH] ovsdb-server: Write manager status information to Manager table.

Andrew Evans aevans at nicira.com
Tue Feb 1 22:26:42 UTC 2011


On 2/1/11 9:55 AM, Ben Pfaff wrote:
> I think that parse_db_column() will segfault if its 'name_' argument
> does not contain a colon.  Indeed:

Fixed by reverting those changes:

diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
index 577fbd5..09bbd43 100644
--- a/ovsdb/ovsdb-server.c
+++ b/ovsdb/ovsdb-server.c
@@ -186,51 +186,38 @@ main(int argc, char *argv[])
 }

 static void
-split_remote_spec(const char *name_, char **proto,
-                  char **table_name, char **column_name)
-{
-    char *name, *save_ptr = NULL;
-
-    name = xstrdup(name_);
-    *proto = xstrdup(strtok_r(name, ":", &save_ptr)); /* "db:" */
-    *table_name = xstrdup(strtok_r(NULL, ",", &save_ptr));
-    *column_name = strtok_r(NULL, ",", &save_ptr);
-    if (*column_name) {
-        *column_name = xstrdup(*column_name);
-    }
-    free(name);
-}
-
-static void
 parse_db_column(const struct ovsdb *db,
-                const char *name,
+                const char *name_,
                 const struct ovsdb_table **tablep,
                 const struct ovsdb_column **columnp)
 {
-    char *proto, *table_name, *column_name;
-
-    split_remote_spec(name, &proto, &table_name, &column_name);
+    char *name, *table_name, *column_name;
+    const struct ovsdb_column *column;
+    const struct ovsdb_table *table;
+    char *save_ptr = NULL;

-    if (!strcmp(proto, "db")) {
-        if (!table_name || !column_name) {
-            ovs_fatal(0, "\"%s\": invalid syntax", name);
-        }
+    name = xstrdup(name_);
+    strtok_r(name, ":", &save_ptr); /* "db:" */
+    table_name = strtok_r(NULL, ",", &save_ptr);
+    column_name = strtok_r(NULL, ",", &save_ptr);
+    if (!table_name || !column_name) {
+        ovs_fatal(0, "\"%s\": invalid syntax", name_);
+    }

-        *tablep = ovsdb_get_table(db, table_name);
-        if (!*tablep) {
-            ovs_fatal(0, "\"%s\": no table named %s", name, table_name);
-        }
+    table = ovsdb_get_table(db, table_name);
+    if (!table) {
+        ovs_fatal(0, "\"%s\": no table named %s", name_, table_name);
+    }

-        *columnp = ovsdb_table_schema_get_column((*tablep)->schema,
column_name);
-        if (!*columnp) {
-            ovs_fatal(0, "\"%s\": table \"%s\" has no column \"%s\"",
-                      name, table_name, column_name);
-        }
+    column = ovsdb_table_schema_get_column(table->schema, column_name);
+    if (!column) {
+        ovs_fatal(0, "\"%s\": table \"%s\" has no column \"%s\"",
+                  name_, table_name, column_name);
     }
+    free(name);

-    free(proto);
-    free(table_name);
-    free(column_name);
+    *columnp = column;
+    *tablep = table;
 }

 static void

> I'm not sure why you removed the warning in add_manager_options().  Did
> it just get logged too much?  You could use VLOG_INFO_ONCE instead.

Reverted:

diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
index 2b15218..99e3dc1 100644
--- a/ovsdb/ovsdb-server.c
+++ b/ovsdb/ovsdb-server.c
@@ -408,11 +408,14 @@ write_string_string_column(struct ovsdb_row *row,
const char *column_name,
 static void
 add_manager_options(struct shash *remotes, const struct ovsdb_row *row)
 {
+    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
     struct ovsdb_jsonrpc_options *options;
     long long int max_backoff, probe_interval;
     const char *target;

     if (!read_string_column(row, "target", &target) || !target) {
+        VLOG_INFO_RL(&rl, "Table `%s' has missing or invalid `target'
column",
+                     row->table->schema->name);
         return;
     }


> What's the reason for doing the update in two phases, first collecting
> rows in get_remote_rows() and then updating them in the caller?  Does
> this somehow work better than just iterating over the rows and modifying
> them in a single pass?  It looks like this way actually involves more
> code and more work, so I'm curious about the design motivation here.

New, cleaner version:

diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
index 09bbd43..2b15218 100644
--- a/ovsdb/ovsdb-server.c
+++ b/ovsdb/ovsdb-server.c
@@ -467,25 +467,66 @@ query_db_remotes(const char *name, const struct
ovsdb *db,
     }
 }

-static size_t
-get_remote_rows(const struct ovsdb *db, const char *remote_spec,
-                const struct ovsdb_row ***rows)
+static void
+update_remote_row(const struct ovsdb_row *row, struct ovsdb_txn *txn,
+                  const struct shash *statuses)
+{
+    struct ovsdb_row *rw_row;
+    const char *target;
+    const struct ovsdb_jsonrpc_remote_status *status;
+    char *keys[3], *values[3];
+    size_t n = 0;
+
+    /* Get the "target" (protocol/host/port) spec. */
+    if (!read_string_column(row, "target", &target)) {
+        /* Bad remote spec or incorrect schema. */
+        return;
+    }
+
+    /* Prepare to modify this row. */
+    rw_row = ovsdb_txn_row_modify(txn, row);
+
+    /* Find status information for this target. */
+    status = shash_find_data(statuses, target);
+
+    /* Update status information columns. */
+
+    write_bool_column(rw_row, "is_connected",
+                      status->is_connected);
+
+    keys[n] = xstrdup("state");
+    values[n++] = xstrdup(status->state);
+    keys[n] = xstrdup("time_in_state");
+    values[n++] = xasprintf("%u", status->state_elapsed);
+    if (status->last_error) {
+        keys[n] = xstrdup("last_error");
+        values[n++] =
+            xstrdup(ovs_retval_to_string(status->last_error));
+    }
+    write_string_string_column(rw_row, "status", keys, values, n);
+}
+
+static void
+update_remote_rows(const struct ovsdb *db, struct ovsdb_txn *txn,
+                   const char *remote_name, const struct shash *statuses)
 {
     const struct ovsdb_table *table, *ref_table;
     const struct ovsdb_column *column;
     const struct ovsdb_row *row;
-    size_t max_rows = 0, n_rows = 0;

-    parse_db_column(db, remote_spec, &table, &column);
+    if (strncmp("db:", remote_name, 3)) {
+        return;
+    }
+
+    parse_db_column(db, remote_name, &table, &column);

     if (column->type.key.type != OVSDB_TYPE_UUID
         || !column->type.key.u.uuid.refTable
         || column->type.value.type != OVSDB_TYPE_VOID) {
-        return 0;
+        return;
     }

     ref_table = column->type.key.u.uuid.refTable;
-    *rows = NULL;

     HMAP_FOR_EACH (row, hmap_node, &table->rows) {
         const struct ovsdb_datum *datum;
@@ -497,14 +538,10 @@ get_remote_rows(const struct ovsdb *db, const char
*remote_spec,

             ref_row = ovsdb_table_get_row(ref_table, &datum->keys[i].uuid);
             if (ref_row) {
-                if (n_rows >= max_rows) {
-                    *rows = x2nrealloc(*rows, &max_rows, sizeof **rows);
-                }
-                (*rows)[n_rows++] = ref_row;
+                update_remote_row(ref_row, txn, statuses);
             }
         }
     }
-    return n_rows;
 }

 static void
@@ -525,50 +562,7 @@ update_remote_status(const struct
ovsdb_jsonrpc_server *jsonrpc,

     /* Iterate over --remote arguments given on command line. */
     SHASH_FOR_EACH (remote, remotes) {
-        const struct ovsdb_row **remote_rows;
-        size_t n_remote_rows;
-        size_t i;
-
-        /* Get all rows for this remote. */
-        n_remote_rows = get_remote_rows(db, remote->name, &remote_rows);
-
-        /* Iterate over each row found. */
-        for (i = 0; i < n_remote_rows; ++i) {
-            struct ovsdb_row *rw_row;
-            const char *target;
-            struct ovsdb_jsonrpc_remote_status *status;
-            char *keys[3], *values[3];
-            size_t n = 0;
-
-            /* Get the "target" (protocol/host/port) spec. */
-            if (!read_string_column(remote_rows[i], "target", &target)) {
-                /* Bad remote spec or incorrect schema. */
-                break;
-            }
-
-            /* Prepare to modify this row. */
-            rw_row = ovsdb_txn_row_modify(txn, remote_rows[i]);
-
-            /* Find status information for this target. */
-            status = shash_find_data(&statuses, target);
-
-            /* Update status information columns. */
-
-            write_bool_column(rw_row, "is_connected",
-                              status->is_connected);
-
-            keys[n] = xstrdup("state");
-            values[n++] = xstrdup(status->state);
-            keys[n] = xstrdup("time_in_state");
-            values[n++] = xasprintf("%u", status->state_elapsed);
-            if (status->last_error) {
-                keys[n] = xstrdup("last_error");
-                values[n++] =
-                    xstrdup(ovs_retval_to_string(status->last_error));
-            }
-            write_string_string_column(rw_row, "status", keys, values, n);
-        }
-        free(remote_rows);
+        update_remote_rows(db, txn, remote->name, &statuses);
     }

     error = ovsdb_txn_commit(txn, durable_txn);

> I think that write_string_string_column() could use
> ovsdb_datum_sort_assert(), since its caller never adds a duplicate key.

diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
index 99e3dc1..93ccd98 100644
--- a/ovsdb/ovsdb-server.c
+++ b/ovsdb/ovsdb-server.c
@@ -369,11 +369,9 @@ static void
 write_string_string_column(struct ovsdb_row *row, const char *column_name,
                            char **keys, char **values, size_t n)
 {
-    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
     const struct ovsdb_column *column;
     struct ovsdb_datum *datum;
     size_t i;
-    struct ovsdb_error *error;

     column = ovsdb_table_schema_get_column(row->table->schema,
column_name);
     datum = get_datum(row, column_name, OVSDB_TYPE_STRING,
OVSDB_TYPE_STRING,
@@ -396,11 +394,7 @@ write_string_string_column(struct ovsdb_row *row,
const char *column_name,
     }

     /* Sort and check constraints. */
-    error = ovsdb_datum_sort(datum, column->type.key.type);
-    if (error) {
-        VLOG_ERR_RL(&rl, "Error sorting string/string map '%s': %s",
-                    column_name, ovsdb_error_to_string(error));
-    }
+    ovsdb_datum_sort_assert(datum, column->type.key.type);
 }

 /* Adds a remote and options to 'remotes', based on the Manager table
row in


> In update_remote_status(), I'm not sure that it's guaranteed that
> 'status' will be nonnull.  Are you sure about that?

I have verified that a target of ssk:127.0.0.1:6632 doesn't cause a
crash. Nonetheless, I've defanged this with this patch:

diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
index 93ccd98..41f1146 100644
--- a/ovsdb/ovsdb-server.c
+++ b/ovsdb/ovsdb-server.c
@@ -485,6 +485,10 @@ update_remote_row(const struct ovsdb_row *row,
struct ovsdb_txn *txn,

     /* Find status information for this target. */
     status = shash_find_data(statuses, target);
+    if (!status) {
+        /* Should never happen, but just in case... */
+        return;
+    }

     /* Update status information columns. */





More information about the dev mailing list