core/ovs: cleanup handling of call id for OVS commands

- rename "id" to something more distinct: "call_id".

- consistently use guint64 type. We don't want nor need
  to handle negative values. For CALL_ID_UNSPEC we can use
  G_MAXUINT64.

- don't use "i" format string for the call id. That expects
  an "int", so it's not clear how this was working correctly
  previously. Also, "int" has a smaller range than our 64bits.
  Use instead "json_int_t" and cast properly in the variadic
  arguments of json_pack().
This commit is contained in:
Thomas Haller 2020-11-05 15:10:52 +01:00
parent 609b08e2eb
commit e05edcfd7e
No known key found for this signature in database
GPG key ID: 29C2366E4DFC5728

View file

@ -53,7 +53,7 @@ typedef struct {
size_t bufp; /* Last decoded byte in the input buffer. */ size_t bufp; /* Last decoded byte in the input buffer. */
GString * input; /* JSON stream waiting for decoding. */ GString * input; /* JSON stream waiting for decoding. */
GString * output; /* JSON stream to be sent. */ GString * output; /* JSON stream to be sent. */
gint64 seq; guint64 call_id_counter;
GArray * calls; /* Method calls waiting for a response. */ GArray * calls; /* Method calls waiting for a response. */
GHashTable * interfaces; /* interface uuid => OpenvswitchInterface */ GHashTable * interfaces; /* interface uuid => OpenvswitchInterface */
GHashTable * ports; /* port uuid => OpenvswitchPort */ GHashTable * ports; /* port uuid => OpenvswitchPort */
@ -104,9 +104,10 @@ typedef enum {
OVSDB_SET_INTERFACE_MTU, OVSDB_SET_INTERFACE_MTU,
} OvsdbCommand; } OvsdbCommand;
#define CALL_ID_UNSPEC G_MAXUINT64
typedef struct { typedef struct {
gint64 id; guint64 call_id;
#define COMMAND_PENDING -1 /* id not yet assigned */
OvsdbCommand command; OvsdbCommand command;
OvsdbMethodCallback callback; OvsdbMethodCallback callback;
gpointer user_data; gpointer user_data;
@ -207,7 +208,7 @@ ovsdb_call_method(NMOvsdb * self,
g_array_set_size(priv->calls, priv->calls->len + 1); g_array_set_size(priv->calls, priv->calls->len + 1);
call = &g_array_index(priv->calls, OvsdbMethodCall, priv->calls->len - 1); call = &g_array_index(priv->calls, OvsdbMethodCall, priv->calls->len - 1);
} }
call->id = COMMAND_PENDING; call->call_id = CALL_ID_UNSPEC;
call->command = command; call->command = command;
call->callback = callback; call->callback = callback;
call->user_data = user_data; call->user_data = user_data;
@ -975,20 +976,21 @@ ovsdb_next_command(NMOvsdb *self)
if (!priv->calls->len) if (!priv->calls->len)
return; return;
call = &g_array_index(priv->calls, OvsdbMethodCall, 0); call = &g_array_index(priv->calls, OvsdbMethodCall, 0);
if (call->id != COMMAND_PENDING) if (call->call_id != CALL_ID_UNSPEC)
return; return;
call->id = priv->seq++;
call->call_id = ++priv->call_id_counter;
switch (call->command) { switch (call->command) {
case OVSDB_MONITOR: case OVSDB_MONITOR:
msg = json_pack("{s:i, s:s, s:[s, n, {" msg = json_pack("{s:I, s:s, s:[s, n, {"
" s:[{s:[s, s, s]}]," " s:[{s:[s, s, s]}],"
" s:[{s:[s, s, s]}]," " s:[{s:[s, s, s]}],"
" s:[{s:[s, s, s, s]}]," " s:[{s:[s, s, s, s]}],"
" s:[{s:[]}]" " s:[{s:[]}]"
"}]}", "}]}",
"id", "id",
call->id, (json_int_t) call->call_id,
"method", "method",
"monitor", "monitor",
"params", "params",
@ -1025,7 +1027,13 @@ ovsdb_next_command(NMOvsdb *self)
call->bridge_device, call->bridge_device,
call->interface_device); call->interface_device);
msg = json_pack("{s:i, s:s, s:o}", "id", call->id, "method", "transact", "params", params); msg = json_pack("{s:I, s:s, s:o}",
"id",
(json_int_t) call->call_id,
"method",
"transact",
"params",
params);
break; break;
case OVSDB_DEL_INTERFACE: case OVSDB_DEL_INTERFACE:
params = json_array(); params = json_array();
@ -1034,7 +1042,13 @@ ovsdb_next_command(NMOvsdb *self)
_delete_interface(self, params, call->ifname); _delete_interface(self, params, call->ifname);
msg = json_pack("{s:i, s:s, s:o}", "id", call->id, "method", "transact", "params", params); msg = json_pack("{s:I, s:s, s:o}",
"id",
(json_int_t) call->call_id,
"method",
"transact",
"params",
params);
break; break;
case OVSDB_SET_INTERFACE_MTU: case OVSDB_SET_INTERFACE_MTU:
params = json_array(); params = json_array();
@ -1055,7 +1069,13 @@ ovsdb_next_command(NMOvsdb *self)
"==", "==",
call->ifname)); call->ifname));
msg = json_pack("{s:i, s:s, s:o}", "id", call->id, "method", "transact", "params", params); msg = json_pack("{s:I, s:s, s:o}",
"id",
(json_int_t) call->call_id,
"method",
"transact",
"params",
params);
break; break;
} }
@ -1437,7 +1457,7 @@ ovsdb_got_msg(NMOvsdb *self, json_t *msg)
0, 0,
}; };
json_t * json_id = NULL; json_t * json_id = NULL;
gint64 id = -1; json_int_t id = (json_int_t) -1;
const char * method = NULL; const char * method = NULL;
json_t * params = NULL; json_t * params = NULL;
json_t * result = NULL; json_t * result = NULL;
@ -1490,18 +1510,18 @@ ovsdb_got_msg(NMOvsdb *self, json_t *msg)
return; return;
} }
if (id > -1) { if (id >= 0) {
/* This is a response to a method call. */ /* This is a response to a method call. */
if (!priv->calls->len) { if (!priv->calls->len) {
_LOGE("there are no queued calls expecting response %" G_GUINT64_FORMAT, id); _LOGE("there are no queued calls expecting response %" G_GUINT64_FORMAT, (guint64) id);
ovsdb_disconnect(self, FALSE, FALSE); ovsdb_disconnect(self, FALSE, FALSE);
return; return;
} }
call = &g_array_index(priv->calls, OvsdbMethodCall, 0); call = &g_array_index(priv->calls, OvsdbMethodCall, 0);
if (call->id != id) { if (call->call_id != id) {
_LOGE("expected a response to call %" G_GUINT64_FORMAT ", not %" G_GUINT64_FORMAT, _LOGE("expected a response to call %" G_GUINT64_FORMAT ", not %" G_GUINT64_FORMAT,
call->id, call->call_id,
id); (guint64) id);
ovsdb_disconnect(self, FALSE, FALSE); ovsdb_disconnect(self, FALSE, FALSE);
return; return;
} }
@ -1705,7 +1725,7 @@ ovsdb_disconnect(NMOvsdb *self, gboolean retry, gboolean is_disposing)
if (retry) { if (retry) {
if (priv->calls->len != 0) if (priv->calls->len != 0)
g_array_index(priv->calls, OvsdbMethodCall, 0).id = COMMAND_PENDING; g_array_index(priv->calls, OvsdbMethodCall, 0).call_id = CALL_ID_UNSPEC;
} else { } else {
nm_utils_error_set_cancelled(&error, is_disposing, "NMOvsdb"); nm_utils_error_set_cancelled(&error, is_disposing, "NMOvsdb");