| Message ID | 19a4c6b74e52842fe283cd0bc0f6d3495c48158f.1463255562.git.crobinso@redhat.com |
|---|---|
| State | Accepted |
| Commit | 383833e230d0f2b92702181819588801380a7a60 |
| Headers | show |
On 05/17/2016 01:20 PM, Andrea Bolognani wrote: > 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? > I think the idea behind that check was 'adding a virtio disk doesn't need any implied controller, but bus=scsi might, so only call AddImplicit for non-virtio' However AddImplicit _must_ do the right thing here anyways, since for example it is called every time we parse the XML, like reading it from disk on libvirtd startup. So the check here was overly paranoid (but maybe it made sense once) >> - 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. Unfortunately the test suite may not cover this stuff, I didn't confirm. There is qemuhotplugtest.c but it's kind of complicated. Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 05/17/2016 01:20 PM, Andrea Bolognani wrote: > 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 Actually everywhere else in this file uses parse_flags, including other hotplug routines, so I kept it this way Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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, + 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; - 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; }