Re: [PATCH 4/5] conf, docs, schemas: Add support for interface type vds

On Wed, Aug 17, 2022 at 02:50:39PM +0200, Martin Kletzander wrote: > This represents an interface connected to a VMWare Distributed Switch, > previously obscured as a dummy interface. > > Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; > --- > docs/formatdomain.rst | 30 +++++++++++---- > src/ch/ch_monitor.c | 1 + > src/conf/domain_conf.c | 64 +++++++++++++++++++++++++++++++ > src/conf/domain_conf.h | 7 ++++ > src/conf/netdev_bandwidth_conf.c | 1 + > src/conf/schemas/domaincommon.rng | 23 +++++++++++ > src/libxl/libxl_conf.c | 1 + > src/libxl/xen_common.c | 1 + > src/lxc/lxc_controller.c | 1 + > src/lxc/lxc_driver.c | 3 ++ > src/lxc/lxc_process.c | 2 + > src/qemu/qemu_command.c | 4 ++ > src/qemu/qemu_domain.c | 1 + > src/qemu/qemu_hotplug.c | 3 ++ > src/qemu/qemu_interface.c | 2 + > src/qemu/qemu_process.c | 2 + > src/qemu/qemu_validate.c | 1 + > src/vmx/vmx.c | 1 + > tools/virsh-domain.c | 1 + > 19 files changed, 141 insertions(+), 8 deletions(-) > > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst > index ed0d9c19593b..3a8aa96fdc0a 100644 > --- a/docs/formatdomain.rst > +++ b/docs/formatdomain.rst > @@ -5244,22 +5244,36 @@ Dummy network interface > ^^^^^^^^^^^^^^^^^^^^^^^ > > An unconnected network interface sounds pretty pointless, but can show up for > -example with VMWare when libvirt does not have any more information to provide. > -Two such scenarios are currently known: > +example with VMWare without any specified network to be connected to. > +:since:`Since 8.7.0` > > -1) network interface exists, but is not connected to any existing network > -2) the interface is connected to something known as VMWare Distributed Switch > +:: > + > + ... > + <devices> > + <interface type='dummy'> > + <mac address='52:54:00:22:c9:42'/> > + </interface> > + </devices> > + ... > > -The difference between these two is not (yet?) discoverable by libvirt, so at > -least the information gathered from the hypervisor is provided in the > -element. :since:`Since 8.7.0` > +VMWare Distributed Switch > +^^^^^^^^^^^^^^^^^^^^^^^^^ > + > +Interface can be connected to VMWare Distributed Switch, but since libvirt > +cannot provide information about that architecture, the information presented > +here is only what can be gathered from the VM configuration. VMs with this > +interface type can be created, so that editing of the XML works properly, > +however libvirt cannot guarantee that any changes in these parameters will be > +valid in the hypervisor. :since:`Since 8.7.0` > > :: > > ... > <devices> > - <interface type='dummy'> > + <interface type='vds'> > <mac address='52:54:00:22:c9:42'/> > + <source switchid='12345678-1234-1234-1234-123456789abc' portid='6' portgroupid='pg-4321' connectionid='12345'/> > </interface> > </devices> > ... > @@ -8899,6 +8905,8 @@ virDomainNetDefParseXML(virDomainXMLOption *xmlopt, > g_autofree char *vhost_path = NULL; > g_autofree char *tap = NULL; > g_autofree char *vhost = NULL; > + g_autofree char *switchid = NULL; > + g_autofree char *connectionid = NULL; > const char *prefix = xmlopt ? xmlopt->config.netPrefix : NULL; > > if (!(def = virDomainNetDefNew(xmlopt))) > @@ -8932,6 +8940,13 @@ virDomainNetDefParseXML(virDomainXMLOption *xmlopt, > portid = virXMLPropString(source_node, "portid"); > } > > + if (def->type == VIR_DOMAIN_NET_TYPE_VDS) { > + switchid = virXMLPropString(source_node, "switchid"); > + portid = virXMLPropString(source_node, "portid"); > + portgroup = virXMLPropString(source_node, "portgroupid"); > + connectionid = virXMLPropString(source_node, "connectionid"); > + } > + > if (def->type == VIR_DOMAIN_NET_TYPE_INTERNAL) > internal = virXMLPropString(source_node, "name"); > > @@ -9313,6 +9328,36 @@ virDomainNetDefParseXML(virDomainXMLOption *xmlopt, > } > break; > > + case VIR_DOMAIN_NET_TYPE_VDS: > + if (!switchid) { > + virReportError(VIR_ERR_XML_ERROR, > + _("Missing source switchid for interface type '%s'"), > + virDomainNetTypeToString(def->type)); > + goto error; > + } > + > + if (virUUIDParse(switchid, def->data.vds.switch_id) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Unable to parse switchid '%s'"), switchid); > + goto error; > + } Report if switch id is missing, which is good. > + > + if (virStrToLong_ll(portid, NULL, 0, &def->data.vds.port_id) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Unable to parse portid '%s'"), portid); > + goto error; > + } > + > + if (virStrToLong_ll(connectionid, NULL, 0, &def->data.vds.connection_id) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Unable to parse connectionid '%s'"), connectionid); > + goto error; > + } AFAICT, this may well crash if port id or connedtion id are missing, since it ends up passing NULL to strtol which is not defined to allow NULL IIUC. > + > + def->data.vds.portgroup_id = g_steal_pointer(&portgroup); Allows portgroup id to be missing > + > + break; > + > case VIR_DOMAIN_NET_TYPE_ETHERNET: > case VIR_DOMAIN_NET_TYPE_USER: > case VIR_DOMAIN_NET_TYPE_DUMMY: > @@ -9495,6 +9540,7 @@ virDomainNetDefParseXML(virDomainXMLOption *xmlopt, > case VIR_DOMAIN_NET_TYPE_HOSTDEV: > case VIR_DOMAIN_NET_TYPE_UDP: > case VIR_DOMAIN_NET_TYPE_DUMMY: > + case VIR_DOMAIN_NET_TYPE_VDS: > case VIR_DOMAIN_NET_TYPE_VDPA: > break; > case VIR_DOMAIN_NET_TYPE_LAST: > @@ -23683,7 +23729,21 @@ virDomainNetDefFormat(virBuffer *buf, > def->data.vdpa.devicepath); > sourceLines++; > } > + break; > + > + case VIR_DOMAIN_NET_TYPE_VDS: { > + char switchidstr[VIR_UUID_STRING_BUFLEN]; > + > + virUUIDFormat(def->data.vds.switch_id, switchidstr); > + virBufferEscapeString(buf, "<source switchid='%s'", switchidstr); > + virBufferAsprintf(buf, " portid='%lld'", def->data.vds.port_id); > + virBufferEscapeString(buf, " portgroupid='%s'", def->data.vds.portgroup_id); > + virBufferAsprintf(buf, " connectionid='%lld'", def->data.vds.connection_id); Based on this logic switch id, port id and connection id are mandatory, but missing portgroup_id would be handled gracefully. > + > + sourceLines++; > + > break; > + } > > case VIR_DOMAIN_NET_TYPE_USER: > case VIR_DOMAIN_NET_TYPE_DUMMY: > diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng > index 5d530f957b0d..7f6ea1d8887d 100644 > --- a/src/conf/schemas/domaincommon.rng > +++ b/src/conf/schemas/domaincommon.rng > @@ -3440,6 +3440,29 @@ > <ref name="interface-options"/> > </group> > > + <group> > + <attribute name="type"> > + <value>vds</value> > + </attribute> > + <interleave> > + <element name="source"> > + <attribute name="switchid"> > + <ref name="UUID"/> > + </attribute> > + <attribute name="portid"> > + <data type="long"/> > + </attribute> > + <attribute name="portgroupid"> > + <data type="string"/> > + </attribute> > + <attribute name="connectionid"> > + <data type="long"/> > + </attribute> Indicates all attributes are mandatory even though the code gracefully handles port group ID being missing. Overall there's some error checking missing when parsing I believe, or this needs relaxing for port group ID.

> + </element> > + <ref name="interface-options"/> > + </interleave> > + </group> > + > </choice> > <optional> > <attribute name="trustGuestRxFilters"> With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|