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

[Jason Wang <jasowang@redhat.com>]

----- Original Message -----
> 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?

I think it works and there's no need for sticking to the old HMP
format since it's not an ABI. And it would be better if you can
generate more human readable format from JSON.

Thanks


> 
> 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
> 
>