ovsdb-server: Correct malformed error replies to certain JSON-RPC req…

…uests. I realized that this bug existed only a few weeks ago, but it has been there since the earliest ovsdb-server code. Signed-off-by: Ben Pfaff <[email protected]> Acked-by: Alex Wang <[email protected]>
huiweics · Apr 1, 2015 · 508624b · 508624b
1 parent d18e52e
commit 508624b
Show file tree

Hide file tree

Showing 3 changed files with 43 additions and 14 deletions.
diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
@@ -78,8 +78,9 @@ static void ovsdb_jsonrpc_trigger_complete_done(
     struct ovsdb_jsonrpc_session *);
 
 /* Monitors. */
-static struct json *ovsdb_jsonrpc_monitor_create(
-    struct ovsdb_jsonrpc_session *, struct ovsdb *, struct json *params);
+static struct jsonrpc_msg *ovsdb_jsonrpc_monitor_create(
+    struct ovsdb_jsonrpc_session *, struct ovsdb *, struct json *params,
+    const struct json *request_id);
 static struct jsonrpc_msg *ovsdb_jsonrpc_monitor_cancel(
     struct ovsdb_jsonrpc_session *,
     struct json_array *params,
@@ -674,7 +675,7 @@ ovsdb_jsonrpc_lookup_db(const struct ovsdb_jsonrpc_session *s,
     return db;
 
 error:
-    *replyp = jsonrpc_create_reply(ovsdb_error_to_json(error), request->id);
+    *replyp = jsonrpc_create_error(ovsdb_error_to_json(error), request->id);
     ovsdb_error_destroy(error);
     return NULL;
 }
@@ -752,7 +753,7 @@ ovsdb_jsonrpc_session_lock(struct ovsdb_jsonrpc_session *s,
     return jsonrpc_create_reply(result, request->id);
 
 error:
-    reply = jsonrpc_create_reply(ovsdb_error_to_json(error), request->id);
+    reply = jsonrpc_create_error(ovsdb_error_to_json(error), request->id);
     ovsdb_error_destroy(error);
     return reply;
 }
@@ -812,7 +813,7 @@ ovsdb_jsonrpc_session_unlock(struct ovsdb_jsonrpc_session *s,
     return jsonrpc_create_reply(json_object_create(), request->id);
 
 error:
-    reply = jsonrpc_create_reply(ovsdb_error_to_json(error), request->id);
+    reply = jsonrpc_create_error(ovsdb_error_to_json(error), request->id);
     ovsdb_error_destroy(error);
     return reply;
 }
@@ -842,9 +843,8 @@ ovsdb_jsonrpc_session_got_request(struct ovsdb_jsonrpc_session *s,
     } else if (!strcmp(request->method, "monitor")) {
         struct ovsdb *db = ovsdb_jsonrpc_lookup_db(s, request, &reply);
         if (!reply) {
-            reply = jsonrpc_create_reply(
-                ovsdb_jsonrpc_monitor_create(s, db, request->params),
-                request->id);
+            reply = ovsdb_jsonrpc_monitor_create(s, db, request->params,
+                                                 request->id);
         }
     } else if (!strcmp(request->method, "monitor_cancel")) {
         reply = ovsdb_jsonrpc_monitor_cancel(s, json_array(request->params),
@@ -1221,9 +1221,10 @@ ovsdb_jsonrpc_parse_monitor_request(struct ovsdb_jsonrpc_monitor_table *mt,
     return NULL;
 }
 
-static struct json *
+static struct jsonrpc_msg *
 ovsdb_jsonrpc_monitor_create(struct ovsdb_jsonrpc_session *s, struct ovsdb *db,
-                             struct json *params)
+                             struct json *params,
+                             const struct json *request_id)
 {
     struct ovsdb_jsonrpc_monitor *m = NULL;
     struct json *monitor_id, *monitor_requests;
@@ -1310,7 +1311,8 @@ ovsdb_jsonrpc_monitor_create(struct ovsdb_jsonrpc_session *s, struct ovsdb *db,
         }
     }
 
-    return ovsdb_jsonrpc_monitor_get_initial(m);
+    return jsonrpc_create_reply(ovsdb_jsonrpc_monitor_get_initial(m),
+                                request_id);
 
 error:
     if (m) {
@@ -1319,7 +1321,7 @@ ovsdb_jsonrpc_monitor_create(struct ovsdb_jsonrpc_session *s, struct ovsdb *db,
 
     json = ovsdb_error_to_json(error);
     ovsdb_error_destroy(error);
-    return json;
+    return jsonrpc_create_error(json, request_id);
 }
 
 static struct jsonrpc_msg *

diff --git a/ovsdb/ovsdb-server.1.in b/ovsdb/ovsdb-server.1.in
@@ -251,7 +251,34 @@ vSwitch 2.4 and later extend <condition> to allow the use of \fB<\fR,
 of 0 or 1 integer'' and ``set of 0 or 1 real''.  These conditions
 evaluate to false when the column is empty, and otherwise as described
 in RFC 7047 for integer and real types.
-
+.
+.SH "BUGS"
+.
+In Open vSwitch before version 2.4, when \fBovsdb\-server\fR sent
+JSON-RPC error responses to some requests, it incorrectly formulated
+them with the \fBresult\fR and \fBerror\fR swapped, so that the
+response appeared to indicate success (with a nonsensical result)
+rather than an error.  The requests that suffered from this problem
+were:
+.
+.IP \fBtransact\fR
+.IQ \fBget_schema\fR
+Only if the request names a nonexistent database.
+.IP \fBmonitor\fR
+.IQ \fBlock\fR
+.IQ \fBunlock\fR
+In all error cases.
+.
+.PP
+Of these cases, the only error that a well-written application is
+likely to encounter in practice is \fBmonitor\fR of tables or columns
+that do not exist, in an situation where the application has been
+upgraded but the old database schema is still temporarily in use.  To
+handle this situation gracefully, we recommend that clients should
+treat a \fBmonitor\fR response with a \fBresult\fR that contains an
+\fBerror\fR key-value pair as an error (assuming that the database
+being monitored does not contain a table named \fBerror\fR).
+.
 .SH "SEE ALSO"
 .
 .BR ovsdb\-tool (1).
diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at
@@ -163,7 +163,7 @@ ordinals
 ], [ignore], [test ! -e pid || kill `cat pid`])
 AT_CHECK(
   [[ovstest test-jsonrpc request unix:socket get_schema [\"nonexistent\"]]], [0],
-  [[{"error":null,"id":0,"result":{"details":"get_schema request specifies unknown database nonexistent","error":"unknown database","syntax":"[\"nonexistent\"]"}}
+  [[{"error":{"details":"get_schema request specifies unknown database nonexistent","error":"unknown database","syntax":"[\"nonexistent\"]"},"id":0,"result":null}
 ]], [], [test ! -e pid || kill `cat pid`])
 OVSDB_SERVER_SHUTDOWN
 AT_CLEANUP