Laurent Vivier <lvivier@redhat.com> writes:
On 21/06/2022 10:49, Markus Armbruster wrote:
Laurent Vivier <lvivier@redhat.com> writes:
On 20/06/2022 17:21, Markus Armbruster wrote:
Laurent Vivier <lvivier@redhat.com> writes:
Copied from socket netdev file and modified to use SocketAddress
to be able to introduce new features like unix socket.
"udp" and "mcast" are squashed into dgram netdev, multicast is detected
according to the IP address type.
"listen" and "connect" modes are managed by stream netdev. An optional
parameter "server" defines the mode (server by default)
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
---
[...]
diff --git a/net/net.c b/net/net.c
index c337d3d753fe..440957b272ee 100644
--- a/net/net.c
+++ b/net/net.c
...
@@ -1612,7 +1617,19 @@ void net_init_clients(void)
*/
static bool netdev_is_modern(const char *optarg)
{
- return false;
+ QDict *args;
+ const char *type;
+ bool is_modern;
+
+ args = keyval_parse(optarg, "type", NULL, NULL);
+ if (!args) {
+ return false;
+ }
+ type = qdict_get_try_str(args, "type");
+ is_modern = !g_strcmp0(type, "stream") || !g_strcmp0(type, "dgram");
+ qobject_unref(args);
+
+ return is_modern;
}
You could use g_autoptr here:
g_autoptr(QDict) args = NULL;
const char *type;
bool is_modern;
args = keyval_parse(optarg, "type", NULL, NULL);
if (!args) {
return false;
}
type = qdict_get_try_str(args, "type");
return !g_strcmp0(type, "stream") || !g_strcmp0(type, "dgram");
Matter of taste; you decide.
Looks good. We already had some series to convert existing code to g_autoptr(),
so it
seems the way to do.
Now recall how this function is used: it decides whether to parse the
modern way (with qobject_input_visitor_new_str()) or the traditional way
(with qemu_opts_parse_noisily()).
qemu_opts_parse_noisily() parses into a QemuOpts, for later use with the
opts visitor.
qobject_input_visitor_new_str() supports both dotted keys and JSON. The
former is parsed with keyval_parse(), the latter with
qobject_from_json(). It returns the resulting parse tree wrapped in a
suitable QAPI input visitor.
Issue 1: since we get there only when keyval_parse() succeeds, JSON is
unreachable. Reproducer:
$ qemu-system-x86_64 -netdev '{"id":"foo"}'
upstream-qemu: -netdev {"id":"foo"}: Parameter 'id' is missing
This is parsed with qemu_opts_parse_noisily(), resulting in a QemuOpts
with a single option 'type' with value '{"id":"foo"}'. The error
message comes from the opts visitor.
To fix this, make netdev_is_modern() return true when optarg[0] == '{'.
This matches how qobject_input_visitor_new_str() recognizes JSON.
OK
Issue 2: when keyval_parse() detects an error, we throw it away and fall
back to QemuOpts. This is commonly what we want. But not always. For
instance:
$ qemu-system-x86_64 -netdev
'type=stream,id=foo,addr.type=inet,addr.host=localhost,addr.port=1234,addr.ipv4-off'
Note the typo "ipv4-off" instead of ipv4=off. The error reporting is crap:
qemu-system-x86_64: -netdev
type=stream,id=foo,addr.type=inet,addr.host=localhost,addr.port=1234,addr.ipv4-off:
warning: short-form boolean option 'addr.ipv4-off' deprecated
Please use addr.ipv4-off=on instead
qemu-system-x86_64: -netdev
type=stream,id=foo,addr.type=inet,addr.host=localhost,addr.port=1234,addr.ipv4-off:
Parameter 'type' is missing
We get this because netdev_is_modern() guesses wrongly: keyval_parse()
fails with the perfectly reasonable error message "Expected '=' after
parameter 'addr.ipv4-off'", but netdev_is_modern() ignores the error,
and fails. We fall back to QemuOpts, and confusion ensues.
I'm not sure we can do much better with reasonable effort. If we decide
to accept this behavior, it should be documented at least in the source
code.
What about using modern syntax by default?
args = keyval_parse(optarg, "type", NULL, NULL);
if (!args) {
/* cannot detect the syntax, use new style syntax */
return true;
}
As is, netdev_is_modern() has three cases:
1. keyval_parse() fails
2. keyval_parse() succeeds, but value of @type is not modern
3. keyval_parse() succeeds, and value of @type is modern
In case 3. we're sure, because even if qemu_opts_parse_noisily() also
succeeded, it would result in the same value of @type.
In case 2, assuming traditional seems reasonable. The assumption can be
wrong when the user intends modern, but fat-fingers the type=T part.
In case 1, we know nothing.
Guessing modern is wrong when the user intends traditional. This
happens when a meant-to-be-traditional @optarg also parses as modern.
Quite possible.
I don't see why keyval_parse() fails in this case. Any example?
Brain cramp on my part, I'm afraid %-} Let me start over.
Guessing modern is wrong when the user intends traditional. Two
sub-cases then:
* @optarg parses fine as traditional. For instance,
$ qemu-system-x86_64 -netdev type=user,id=foo,ipv4
parses with a warning:
option 'ipv4' deprecated
Please use ipv4=on instead
This is how current master behaves.
Guessing modern makes this fail instead:
qemu-system-x86_64: -netdev type=user,id=foo,ipv4: Expected '=' after
parameter 'ipv4'
Regression.
* @optarg fails to parse traditional, too. The error reporting is for
modern even though the user intends traditional. Can be misleading.
Example:
$ qemu-system-x86_64 -netdev type=user,id=_,ipv4
Current master:
qemu-system-x86_64: -netdev type=user,id=_,ipv4: Parameter 'id' expects an
identifier
Identifiers consist of letters, digits, '-', '.', '_', starting with a
letter.
Guessing modern instead:
qemu-system-x86_64: -netdev type=user,id=_,ipv4: Expected '=' after
parameter 'ipv4'
This should be rare in practice, as traditional parsing detects very
few errors.
Guessing traditional is wrong when the user intends modern. This
happens when a meant-to-be-modern @optarg fails to parse as modern,
i.e. whenever the user screws up modern syntax.
This one is the example you gave (ipv4-off)
Two sub-cases then:
* @optarg parses fine as traditional. The parse result is unlikely to
make sense, though. For instance,
$ qemu-system-x86_64 -netdev type=stream,id=foo,server
parses with a warning:
qemu-system-x86_64: -netdev type=stream,id=foo,server: warning: short-form
boolean option 'server' deprecated
Please use server=on instead
But the result fails in the opts visitor:
qemu-system-x86_64: -netdev type=stream,id=foo,server: Parameter 'type' is
missing
In this case, we're better off with guessing modern:
qemu-system-x86_64: -netdev type=stream,id=foo,server: Expected '=' after
parameter
* @optarg fails to parse traditional, too. The error reporting is for
traditional even though the user intends modern. Can be misleading.
This is my ipv4-off example.
Can't win. Parsers simply don't compose that way.
Which guess is less bad? I'm not sure. Thoughts?
Perhaps we can simply fail if keyval_parse() fails?
something like:
args = keyval_parse(optarg, "type", NULL, &error_fatal);
type = qdict_get_try_str(args, "type");
return !g_strcmp0(type, "stream") || !g_strcmp0(type, "dgram");
This rejects working option arguments that don't also parse as modern,
such as "-netdev type=user,id=foo,ipv4".
Guessing traditional seems to be the least bad solution so far.
Supporting both traditional and modern syntax in an option argument is a
swamp. Can we bypass it somehow?
-object uses traditional QemuOpts parsing for key=value,..., and modern
parsing for JSON. Sticking to traditional sidesteps compatibility
issues. But you have to use JSON for things traditional can't express.
Thoughts?