Re: [PATCH v5 3/4] hmp: Use QMP query-netdev in hmp_info_network

[Alexey Kirillov <lekiravi@yandex-team.ru>]

Hi!

07.12.2020, 08:52, "Jason Wang" <jasowang@redhat.com>:
> On 2020/11/9 上午7:59, Alexey Kirillov wrote:
>>  +#ifdef CONFIG_SLIRP
>>  + case NET_BACKEND_USER: {
>>  + size_t len = strchr(ni->u.user.net, '/') - ni->u.user.net;
>>  + char *net = g_strndup(ni->u.user.net, len);
>>  +
>>  + info_str = g_strdup_printf("net=%s,restrict=%s",
>>  + net,
>>  + ni->u.user.q_restrict ? "on" : "off");
>>  + g_free(net);
>>  + break;
>>  + }
>>  +#endif /* CONFIG_SLIRP */
>>  + case NET_BACKEND_TAP: {
>>  +#ifndef _WIN32
>>  + if (ni->u.tap.has_fds) {
>>  + char **fds = g_strsplit(ni->u.tap.fds, ":", -1);
>>  +
>>  + info_str = g_strdup_printf("fd=%s", fds[nc->queue_index]);
>>  + g_strfreev(fds);
>>  + } else if (ni->u.tap.has_helper) {
>>  + info_str = g_strdup_printf("helper=%s", ni->u.tap.helper);
>>  + } else {
>>  + info_str = g_strdup_printf("ifname=%s,script=%s,downscript=%s",
>>  + ni->u.tap.ifname,
>>  + nc->queue_index == 0 ? ni->u.tap.script : "no",
>>  + nc->queue_index == 0 ? ni->u.tap.downscript : "no");
>>  + }
>>  +#else
>>  + info_str = g_strdup_printf("tap: ifname=%s", ni->u.tap.ifname);
>>  +#endif /* _WIN32 */
>>  + break;
>>  + }
>>  +#ifdef CONFIG_L2TPV3
>>  + case NET_BACKEND_L2TPV3: {
>>  + info_str = g_strdup_printf("l2tpv3: connected");
>>  + break;
>>  + }
>>  +#endif /* CONFIG_L2TPV3 */
>>  + case NET_BACKEND_SOCKET: {
>>  + if (ni->u.socket.has_listen) {
>>  + if (ni->u.socket.has_fd) {
>>  + info_str = g_strdup_printf("socket: connection from %s",
>>  + ni->u.socket.listen);
>>  + } else {
>>  + info_str = g_strdup_printf("socket: wait from %s",
>>  + ni->u.socket.listen);
>>  + }
>>  + } else if (ni->u.socket.has_connect && ni->u.socket.has_fd) {
>>  + info_str = g_strdup_printf("socket: connect to %s",
>>  + ni->u.socket.connect);
>>  + } else if (ni->u.socket.has_mcast && ni->u.socket.has_fd) {
>>  + info_str = g_strdup_printf("socket: mcast=%s",
>>  + ni->u.socket.mcast);
>>  + } else if (ni->u.socket.has_udp && ni->u.socket.has_fd) {
>>  + info_str = g_strdup_printf("socket: udp=%s", ni->u.socket.udp);
>>  + } else {
>>  + g_assert(ni->u.socket.has_fd);
>>  + int so_type = -1;
>>  + int optlen = sizeof(so_type);
>>  + int fd = atoi(ni->u.socket.fd);
>>  +
>>  + getsockopt(fd, SOL_SOCKET, SO_TYPE, (char *)&so_type,
>>  + (socklen_t *)&optlen);
>>  + if (so_type == SOCK_STREAM) {
>>  + info_str = g_strdup_printf("socket: fd=%s",
>>  + ni->u.socket.fd);
>>  + } else {
>>  + if (ni->u.socket.has_mcast) {
>>  + /*
>>  + * This branch is unreachable, according to how it is in
>>  + * net/socket.c at this moment
>>  + */
>>  + info_str = g_strdup_printf("socket: fd=%s "
>>  + "(cloned mcast=%s)",
>>  + ni->u.socket.fd,
>>  + ni->u.socket.mcast);
>>  + } else {
>>  + SocketAddress *sa = socket_local_address(fd, NULL);
>>  +
>>  + info_str = g_strdup_printf("socket: fd=%s %s",
>>  + ni->u.socket.fd,
>>  + SocketAddressType_str(sa->type));
>>  + qapi_free_SocketAddress(sa);
>>  + }
>>  + }
>>  + }
>>  + break;
>>  + }
>>  +#ifdef CONFIG_VDE
>>  + case NET_BACKEND_VDE: {
>>  + info_str = g_strdup_printf("sock=%s,fd=%d",
>>  + ni->u.vde.sock,
>>  + net_vde_get_fd(nc));
>>  + break;
>>  + }
>>  +#endif /* CONFIG_VDE */
>>  +#ifdef CONFIG_NET_BRIDGE
>>  + case NET_BACKEND_BRIDGE: {
>>  + info_str = g_strdup_printf("helper=%s,br=%s",
>>  + ni->u.bridge.helper,
>>  + ni->u.bridge.br);
>>  + break;
>>  + }
>>  +#endif /* CONFIG_NET_BRIDGE */
>>  +#ifdef CONFIG_NETMAP
>>  + case NET_BACKEND_NETMAP: {
>>  + info_str = g_strdup_printf("netmap: ifname=%s",
>>  + ni->u.netmap.ifname);
>>  + break;
>>  + }
>>  +#endif /* CONFIG_NETMAP */
>>  +#ifdef CONFIG_VHOST_NET_USER
>>  + case NET_BACKEND_VHOST_USER: {
>>  + info_str = g_strdup_printf("vhost-user%d to %s",
>>  + nc->queue_index,
>>  + ni->u.vhost_user.chardev);
>>  + break;
>>  + }
>>  +#endif /* CONFIG_VHOST_NET_USER */
>>  +#ifdef CONFIG_VHOST_NET_VDPA
>>  + case NET_BACKEND_VHOST_VDPA: {
>>  + info_str = g_strdup("vhost-vdpa");
>>  + break;
>>  + }
>>  +#endif /* CONFIG_VHOST_NET_VDPA */
>
> This will introduce burdens for new netdevs or new attributes since
> people can easily forget to add the routine here.
>
> I think at least we need introduce callbacks for this.

Thanks for pointing. I can't remember why exactly I chose to not do it.
So it's definitely better to split this chunk to several callbacks.
I'll do it in the next version of series.

> One more stupid question, instead of generating the string via hard
> codes, is there any method (dict?) to iterate all the key/values
> automatically?
>
> Thanks

Oh yes, that the point.
Now there are no common format for info_str.
This patch is aimed to keep old HMP command mostly untouched.
But if we can drop old format, all this mess can be generalized as JSON
lines replacing old info_str stuff.

What do you think about that?

Originally I wanted to completely drop old info_str and use
QAPI to store and provide information about netdevs (and NICs too).

Thanks!

-- 
Alexey Kirillov
Yandex.Cloud