On Sat, 2016-05-14 at 16:00 -0400, Cole Robinson wrote:
> hotplug APIs with the AFFECT_CONFIG flag are essentially replicating
> 'insert <device> into XML document, and redefine XML'. Thinking of
> it this way, it's natural that we call virDomainDefPostParse after
> manually editing the XML here.
>
> Not only does doing so allow us to drop a bunch of open coded calls
> to qemuDomainAssignAddresses, but it also means we are going through
> the standard channels for XML validation and potentially catching
> errors in user submitted XML.
> ---
> src/qemu/qemu_driver.c | 71 ++++++++++++++++++++++++++------------------------
> 1 file changed, 37 insertions(+), 34 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index f47c620..c5fc069 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7775,10 +7775,12 @@ qemuDomainUpdateDeviceLive(virConnectPtr conn,
> }
>
> static int
> -qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps,
> - virDomainDefPtr vmdef,
> +qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef,
> virDomainDeviceDefPtr dev,
> - virConnectPtr conn)
> + virConnectPtr conn,
> + virCapsPtr caps,
> + unsigned int parse_flags,
s/parse_flags/parseFlags/g
> + virDomainXMLOptionPtr xmlopt)
> {
> virDomainDiskDefPtr disk;
> virDomainNetDefPtr net;
> @@ -7803,11 +7805,6 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps,
> return -1;
> /* vmdef has the pointer. Generic codes for vmdef will do all jobs */
> dev->data.disk = NULL;
> - if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO)
> - if (virDomainDefAddImplicitDevices(vmdef) < 0)
> - return -1;
You removed the check on disk->bus here, and that concerns me a
little. Can you please spend a few words explaining why this is
safe?
> - if (qemuDomainAssignAddresses(vmdef, qemuCaps, NULL) < 0)
> - return -1;
> break;
>
> case VIR_DOMAIN_DEVICE_NET:
> @@ -7815,8 +7812,6 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps,
> if (virDomainNetInsert(vmdef, net))
> return -1;
> dev->data.net = NULL;
> - if (qemuDomainAssignAddresses(vmdef, qemuCaps, NULL) < 0)
> - return -1;
> break;
>
> case VIR_DOMAIN_DEVICE_HOSTDEV:
> @@ -7829,10 +7824,6 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps,
> if (virDomainHostdevInsert(vmdef, hostdev))
> return -1;
> dev->data.hostdev = NULL;
> - if (virDomainDefAddImplicitDevices(vmdef) < 0)
> - return -1;
> - if (qemuDomainAssignAddresses(vmdef, qemuCaps, NULL) < 0)
> - return -1;
> break;
>
> case VIR_DOMAIN_DEVICE_LEASE:
> @@ -7863,18 +7854,12 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps,
> return -1;
> dev->data.controller = NULL;
>
> - if (qemuDomainAssignAddresses(vmdef, qemuCaps, NULL) < 0)
> - return -1;
> break;
>
> case VIR_DOMAIN_DEVICE_CHR:
> if (qemuDomainChrInsert(vmdef, dev->data.chr) < 0)
> return -1;
> dev->data.chr = NULL;
> - if (virDomainDefAddImplicitDevices(vmdef) < 0)
> - return -1;
> - if (qemuDomainAssignAddresses(vmdef, qemuCaps, NULL) < 0)
> - return -1;
> break;
>
> case VIR_DOMAIN_DEVICE_FS:
> @@ -7902,8 +7887,6 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps,
> return -1;
> dev->data.rng = NULL;
>
> - if (qemuDomainAssignAddresses(vmdef, qemuCaps, NULL) < 0)
> - return -1;
> break;
>
> case VIR_DOMAIN_DEVICE_MEMORY:
> @@ -7941,13 +7924,20 @@ qemuDomainAttachDeviceConfig(virQEMUCapsPtr qemuCaps,
> virDomainDeviceTypeToString(dev->type));
> return -1;
> }
> +
> + if (virDomainDefPostParse(vmdef, caps, parse_flags, xmlopt) < 0)
> + return -1;
> +
> return 0;
> }
>
>
> static int
> qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
> - virDomainDeviceDefPtr dev)
> + virDomainDeviceDefPtr dev,
> + virCapsPtr caps,
> + unsigned int parse_flags,
> + virDomainXMLOptionPtr xmlopt)
> {
> virDomainDiskDefPtr disk, det_disk;
> virDomainNetDefPtr net;
> @@ -8077,13 +8067,19 @@ qemuDomainDetachDeviceConfig(virDomainDefPtr vmdef,
> virDomainDeviceTypeToString(dev->type));
> return -1;
> }
> +
> + if (virDomainDefPostParse(vmdef, caps, parse_flags, xmlopt) < 0)
> + return -1;
> +
> return 0;
> }
>
> static int
> -qemuDomainUpdateDeviceConfig(virQEMUCapsPtr qemuCaps,
> - virDomainDefPtr vmdef,
> - virDomainDeviceDefPtr dev)
> +qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
> + virDomainDeviceDefPtr dev,
> + virCapsPtr caps,
> + unsigned int parse_flags,
> + virDomainXMLOptionPtr xmlopt)
> {
> virDomainDiskDefPtr orig, disk;
> virDomainGraphicsDefPtr newGraphics;
> @@ -8141,9 +8137,6 @@ qemuDomainUpdateDeviceConfig(virQEMUCapsPtr qemuCaps,
>
> vmdef->nets[pos] = net;
> dev->data.net = NULL;
> -
> - if (qemuDomainAssignAddresses(vmdef, qemuCaps, NULL) < 0)
> - return -1;
> break;
>
> case VIR_DOMAIN_DEVICE_FS:
> @@ -8172,6 +8165,10 @@ qemuDomainUpdateDeviceConfig(virQEMUCapsPtr qemuCaps,
> virDomainDeviceTypeToString(dev->type));
> return -1;
> }
> +
> + if (virDomainDefPostParse(vmdef, caps, parse_flags, xmlopt) < 0)
> + return -1;
> +
> return 0;
> }
>
> @@ -8247,8 +8244,9 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml,
> VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0)
> goto endjob;
>
> - if ((ret = qemuDomainAttachDeviceConfig(qemuCaps, vmdef, dev,
> - dom->conn)) < 0)
> + if ((ret = qemuDomainAttachDeviceConfig(vmdef, dev, dom->conn, caps,
> + parse_flags,
> + driver->xmlopt)) < 0)
> goto endjob;
> }
>
> @@ -8316,6 +8314,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
> qemuDomainObjPrivatePtr priv;
> virQEMUDriverConfigPtr cfg = NULL;
> virCapsPtr caps = NULL;
> + unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
>
> virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> VIR_DOMAIN_AFFECT_CONFIG |
> @@ -8341,7 +8340,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
>
> dev = dev_copy = virDomainDeviceDefParse(xml, vm->def,
> caps, driver->xmlopt,
> - VIR_DOMAIN_DEF_PARSE_INACTIVE);
> + parse_flags);
> if (dev == NULL)
> goto endjob;
>
> @@ -8374,7 +8373,9 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
> VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0)
> goto endjob;
>
> - if ((ret = qemuDomainUpdateDeviceConfig(qemuCaps, vmdef, dev)) < 0)
> + if ((ret = qemuDomainUpdateDeviceConfig(vmdef, dev, caps,
> + parse_flags,
> + driver->xmlopt)) < 0)
> goto endjob;
> }
>
> @@ -8494,7 +8495,9 @@ static int qemuDomainDetachDeviceFlags(virDomainPtr dom, const char *xml,
> VIR_DOMAIN_DEVICE_ACTION_DETACH) < 0)
> goto endjob;
>
> - if ((ret = qemuDomainDetachDeviceConfig(vmdef, dev)) < 0)
> + if ((ret = qemuDomainDetachDeviceConfig(vmdef, dev, caps,
> + parse_flags,
> + driver->xmlopt)) < 0)
> goto endjob;
> }
The rest looks reasonable, and the fact that this change
doesn't require altering the test suite in any way is
definitely reassuring.
--
Andrea Bolognani
Software Engineer - Virtualization Team
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list