Re: [libvirt] [PATCH 4/4] qemu: Call virDomainDefPostParse via CONFIG hotplug

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.