Stefano Garzarella <sgarz...@redhat.com> writes:
> net_client_init() uses a variable length array to store the prefix
> of 'ipv6-net' parameter (e.g. if ipv6-net=fec0::0/64, the prefix
> is 'fec0::0').
> This patch introduces g_strsplit() to split the 'ipv6-net' parameter,
> so we can remove the variable length array.
>
> Suggested-by: Markus Armbruster <arm...@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarz...@redhat.com>
> ---
> net/net.c | 33 +++++++++++++++++++++------------
> 1 file changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/net/net.c b/net/net.c
> index d5071e49e2..932fa5abb5 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1118,29 +1118,38 @@ static int net_client_init(QemuOpts *opts, bool
> is_netdev, Error **errp)
> const char *ip6_net = qemu_opt_get(opts, "ipv6-net");
>
> if (ip6_net) {
> - char buf[strlen(ip6_net) + 1];
> + gchar **substrings;
> + char *prefix_addr;
> + unsigned long prefix_len = 64; /* Default 64bit prefix length. */
>
> - if (get_str_sep(buf, sizeof(buf), &ip6_net, '/') < 0) {
> - /* Default 64bit prefix length. */
> - qemu_opt_set(opts, "ipv6-prefix", ip6_net, &error_abort);
> - qemu_opt_set_number(opts, "ipv6-prefixlen", 64,
> &error_abort);
> - } else {
> + substrings = g_strsplit(ip6_net, "/", 2);
> + if (!substrings || !substrings[0]) {
> + error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> + "ipv6-net", "a valid IPv6 prefix");
> + g_strfreev(substrings);
> + goto out;
Indentation's off.
> + }
> +
> + *prefix_addr = substrings[0];
> +
> + if (substrings[1]) {
> /* User-specified prefix length. */
> - unsigned long len;
> int err;
>
> - qemu_opt_set(opts, "ipv6-prefix", buf, &error_abort);
> - err = qemu_strtoul(ip6_net, NULL, 10, &len);
> -
> + err = qemu_strtoul(substrings[1], NULL, 10, &prefix_len);
> if (err) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> "ipv6-prefixlen", "a number");
> + g_strfreev(substrings);
> goto out;
Two g_strfreev() before goto out. Avoidable: declare substrings at the
function level, initialize to NULL, then call g_strfreev(substrings) ...
> }
> -
> - qemu_opt_set_number(opts, "ipv6-prefixlen", len,
> &error_abort);
> }
> +
> + qemu_opt_set(opts, "ipv6-prefix", prefix_addr, &error_abort);
> + qemu_opt_set_number(opts, "ipv6-prefixlen", prefix_len,
> + &error_abort);
> qemu_opt_unset(opts, "ipv6-net");
> + g_strfreev(substrings);
> }
> }
if (is_netdev) {
visit_type_Netdev(v, NULL, (Netdev **)&object, &err);
} else {
visit_type_NetLegacy(v, NULL, (NetLegacy **)&object, &err);
}
if (!err) {
ret = net_client_init1(object, is_netdev, &err);
}
if (is_netdev) {
qapi_free_Netdev(object);
} else {
qapi_free_NetLegacy(object);
}
out:
error_propagate(errp, err);
... here. Your choice.
visit_free(v);
return ret;
}
With at least the indentation fixed:
Reviewed-by: Markus Armbruster <arm...@redhat.com>
Not this patch's problem: when visit_type_FOO() fails with an input
visitor such as @v, you should not call qapi_free_FOO(). Nothing bad
happens when you do, it's just sloppy. See visitor.h's big comment for
details.
Cleaner:
if (is_netdev) {
visit_type_Netdev(v, NULL, (Netdev **)&object, &err);
} else {
visit_type_NetLegacy(v, NULL, (NetLegacy **)&object, &err);
}
if (err) {
goto out;
}
ret = net_client_init1(object, is_netdev, &err);
if (is_netdev) {
qapi_free_Netdev(object);
} else {
qapi_free_NetLegacy(object);
}
out:
Or maybe:
if (is_netdev) {
visit_type_Netdev(v, NULL, &netdev, &err);
if (err) {
goto out;
}
ret = net_client_init1(netdev, is_netdev, &err);
qapi_free_Netdev(netdev);
} else {
visit_type_NetLegacy(v, NULL, &netlegacy, &err);
if (err) {
goto out;
}
ret = net_client_init1(netlegacy, is_netdev, &err);
qapi_free_NetLegacy(netlegacy);
}
out:
with
Netdev *netdev;
NetLegacy *netlegacy;
replacing @object.
Or one step further: observe net_client_init() is always called with a
compile-time constant second argument. Split it into two functions,
factor the common part into a helper.
