[libvirt] [PATCHv2 3/7] conf: introduce <vsock> element
Peter Krempa
pkrempa at redhat.com
Tue May 29 08:26:20 UTC 2018
On Thu, May 24, 2018 at 12:39:11 +0200, Ján Tomko wrote:
> Add a new 'vsock' element for the vsock device.
> The 'model' attribute is optional.
> A <source cid> subelement should be used to specify the guest cid,
> or <source auto='yes'/> should be used.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1291851
> ---
> docs/formatdomain.html.in | 20 +++
> docs/schemas/domaincommon.rng | 29 ++++
> src/conf/domain_conf.c | 195 +++++++++++++++++++++++++-
> src/conf/domain_conf.h | 17 +++
> src/qemu/qemu_domain.c | 1 +
> src/qemu/qemu_domain_address.c | 11 ++
> src/qemu/qemu_driver.c | 6 +
> src/qemu/qemu_hotplug.c | 1 +
> tests/qemuxml2argvdata/vhost-vsock-auto.xml | 35 +++++
> tests/qemuxml2argvdata/vhost-vsock.xml | 36 +++++
> tests/qemuxml2xmloutdata/vhost-vsock-auto.xml | 36 +++++
> tests/qemuxml2xmloutdata/vhost-vsock.xml | 1 +
> tests/qemuxml2xmltest.c | 3 +
> 13 files changed, 390 insertions(+), 1 deletion(-)
> create mode 100644 tests/qemuxml2argvdata/vhost-vsock-auto.xml
> create mode 100644 tests/qemuxml2argvdata/vhost-vsock.xml
> create mode 100644 tests/qemuxml2xmloutdata/vhost-vsock-auto.xml
> create mode 120000 tests/qemuxml2xmloutdata/vhost-vsock.xml
I've recently added a new swithc case which breaks with the following
message:
qemu/qemu_domain.c: In function 'qemuDomainDeviceDefPostParse':
qemu/qemu_domain.c:5802:5: error: enumeration value 'VIR_DOMAIN_DEVICE_VSOCK' not handled in switch [-Werror=switch-enum]
switch ((virDomainDeviceType) dev->type) {
^~~~~~
Apply the following patch:
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 527781c0fa..3e8e6358de 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5845,6 +5845,7 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
case VIR_DOMAIN_DEVICE_TPM:
case VIR_DOMAIN_DEVICE_MEMORY:
case VIR_DOMAIN_DEVICE_IOMMU:
+ case VIR_DOMAIN_DEVICE_VSOCK:
ret = 0;
break;
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 0d0fd3b9f3..bfe7f757b8 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -8133,6 +8133,26 @@ qemu-kvm -net nic,model=? /dev/null
> </dd>
> </dl>
>
> + <h3><a id="vsock">Vsock</a></h3>
> +
> + <p>A vsock host/guest interface. The <code>model</code> attribute
> + defaults to <code>virtio</code>.
> + The optional attribute <code>cid</code> of the source element
> + specifies the CID assigned to the guest. If the attribute
> + <code>auto</code> is set to <code>yes</code>, libvirt
> + will assign a free CID automatically on domain startup.
> + <span class="since">Since 4.4.0</span></p>
Should we perhaps mention some docs where users can figure out how to
use it?
Also please mention what the default value of 'auto' is. e.g. what the
following definition actually configures:
<vsock model='virtio'/>
> +
> +<pre>
> +...
> +<devices>
> + <vsock model='virtio'>
> + <source auto='no' cid='3'/>
> + </vsock>
> +</devices>
> +...</pre>
> +
> +
> <h3><a id="seclabel">Security label</a></h3>
>
> <p>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 71ac3d079c..3ea5c91773 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -4140,6 +4140,32 @@
> </optional>
> </define>
>
> + <define name="vsock">
> + <element name="vsock">
> + <optional>
> + <attribute name="model">
> + <value>virtio</value>
> + </attribute>
> + </optional>
> + <interleave>
> + <element name="source">
So, source is mandatory? It should be noted in the docs ...
> + <optional>
> + <attribute name="auto">
> + <ref name="virYesNo"/>
> + </attribute>
> + </optional>
> + <optional>
> + <attribute name="cid">
> + <ref name="unsignedInt"/>
> + </attribute>
> + </optional>
Also both attributes are optional which is kind of weird if <source> is
mandatory.
> + </element>
> + <optional>
> + <ref name="address"/>
> + </optional>
> + </interleave>
> + </element>
> + </define>
[...]
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index b2982fc3d4..1a3dbf884b 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
[...]
> @@ -15878,6 +15911,68 @@ virDomainIOMMUDefParseXML(xmlNodePtr node,
> }
>
>
> +static virDomainVsockDefPtr
> +virDomainVsockDefParseXML(virDomainXMLOptionPtr xmlopt,
> + xmlNodePtr node,
> + xmlXPathContextPtr ctxt,
> + unsigned int flags)
> +{
> + virDomainVsockDefPtr vsock = NULL, ret = NULL;
> + xmlNodePtr save = ctxt->node;
> + xmlNodePtr source;
> + char *tmp = NULL;
> + int val;
> +
> + ctxt->node = node;
> +
> + if (!(vsock = virDomainVsockDefNew(xmlopt)))
> + goto cleanup;
> +
> + if ((tmp = virXMLPropString(node, "model"))) {
> + if ((val = virDomainVsockModelTypeFromString(tmp)) < 0) {
Model 'default' should not be allowed.
> + virReportError(VIR_ERR_XML_ERROR, _("unknown vsock model: %s"), tmp);
> + goto cleanup;
> + }
> + vsock->model = val;
> + }
> +
> + source = virXPathNode("./source", ctxt);
> +
> + VIR_FREE(tmp);
> + if (source && (tmp = virXMLPropString(source, "cid"))) {
'source' is the common condition in both if the statements below, so
perhaps it can be extracted one level up.
> + if (virStrToLong_uip(tmp, NULL, 10, &vsock->guest_cid) < 0 ||
> + vsock->guest_cid == 0) {
> + virReportError(VIR_ERR_XML_DETAIL,
> + _("'cid' attribute must be a positive number: %s"),
> + tmp);
> + goto cleanup;
> + }
> + }
> +
> + VIR_FREE(tmp);
> + if (source && (tmp = virXMLPropString(source, "auto"))) {
> + val = virTristateBoolTypeFromString(tmp);
> + if (val < 0) {
the "default" value should not be allowed ...
> + virReportError(VIR_ERR_XML_DETAIL,
> + _("'auto' attribute can be 'yes' or 'no': %s"),
... specifically given this error message.
> + tmp);
> + goto cleanup;
> + }
> + vsock->auto_cid = val;
> + }
> +
> + if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, &vsock->info, flags) < 0)
> + goto cleanup;
> +
> + VIR_STEAL_PTR(ret, vsock);
> +
> + cleanup:
> + ctxt->node = save;
> + VIR_FREE(vsock);
> + VIR_FREE(tmp);
> + return ret;
> +}
[...]
> @@ -28595,6 +28787,7 @@ virDomainDeviceDefCopy(virDomainDeviceDefPtr src,
> case VIR_DOMAIN_DEVICE_MEMBALLOON:
> case VIR_DOMAIN_DEVICE_NVRAM:
> case VIR_DOMAIN_DEVICE_IOMMU:
> + case VIR_DOMAIN_DEVICE_VSOCK:
This should be very easy to implement.
[...]
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index b35594be5f..e429f54a43 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -4626,6 +4626,7 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver,
> case VIR_DOMAIN_DEVICE_TPM:
> case VIR_DOMAIN_DEVICE_PANIC:
> case VIR_DOMAIN_DEVICE_IOMMU:
> + case VIR_DOMAIN_DEVICE_VSOCK:
Note that I've did not look through the impl patch yet, so the below
comment may be actually moot.
At least this should be implemented as you can trigger a device detach
from the guest as well. Since this does not have any backend data, the
removal function is trivial.
> case VIR_DOMAIN_DEVICE_LAST:
> virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> _("don't know how to remove a %s device"),
> diff --git a/tests/qemuxml2argvdata/vhost-vsock-auto.xml b/tests/qemuxml2argvdata/vhost-vsock-auto.xml
> new file mode 100644
> index 0000000000..729d58bea4
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/vhost-vsock-auto.xml
> @@ -0,0 +1,35 @@
> +<domain type='qemu'>
> + <name>test</name>
> + <uuid>bba65c0e-c049-934f-b6aa-4e2c0582acdf</uuid>
> + <memory unit='KiB'>1048576</memory>
> + <currentMemory unit='KiB'>1048576</currentMemory>
> + <vcpu placement='static'>1</vcpu>
> + <os>
> + <type arch='x86_64' machine='pc-0.13'>hvm</type>
Our XMLs are living in the past.
> + <boot dev='hd'/>
> + <bootmenu enable='yes'/>
For an ACK please state how you plan to deal with the schema issues I've
pointed out. All other necessary changes were pointed out.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180529/16020f61/attachment-0001.sig>
More information about the libvir-list
mailing list