Re: [ovs-dev] [PATCH v4 6/6] unixctl: Pass output format as command args via JSON-RPC API.
On 03.01.24 00:57, Ilya Maximets wrote:
> On 11/16/23 11:41, jm...@redhat.com wrote:
>> From: Jakob Meng
>>
>> A previous patch had changed the JSON-RPC API in lib/unixctl.* (and
>> its Python counterpart) in order to allow transporting the requested
>> output format from ovs-xxx tools to ovs-vswitchd across unix sockets:
>>
>> The meaning of 'method' and 'params' had be changed: 'method' would be
>> 'execute/v1' when an output format other than 'text' is requested. In
>> that case, the first parameter of the JSON array 'params' would be the
>> designated command, the second one the output format and the rest
>> will be command args.
>> That change allowed to transport the output format in a backward
>> compatible way: One could use an updated client (ovs-appctl) with an
>> old server (ovs-vswitchd) and vice versa. Of course, JSON output would
>> only work when both sides have been updated.
>>
>> This patch reverts the JSON-RPC API to the old behaviour: The command
>> will be copied to JSON-RPC's 'method' parameter while command args will
>> be copied to the 'params' parameter.
>> The client will inject the output format into the command args and the
>> server will parse and remove it before passing the args to the command
>> callback.
>>
>> The advantage of this (old) approach is a simpler JSON-RPC API but at
>> the cost of inferior usability: ovs-xxx tools are no longer able to
>> detect missing JSON support at the server side. Old servers will simply
>> pass the output format as an arg to the command which would then result
>> in an error such as 'command takes at most ... arguments' or a non-\
>> uniform error from the command callback.
> Hi, Jakob.
>
> I see you don't like the option of passing a format argument by filtering
> it out. :)
>
> Both solutions look like nasty workarounds at this point, some more, some
> less, but in the end they all are just abusing the system. So, I came
> to a conclusion that two possible "correct" solutions are:
>
> 1. Implement JSON-RPC v2: https://www.jsonrpc.org/specification
>This will give us an automatic protocol version verification and
>we can pass paramaters as object, with separated server and method
>parameters, e.g.
> "params": {
> "server-params": { "format": "json" },
> "method-params": [ arg1, arg2, ... ]
> }
>This is not a lightweight option to implement, will require proper
>validation, support for both versions of the spec, and potentially
>support for features that we do not actually need.
>
> 2. Just make 2 calls. Add a new internal "unixctl_set_options" method
>that will change the options for a current connection. E.g.
>{
>"method": "unixctl_set_options",
>"params": [ "format:json" ],
>"id": 1
>}
>If that method fails - no json format. If the method succeeds, server
>will reply with JSON results for this connections.
>The current connection options can be directly stored in the struct
>unixctl_conn. Implementation of the method can be just a normal
>jsonrpc method registered by the unixctl module itself.
>
>Perfect compatibility with old servers, they will reject the first
>request with a clear error. Old clients will not use this method
>and have no changes in workflow at all. Only clients that need
>JSON replies will need to use this method.
>
>Should be much easier to implement. SO, I'd vote for this option.
>
> Thoughts?
>
> Best regards, Ilya Maximets.
>
Well, I agree with all your points 😄 Making 2 calls is a good idea, will update
my patch. Thank you!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 6/6] unixctl: Pass output format as command args via JSON-RPC API.
On 11/16/23 11:41, jm...@redhat.com wrote:
> From: Jakob Meng
>
> A previous patch had changed the JSON-RPC API in lib/unixctl.* (and
> its Python counterpart) in order to allow transporting the requested
> output format from ovs-xxx tools to ovs-vswitchd across unix sockets:
>
> The meaning of 'method' and 'params' had be changed: 'method' would be
> 'execute/v1' when an output format other than 'text' is requested. In
> that case, the first parameter of the JSON array 'params' would be the
> designated command, the second one the output format and the rest
> will be command args.
> That change allowed to transport the output format in a backward
> compatible way: One could use an updated client (ovs-appctl) with an
> old server (ovs-vswitchd) and vice versa. Of course, JSON output would
> only work when both sides have been updated.
>
> This patch reverts the JSON-RPC API to the old behaviour: The command
> will be copied to JSON-RPC's 'method' parameter while command args will
> be copied to the 'params' parameter.
> The client will inject the output format into the command args and the
> server will parse and remove it before passing the args to the command
> callback.
>
> The advantage of this (old) approach is a simpler JSON-RPC API but at
> the cost of inferior usability: ovs-xxx tools are no longer able to
> detect missing JSON support at the server side. Old servers will simply
> pass the output format as an arg to the command which would then result
> in an error such as 'command takes at most ... arguments' or a non-\
> uniform error from the command callback.
Hi, Jakob.
I see you don't like the option of passing a format argument by filtering
it out. :)
Both solutions look like nasty workarounds at this point, some more, some
less, but in the end they all are just abusing the system. So, I came
to a conclusion that two possible "correct" solutions are:
1. Implement JSON-RPC v2: https://www.jsonrpc.org/specification
This will give us an automatic protocol version verification and
we can pass paramaters as object, with separated server and method
parameters, e.g.
"params": {
"server-params": { "format": "json" },
"method-params": [ arg1, arg2, ... ]
}
This is not a lightweight option to implement, will require proper
validation, support for both versions of the spec, and potentially
support for features that we do not actually need.
2. Just make 2 calls. Add a new internal "unixctl_set_options" method
that will change the options for a current connection. E.g.
{
"method": "unixctl_set_options",
"params": [ "format:json" ],
"id": 1
}
If that method fails - no json format. If the method succeeds, server
will reply with JSON results for this connections.
The current connection options can be directly stored in the struct
unixctl_conn. Implementation of the method can be just a normal
jsonrpc method registered by the unixctl module itself.
Perfect compatibility with old servers, they will reject the first
request with a clear error. Old clients will not use this method
and have no changes in workflow at all. Only clients that need
JSON replies will need to use this method.
Should be much easier to implement. SO, I'd vote for this option.
Thoughts?
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v4 6/6] unixctl: Pass output format as command args via JSON-RPC API.
From: Jakob Meng
A previous patch had changed the JSON-RPC API in lib/unixctl.* (and
its Python counterpart) in order to allow transporting the requested
output format from ovs-xxx tools to ovs-vswitchd across unix sockets:
The meaning of 'method' and 'params' had be changed: 'method' would be
'execute/v1' when an output format other than 'text' is requested. In
that case, the first parameter of the JSON array 'params' would be the
designated command, the second one the output format and the rest
will be command args.
That change allowed to transport the output format in a backward
compatible way: One could use an updated client (ovs-appctl) with an
old server (ovs-vswitchd) and vice versa. Of course, JSON output would
only work when both sides have been updated.
This patch reverts the JSON-RPC API to the old behaviour: The command
will be copied to JSON-RPC's 'method' parameter while command args will
be copied to the 'params' parameter.
The client will inject the output format into the command args and the
server will parse and remove it before passing the args to the command
callback.
The advantage of this (old) approach is a simpler JSON-RPC API but at
the cost of inferior usability: ovs-xxx tools are no longer able to
detect missing JSON support at the server side. Old servers will simply
pass the output format as an arg to the command which would then result
in an error such as 'command takes at most ... arguments' or a non-\
uniform error from the command callback.
Signed-off-by: Jakob Meng
---
lib/unixctl.c| 147 ---
python/ovs/unixctl/client.py | 19 ++---
python/ovs/unixctl/server.py | 37 -
python/ovs/util.py | 1 -
4 files changed, 91 insertions(+), 113 deletions(-)
diff --git a/lib/unixctl.c b/lib/unixctl.c
index e7edbb154..1eaf6edb0 100644
--- a/lib/unixctl.c
+++ b/lib/unixctl.c
@@ -63,8 +63,6 @@ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
static struct shash commands = SHASH_INITIALIZER(&commands);
-static const char *rpc_marker = "execute/v1";
-
static void
unixctl_list_commands(struct unixctl_conn *conn, int argc OVS_UNUSED,
const char *argv[] OVS_UNUSED,
@@ -292,11 +290,9 @@ process_command(struct unixctl_conn *conn, struct
jsonrpc_msg *request)
struct unixctl_command *command;
struct json_array *params;
-const char *method;
-enum ovs_output_fmt fmt;
+enum ovs_output_fmt fmt = OVS_OUTPUT_FMT_TEXT;
struct svec argv = SVEC_EMPTY_INITIALIZER;
-int args_offset;
-bool plain_rpc;
+int argc;
COVERAGE_INC(unixctl_received);
conn->request_id = json_clone(request->id);
@@ -310,20 +306,9 @@ process_command(struct unixctl_conn *conn, struct
jsonrpc_msg *request)
free(id_s);
}
-/* The JSON-RPC API requires an indirection in order to allow transporting
- * additional data like the output format besides command and args. For
- * backward compatibility with older clients the plain RPC is still
- * supported. */
-plain_rpc = strcmp(request->method, rpc_marker);
-args_offset = plain_rpc ? 0 : 2;
-
params = json_array(request->params);
-if (!plain_rpc && (params->n < 2)) {
-error = xasprintf("JSON-RPC API mismatch: Unexpected # of params:"\
- " %"PRIuSIZE, params->n);
-goto error;
-}
+/* Verify type of arguments. */
for (size_t i = 0; i < params->n; i++) {
if (params->elems[i]->type != JSON_STRING) {
error = xasprintf("command has non-string argument: %s",
@@ -332,54 +317,70 @@ process_command(struct unixctl_conn *conn, struct
jsonrpc_msg *request)
}
}
-/* Extract method name. */
-method = plain_rpc ? request->method : json_string(params->elems[0]);
+argc = params->n;
-/* Extract output format. */
-if (plain_rpc) {
-fmt = OVS_OUTPUT_FMT_TEXT;
-} else {
-if (!ovs_output_fmt_from_string(json_string(params->elems[1]), &fmt)) {
-error = xasprintf("invalid output format: %s",
- json_string(params->elems[1]));
-goto error;
+/* Extract and process command args. */
+svec_add(&argv, request->method);
+for (size_t i = 0; i < params->n; i++) {
+if (!strcmp(json_string(params->elems[i]), "-f") ||
+!strcmp(json_string(params->elems[i]), "--format"))
+{
+/* Parse output format argument. */
+
+if ((i + 1) == (params->n)) {
+error = xasprintf("option requires an argument -- %s",
+ json_string(params->elems[i]));
+goto error;
+}
+
+/* Move index to option argument. */
+i++;
+
+if (!ovs_output_fmt_from_string(json_string(params->elems[i]),
+&fmt))
+{
+error = xaspri
