[libvirt] [PATCH 0/9] Couple of migration fixes
Looks like nobody tried migrations lately. I just did and found interesting bug. On the destination qemu binary was at different path. So I've dumped domain XML I was trying ti migrate and just changed the emulator path. All of a sudden I observed plenty of errors. Problem is that whilst parsing the user provided migration XML (and doing other operations on it), because of parse callbacks we need qemuCaps for the binary. However, we just take the def->emulator and expect it to exist. Well, it doesn't. Michal Privoznik (9): conf: Introduce virDomainDefPostParseOpaque conf: Introduce virDomainDefParseStringOpaque qemuDomainDefPostParse: Fetch qemuCaps from domain object conf: Extend virDomainDeviceDefPostParse for parseOpaque qemuDomainDeviceDefPostParse: Fetch caps from domain object conf: Extend virDomainDefAssignAddressesCallback for parseOpaque qemuDomainDefAssignAddresses: Fetch caps from domain object domain_conf: Introduce VIR_DOMAIN_DEF_PARSE_SKIP_POST_PARSE conf: Skip post parse callbacks when creating copy src/bhyve/bhyve_domain.c | 6 ++-- src/conf/domain_conf.c | 80 +++++++++++++++++++++++++++++++++++++--------- src/conf/domain_conf.h | 43 +++++++++++++++++++++---- src/libvirt_private.syms | 3 ++ src/libxl/libxl_domain.c | 6 ++-- src/lxc/lxc_domain.c | 6 ++-- src/openvz/openvz_driver.c | 6 ++-- src/phyp/phyp_driver.c | 6 ++-- src/qemu/qemu_domain.c | 50 ++++++++++++++++++++++------- src/qemu/qemu_migration.c | 6 ++-- src/uml/uml_driver.c | 6 ++-- src/vbox/vbox_common.c | 6 ++-- src/vmware/vmware_driver.c | 6 ++-- src/vmx/vmx.c | 6 ++-- src/vz/vz_driver.c | 6 ++-- src/xen/xen_driver.c | 6 ++-- src/xenapi/xenapi_driver.c | 6 ++-- 17 files changed, 195 insertions(+), 59 deletions(-) -- 2.8.4
Some callers might want to pass yet another pointer to opaque data to post parse callbacks. The driver generic one is not enough because two threads executing post parse callback might want see different data (e.g. domain object pointer that domain def belongs to). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/bhyve/bhyve_domain.c | 3 ++- src/conf/domain_conf.c | 13 ++++++++++++- src/conf/domain_conf.h | 14 ++++++++++++-- src/libvirt_private.syms | 1 + src/libxl/libxl_domain.c | 3 ++- src/lxc/lxc_domain.c | 3 ++- src/openvz/openvz_driver.c | 3 ++- src/phyp/phyp_driver.c | 3 ++- src/qemu/qemu_domain.c | 3 ++- src/uml/uml_driver.c | 3 ++- src/vbox/vbox_common.c | 3 ++- src/vmware/vmware_driver.c | 3 ++- src/vmx/vmx.c | 3 ++- src/vz/vz_driver.c | 3 ++- src/xen/xen_driver.c | 3 ++- src/xenapi/xenapi_driver.c | 3 ++- 16 files changed, 51 insertions(+), 16 deletions(-) diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c index 89cb171..3273462 100644 --- a/src/bhyve/bhyve_domain.c +++ b/src/bhyve/bhyve_domain.c @@ -61,7 +61,8 @@ static int bhyveDomainDefPostParse(virDomainDefPtr def, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) + void *opaque ATTRIBUTE_UNUSED, + void *parseOpaque ATTRIBUTE_UNUSED) { /* Add an implicit PCI root controller */ if (virDomainDefMaybeAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0, diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b4a3e92..e8c8465 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4532,6 +4532,16 @@ virDomainDefPostParse(virDomainDefPtr def, unsigned int parseFlags, virDomainXMLOptionPtr xmlopt) { + return virDomainDefPostParseOpaque(def, caps, parseFlags, xmlopt, NULL); +} + +int +virDomainDefPostParseOpaque(virDomainDefPtr def, + virCapsPtr caps, + unsigned int parseFlags, + virDomainXMLOptionPtr xmlopt, + void *parseOpaque) +{ int ret; struct virDomainDefPostParseDeviceIteratorData data = { .caps = caps, @@ -4547,7 +4557,8 @@ virDomainDefPostParse(virDomainDefPtr def, /* call the domain config callback */ if (xmlopt->config.domainPostParseCallback) { ret = xmlopt->config.domainPostParseCallback(def, caps, parseFlags, - xmlopt->config.priv); + xmlopt->config.priv, + parseOpaque); if (ret < 0) return ret; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index c14a39c..6056aa7 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2361,11 +2361,15 @@ typedef struct _virDomainXMLOption virDomainXMLOption; typedef virDomainXMLOption *virDomainXMLOptionPtr; /* Called once after everything else has been parsed, for adjusting - * overall domain defaults. */ + * overall domain defaults. + * @parseOpaque is opaque data passed by virDomainDefParse* caller, + * @opqaue is opaque data set by driver (usually pointer to driver + * private data). */ typedef int (*virDomainDefPostParseCallback)(virDomainDefPtr def, virCapsPtr caps, unsigned int parseFlags, - void *opaque); + void *opaque, + void *parseOpaque); /* Called once per device, for adjusting per-device settings while * leaving the overall domain otherwise unchanged. */ typedef int (*virDomainDeviceDefPostParseCallback)(virDomainDeviceDefPtr dev, @@ -2453,6 +2457,12 @@ virDomainDefPostParse(virDomainDefPtr def, unsigned int parseFlags, virDomainXMLOptionPtr xmlopt); +int virDomainDefPostParseOpaque(virDomainDefPtr def, + virCapsPtr caps, + unsigned int parseFlags, + virDomainXMLOptionPtr xmlopt, + void *parseOpaque); + int virDomainDefValidate(virDomainDefPtr def, virCapsPtr caps, unsigned int parseFlags, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9610e0c..5fd1178 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -250,6 +250,7 @@ virDomainDefParseFile; virDomainDefParseNode; virDomainDefParseString; virDomainDefPostParse; +virDomainDefPostParseOpaque; virDomainDefSetMemoryTotal; virDomainDefSetVcpus; virDomainDefSetVcpusMax; diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 43f4a7f..a53b977 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -380,7 +380,8 @@ static int libxlDomainDefPostParse(virDomainDefPtr def, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) + void *opaque ATTRIBUTE_UNUSED, + void *parseOpaque ATTRIBUTE_UNUSED) { /* Xen PV domains always have a PV console, so add one to the domain config * via post-parse callback if not explicitly specified in the XML. */ diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c index 60db229..c2e362a 100644 --- a/src/lxc/lxc_domain.c +++ b/src/lxc/lxc_domain.c @@ -364,7 +364,8 @@ static int virLXCDomainDefPostParse(virDomainDefPtr def, virCapsPtr caps, unsigned int parseFlags ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) + void *opaque ATTRIBUTE_UNUSED, + void *parseOpaque ATTRIBUTE_UNUSED) { /* check for emulator and create a default one if needed */ if (!def->emulator && diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 6c3edc9..e1c008c 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -92,7 +92,8 @@ static int openvzDomainDefPostParse(virDomainDefPtr def, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) + void *opaque ATTRIBUTE_UNUSED, + void *parseOpaque ATTRIBUTE_UNUSED) { /* fill the init path */ if (def->os.type == VIR_DOMAIN_OSTYPE_EXE && !def->os.init) { diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 3dd8927..2c1a854 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -1097,7 +1097,8 @@ static int phypDomainDefPostParse(virDomainDefPtr def ATTRIBUTE_UNUSED, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) + void *opaque ATTRIBUTE_UNUSED, + void *parseOpaque ATTRIBUTE_UNUSED) { return 0; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3f16dbe..c26da14 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2326,7 +2326,8 @@ static int qemuDomainDefPostParse(virDomainDefPtr def, virCapsPtr caps, unsigned int parseFlags, - void *opaque) + void *opaque, + void *parseOpaque ATTRIBUTE_UNUSED) { virQEMUDriverPtr driver = opaque; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 4f25f76..90d2eae 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -435,7 +435,8 @@ static int umlDomainDefPostParse(virDomainDefPtr def ATTRIBUTE_UNUSED, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) + void *opaque ATTRIBUTE_UNUSED, + void *parseOpaque ATTRIBUTE_UNUSED) { return 0; } diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 3624a5e..dfade01 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -255,7 +255,8 @@ static int vboxDomainDefPostParse(virDomainDefPtr def ATTRIBUTE_UNUSED, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) + void *opaque ATTRIBUTE_UNUSED, + void *parseOpaque ATTRIBUTE_UNUSED) { return 0; } diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index eea2d1b..d44b8a3 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -86,7 +86,8 @@ static int vmwareDomainDefPostParse(virDomainDefPtr def ATTRIBUTE_UNUSED, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) + void *opaque ATTRIBUTE_UNUSED, + void *parseOpaque ATTRIBUTE_UNUSED) { return 0; } diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index 4a08b0f..ce0b155 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -528,7 +528,8 @@ static int virVMXDomainDefPostParse(virDomainDefPtr def ATTRIBUTE_UNUSED, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) + void *opaque ATTRIBUTE_UNUSED, + void *parseOpaque ATTRIBUTE_UNUSED) { return 0; } diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 819dad7..2f2be122 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -263,7 +263,8 @@ static int vzDomainDefPostParse(virDomainDefPtr def, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) + void *opaque ATTRIBUTE_UNUSED, + void *parseOpaque ATTRIBUTE_UNUSED) { if (vzDomainDefAddDefaultInputDevices(def) < 0) return -1; diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 2bab61e..5c38bdf 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -370,7 +370,8 @@ static int xenDomainDefPostParse(virDomainDefPtr def, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) + void *opaque ATTRIBUTE_UNUSED, + void *parseOpaque ATTRIBUTE_UNUSED) { if (!def->memballoon) { virDomainMemballoonDefPtr memballoon; diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index 9a861c1..a58c10e 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -75,7 +75,8 @@ static int xenapiDomainDefPostParse(virDomainDefPtr def, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) + void *opaque ATTRIBUTE_UNUSED, + void *parseOpaque ATTRIBUTE_UNUSED) { /* add implicit input device */ if (xenDomainDefAddImplicitInputDevice(def) < 0) -- 2.8.4
This function allows callers to pass arbitrary pointer to domain def parse callback. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 37 ++++++++++++++++++++++++++++++------- src/conf/domain_conf.h | 11 +++++++++++ src/libvirt_private.syms | 2 ++ 3 files changed, 43 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index e8c8465..91cd2ab 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15881,6 +15881,7 @@ virDomainDefParseXML(xmlDocPtr xml, xmlXPathContextPtr ctxt, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, + void *parseOpaque, unsigned int flags) { xmlNodePtr *nodes = NULL, node = NULL; @@ -17532,7 +17533,7 @@ virDomainDefParseXML(xmlDocPtr xml, goto error; /* callback to fill driver specific domain aspects */ - if (virDomainDefPostParse(def, caps, flags, xmlopt) < 0) + if (virDomainDefPostParseOpaque(def, caps, flags, xmlopt, parseOpaque) < 0) goto error; /* valdiate configuration */ @@ -17581,7 +17582,7 @@ virDomainObjParseXML(xmlDocPtr xml, oldnode = ctxt->node; ctxt->node = config; - obj->def = virDomainDefParseXML(xml, config, ctxt, caps, xmlopt, flags); + obj->def = virDomainDefParseXML(xml, config, ctxt, caps, xmlopt, NULL, flags); ctxt->node = oldnode; if (!obj->def) goto error; @@ -17654,6 +17655,7 @@ virDomainDefParse(const char *xmlStr, const char *filename, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, + void *parseOpaque, unsigned int flags) { xmlDocPtr xml; @@ -17661,8 +17663,8 @@ virDomainDefParse(const char *xmlStr, int keepBlanksDefault = xmlKeepBlanksDefault(0); if ((xml = virXMLParse(filename, xmlStr, _("(domain_definition)")))) { - def = virDomainDefParseNode(xml, xmlDocGetRootElement(xml), caps, - xmlopt, flags); + def = virDomainDefParseNodeOpaque(xml, xmlDocGetRootElement(xml), caps, + xmlopt, parseOpaque, flags); xmlFreeDoc(xml); } @@ -17676,7 +17678,17 @@ virDomainDefParseString(const char *xmlStr, virDomainXMLOptionPtr xmlopt, unsigned int flags) { - return virDomainDefParse(xmlStr, NULL, caps, xmlopt, flags); + return virDomainDefParse(xmlStr, NULL, caps, xmlopt, NULL, flags); +} + +virDomainDefPtr +virDomainDefParseStringOpaque(const char *xmlStr, + virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + void *parseOpaque, + unsigned int flags) +{ + return virDomainDefParse(xmlStr, NULL, caps, xmlopt, parseOpaque, flags); } virDomainDefPtr @@ -17685,7 +17697,7 @@ virDomainDefParseFile(const char *filename, virDomainXMLOptionPtr xmlopt, unsigned int flags) { - return virDomainDefParse(NULL, filename, caps, xmlopt, flags); + return virDomainDefParse(NULL, filename, caps, xmlopt, NULL, flags); } @@ -17696,6 +17708,17 @@ virDomainDefParseNode(xmlDocPtr xml, virDomainXMLOptionPtr xmlopt, unsigned int flags) { + return virDomainDefParseNodeOpaque(xml, root, caps, xmlopt, NULL, flags); +} + +virDomainDefPtr +virDomainDefParseNodeOpaque(xmlDocPtr xml, + xmlNodePtr root, + virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + void *parseOpaque, + unsigned int flags) +{ xmlXPathContextPtr ctxt = NULL; virDomainDefPtr def = NULL; @@ -17714,7 +17737,7 @@ virDomainDefParseNode(xmlDocPtr xml, } ctxt->node = root; - def = virDomainDefParseXML(xml, root, ctxt, caps, xmlopt, flags); + def = virDomainDefParseXML(xml, root, ctxt, caps, xmlopt, parseOpaque, flags); cleanup: xmlXPathFreeContext(ctxt); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 6056aa7..f132153 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2679,6 +2679,11 @@ virDomainDefPtr virDomainDefParseString(const char *xmlStr, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, unsigned int flags); +virDomainDefPtr virDomainDefParseStringOpaque(const char *xmlStr, + virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + void *parseOpaque, + unsigned int flags); virDomainDefPtr virDomainDefParseFile(const char *filename, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, @@ -2688,6 +2693,12 @@ virDomainDefPtr virDomainDefParseNode(xmlDocPtr doc, virCapsPtr caps, virDomainXMLOptionPtr xmlopt, unsigned int flags); +virDomainDefPtr virDomainDefParseNodeOpaque(xmlDocPtr xml, + xmlNodePtr root, + virCapsPtr caps, + virDomainXMLOptionPtr xmlopt, + void *parseOpaque, + unsigned int flags); virDomainObjPtr virDomainObjParseNode(xmlDocPtr xml, xmlNodePtr root, virCapsPtr caps, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5fd1178..d7cdfda 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -248,7 +248,9 @@ virDomainDefNew; virDomainDefNewFull; virDomainDefParseFile; virDomainDefParseNode; +virDomainDefParseNodeOpaque; virDomainDefParseString; +virDomainDefParseStringOpaque; virDomainDefPostParse; virDomainDefPostParseOpaque; virDomainDefSetMemoryTotal; -- 2.8.4
We can't rely on def->emulator path. It may be provided by user as we give them opportunity to provide their own XML for migration. Therefore the path may point to just whatever binary (or even to a non-existent file). Moreover, this path is meant for destination, but the capabilities lookup is done on source. What we can do is to assume same capabilities for post parse callbacks as the running domain has. They will be used just to add some default models/controllers/devices/... anyway. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c26da14..8e2c9e9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2327,11 +2327,13 @@ qemuDomainDefPostParse(virDomainDefPtr def, virCapsPtr caps, unsigned int parseFlags, void *opaque, - void *parseOpaque ATTRIBUTE_UNUSED) + void *parseOpaque) { virQEMUDriverPtr driver = opaque; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virQEMUCapsPtr qemuCaps = NULL; + virDomainObjPtr vm = parseOpaque; + qemuDomainObjPrivatePtr priv; int ret = -1; if (def->os.bootloader || def->os.bootloaderArgs) { @@ -2360,9 +2362,15 @@ qemuDomainDefPostParse(virDomainDefPtr def, !(def->emulator = virDomainDefGetDefaultEmulator(def, caps))) goto cleanup; - if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, - def->emulator))) - goto cleanup; + if (vm) { + priv = vm->privateData; + qemuCaps = priv->qemuCaps; + virObjectRef(qemuCaps); + } else { + if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, + def->emulator))) + goto cleanup; + } if (qemuDomainDefAddDefaultDevices(def, qemuCaps) < 0) goto cleanup; -- 2.8.4
Just like virDomainDefPostParseCallback has gained new parseOpaque argument, we need to follow the logic with virDomainDeviceDefPostParse. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/bhyve/bhyve_domain.c | 3 ++- src/conf/domain_conf.c | 13 +++++++++---- src/conf/domain_conf.h | 8 ++++++-- src/libxl/libxl_domain.c | 3 ++- src/lxc/lxc_domain.c | 3 ++- src/openvz/openvz_driver.c | 3 ++- src/phyp/phyp_driver.c | 3 ++- src/qemu/qemu_domain.c | 3 ++- src/qemu/qemu_migration.c | 6 +++--- src/uml/uml_driver.c | 3 ++- src/vbox/vbox_common.c | 3 ++- src/vmware/vmware_driver.c | 3 ++- src/vmx/vmx.c | 3 ++- src/vz/vz_driver.c | 3 ++- src/xen/xen_driver.c | 3 ++- src/xenapi/xenapi_driver.c | 3 ++- 16 files changed, 44 insertions(+), 22 deletions(-) diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c index 3273462..e2a20ce 100644 --- a/src/bhyve/bhyve_domain.c +++ b/src/bhyve/bhyve_domain.c @@ -77,7 +77,8 @@ bhyveDomainDeviceDefPostParse(virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED, const virDomainDef *def ATTRIBUTE_UNUSED, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) + void *opaque ATTRIBUTE_UNUSED, + void *parseOpaque ATTRIBUTE_UNUSED) { return 0; } diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 91cd2ab..a4bc33f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4391,13 +4391,15 @@ virDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, const virDomainDef *def, virCapsPtr caps, unsigned int flags, - virDomainXMLOptionPtr xmlopt) + virDomainXMLOptionPtr xmlopt, + void *parseOpaque) { int ret; if (xmlopt->config.devicesPostParseCallback) { ret = xmlopt->config.devicesPostParseCallback(dev, def, caps, flags, - xmlopt->config.priv); + xmlopt->config.priv, + parseOpaque); if (ret < 0) return ret; } @@ -4415,6 +4417,7 @@ virDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, struct virDomainDefPostParseDeviceIteratorData { virCapsPtr caps; virDomainXMLOptionPtr xmlopt; + void *parseOpaque; unsigned int parseFlags; }; @@ -4427,7 +4430,8 @@ virDomainDefPostParseDeviceIterator(virDomainDefPtr def, { struct virDomainDefPostParseDeviceIteratorData *data = opaque; return virDomainDeviceDefPostParse(dev, def, data->caps, - data->parseFlags, data->xmlopt); + data->parseFlags, data->xmlopt, + data->parseOpaque); } @@ -4546,6 +4550,7 @@ virDomainDefPostParseOpaque(virDomainDefPtr def, struct virDomainDefPostParseDeviceIteratorData data = { .caps = caps, .xmlopt = xmlopt, + .parseOpaque = parseOpaque, .parseFlags = parseFlags, }; @@ -13536,7 +13541,7 @@ virDomainDeviceDefParse(const char *xmlStr, } /* callback to fill driver specific device aspects */ - if (virDomainDeviceDefPostParse(dev, def, caps, flags, xmlopt) < 0) + if (virDomainDeviceDefPostParse(dev, def, caps, flags, xmlopt, NULL) < 0) goto error; /* validate the configuration */ diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f132153..17f1edc 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2371,12 +2371,16 @@ typedef int (*virDomainDefPostParseCallback)(virDomainDefPtr def, void *opaque, void *parseOpaque); /* Called once per device, for adjusting per-device settings while - * leaving the overall domain otherwise unchanged. */ + * leaving the overall domain otherwise unchanged. + * @parseOpaque is opaque data passed by virDomainDefParse* caller, + * @opqaue is opaque data set by driver (usually pointer to driver + * private data). */ typedef int (*virDomainDeviceDefPostParseCallback)(virDomainDeviceDefPtr dev, const virDomainDef *def, virCapsPtr caps, unsigned int parseFlags, - void *opaque); + void *opaque, + void *parseOpaque); /* Drive callback for assigning device addresses, called at the end * of parsing, after all defaults and implicit devices have been added. */ typedef int (*virDomainDefAssignAddressesCallback)(virDomainDef *def, diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index a53b977..09d8684 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -285,7 +285,8 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, const virDomainDef *def, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) + void *opaque ATTRIBUTE_UNUSED, + void *parseOpaque ATTRIBUTE_UNUSED) { if (dev->type == VIR_DOMAIN_DEVICE_CHR && dev->data.chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c index c2e362a..9027c25 100644 --- a/src/lxc/lxc_domain.c +++ b/src/lxc/lxc_domain.c @@ -381,7 +381,8 @@ virLXCDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, const virDomainDef *def ATTRIBUTE_UNUSED, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) + void *opaque ATTRIBUTE_UNUSED, + void *parseOpaque ATTRIBUTE_UNUSED) { if (dev->type == VIR_DOMAIN_DEVICE_CHR && dev->data.chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index e1c008c..5878a4c 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -110,7 +110,8 @@ openvzDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, const virDomainDef *def ATTRIBUTE_UNUSED, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) + void *opaque ATTRIBUTE_UNUSED, + void *parseOpaque ATTRIBUTE_UNUSED) { if (dev->type == VIR_DOMAIN_DEVICE_CHR && dev->data.chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 2c1a854..6bac848 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -1109,7 +1109,8 @@ phypDomainDeviceDefPostParse(virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED, const virDomainDef *def ATTRIBUTE_UNUSED, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) + void *opaque ATTRIBUTE_UNUSED, + void *parseOpaque ATTRIBUTE_UNUSED) { return 0; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8e2c9e9..034ab73 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2563,7 +2563,8 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, const virDomainDef *def, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags, - void *opaque) + void *opaque, + void *parseOpaque ATTRIBUTE_UNUSED) { virQEMUDriverPtr driver = opaque; virQEMUCapsPtr qemuCaps = NULL; diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index e734816..968c240 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3233,9 +3233,9 @@ qemuMigrationBeginPhase(virQEMUDriverPtr driver, } if (xmlin) { - if (!(def = virDomainDefParseString(xmlin, caps, driver->xmlopt, - VIR_DOMAIN_DEF_PARSE_INACTIVE | - VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) + if (!(def = virDomainDefParseStringOpaque(xmlin, caps, driver->xmlopt, vm, + VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) goto cleanup; if (!qemuDomainDefCheckABIStability(driver, vm->def, def)) diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 90d2eae..abcd7c9 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -410,7 +410,8 @@ umlDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, const virDomainDef *def ATTRIBUTE_UNUSED, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) + void *opaque ATTRIBUTE_UNUSED, + void *parseOpaque ATTRIBUTE_UNUSED) { if (dev->type == VIR_DOMAIN_DEVICE_CHR && dev->data.chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index dfade01..4a8987f 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -266,7 +266,8 @@ vboxDomainDeviceDefPostParse(virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED, const virDomainDef *def ATTRIBUTE_UNUSED, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) + void *opaque ATTRIBUTE_UNUSED, + void *parseOpaque ATTRIBUTE_UNUSED) { return 0; } diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index d44b8a3..4e9afc0 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -97,7 +97,8 @@ vmwareDomainDeviceDefPostParse(virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED, const virDomainDef *def ATTRIBUTE_UNUSED, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) + void *opaque ATTRIBUTE_UNUSED, + void *parseOpaque ATTRIBUTE_UNUSED) { return 0; } diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c index ce0b155..994a9a0 100644 --- a/src/vmx/vmx.c +++ b/src/vmx/vmx.c @@ -539,7 +539,8 @@ virVMXDomainDevicesDefPostParse(virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED, const virDomainDef *def ATTRIBUTE_UNUSED, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) + void *opaque ATTRIBUTE_UNUSED, + void *parseOpaque ATTRIBUTE_UNUSED) { return 0; } diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index 2f2be122..1bafc89 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -288,7 +288,8 @@ vzDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, const virDomainDef *def, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) + void *opaque ATTRIBUTE_UNUSED, + void *parseOpaque ATTRIBUTE_UNUSED) { if (dev->type == VIR_DOMAIN_DEVICE_NET && (dev->data.net->type == VIR_DOMAIN_NET_TYPE_NETWORK || diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 5c38bdf..0d484a3 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -325,7 +325,8 @@ xenDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, const virDomainDef *def, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) + void *opaque ATTRIBUTE_UNUSED, + void *parseOpaque ATTRIBUTE_UNUSED) { if (dev->type == VIR_DOMAIN_DEVICE_CHR && dev->data.chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && diff --git a/src/xenapi/xenapi_driver.c b/src/xenapi/xenapi_driver.c index a58c10e..aacc89b 100644 --- a/src/xenapi/xenapi_driver.c +++ b/src/xenapi/xenapi_driver.c @@ -49,7 +49,8 @@ xenapiDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, const virDomainDef *def, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, - void *opaque ATTRIBUTE_UNUSED) + void *opaque ATTRIBUTE_UNUSED, + void *parseOpaque ATTRIBUTE_UNUSED) { if (dev->type == VIR_DOMAIN_DEVICE_CHR && dev->data.chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && -- 2.8.4
Just like we did two commits ago, don't try to fetch capabilities for non-existing binary. Re-use the ones we have for running domain. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 034ab73..36c0f29 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2564,14 +2564,22 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags, void *opaque, - void *parseOpaque ATTRIBUTE_UNUSED) + void *parseOpaque) { virQEMUDriverPtr driver = opaque; virQEMUCapsPtr qemuCaps = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + virDomainObjPtr vm = parseOpaque; + qemuDomainObjPrivatePtr priv; int ret = -1; - qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, def->emulator); + if (vm) { + priv = vm->privateData; + qemuCaps = priv->qemuCaps; + virObjectRef(qemuCaps); + } else { + qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, def->emulator); + } if (dev->type == VIR_DOMAIN_DEVICE_NET && dev->data.net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV && -- 2.8.4
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 3 ++- src/conf/domain_conf.h | 8 ++++++-- src/qemu/qemu_domain.c | 3 ++- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a4bc33f..89a3f62 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4581,7 +4581,8 @@ virDomainDefPostParseOpaque(virDomainDefPtr def, if (xmlopt->config.assignAddressesCallback) { ret = xmlopt->config.assignAddressesCallback(def, caps, parseFlags, - xmlopt->config.priv); + xmlopt->config.priv, + parseOpaque); if (ret < 0) return ret; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 17f1edc..17ade4b 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2382,11 +2382,15 @@ typedef int (*virDomainDeviceDefPostParseCallback)(virDomainDeviceDefPtr dev, void *opaque, void *parseOpaque); /* Drive callback for assigning device addresses, called at the end - * of parsing, after all defaults and implicit devices have been added. */ + * of parsing, after all defaults and implicit devices have been added. + * @parseOpaque is opaque data passed by virDomainDefParse* caller, + * @opqaue is opaque data set by driver (usually pointer to driver + * private data). */ typedef int (*virDomainDefAssignAddressesCallback)(virDomainDef *def, virCapsPtr caps, unsigned int parseFlags, - void *opaque); + void *opaque, + void *parseOpaque); /* Called in appropriate places where the domain conf parser can return failure * for configurations that were previously accepted. This shall not modify the diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 36c0f29..81cb87a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2778,7 +2778,8 @@ static int qemuDomainDefAssignAddresses(virDomainDef *def, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, - void *opaque) + void *opaque, + void *parseOpaque ATTRIBUTE_UNUSED) { virQEMUDriverPtr driver = opaque; virQEMUCapsPtr qemuCaps = NULL; -- 2.8.4
Just like we did two commits ago, don't try to fetch capabilities for non-existing binary. Re-use the ones we have for running domain. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 81cb87a..d3af622 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2779,16 +2779,24 @@ qemuDomainDefAssignAddresses(virDomainDef *def, virCapsPtr caps ATTRIBUTE_UNUSED, unsigned int parseFlags ATTRIBUTE_UNUSED, void *opaque, - void *parseOpaque ATTRIBUTE_UNUSED) + void *parseOpaque) { virQEMUDriverPtr driver = opaque; + virDomainObjPtr vm = parseOpaque; + qemuDomainObjPrivatePtr priv; virQEMUCapsPtr qemuCaps = NULL; int ret = -1; bool newDomain = parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE; - if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, - def->emulator))) - goto cleanup; + if (vm) { + priv = vm->privateData; + qemuCaps = priv->qemuCaps; + virObjectRef(qemuCaps); + } else { + if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, + def->emulator))) + goto cleanup; + } if (qemuDomainAssignAddresses(def, qemuCaps, NULL, newDomain) < 0) goto cleanup; -- 2.8.4
This is an internal flag that prevents our two entry points to XML parsing (virDomainDefParse and virDomainDeviceDefParse) from running post parse callbacks. This is expected to be used in cases when we already have full domain/device XML and we are just parsing it back (i.e. virDomainDefCopy or virDomainDeviceDefCopy) Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 8 ++++++++ src/conf/domain_conf.h | 2 ++ 2 files changed, 10 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 89a3f62..7e4aba6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4396,6 +4396,10 @@ virDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, { int ret; + /* fill in configuration only in certain places */ + if (flags & VIR_DOMAIN_DEF_PARSE_SKIP_POST_PARSE) + return 0; + if (xmlopt->config.devicesPostParseCallback) { ret = xmlopt->config.devicesPostParseCallback(dev, def, caps, flags, xmlopt->config.priv, @@ -4554,6 +4558,10 @@ virDomainDefPostParseOpaque(virDomainDefPtr def, .parseFlags = parseFlags, }; + /* fill in configuration only in certain places */ + if (parseFlags & VIR_DOMAIN_DEF_PARSE_SKIP_POST_PARSE) + return 0; + /* this must be done before the hypervisor-specific callback, * in case presence of a controller at a specific index is checked */ diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 17ade4b..7b097ab 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2647,6 +2647,8 @@ typedef enum { VIR_DOMAIN_DEF_PARSE_ABI_UPDATE = 1 << 9, /* skip definition validation checks meant to be executed on define time only */ VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE = 1 << 10, + /* skip post parse callback */ + VIR_DOMAIN_DEF_PARSE_SKIP_POST_PARSE = 1 << 11, } virDomainDefParseFlags; typedef enum { -- 2.8.4
When creating a copy of virDomainDef we save ourselves the trouble of writing deep-copy functions and just format and parse back domain/device XML. However, the XML we are parsing was already fully formatted - there is no reason to run post parse callbacks (which fill in blanks - there are none!). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 6 ++++-- src/qemu/qemu_domain.c | 3 ++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7e4aba6..e9a48d2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -24603,7 +24603,8 @@ virDomainDefCopy(virDomainDefPtr src, virDomainDefPtr ret; unsigned int format_flags = VIR_DOMAIN_DEF_FORMAT_SECURE; unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE | - VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE; + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE | + VIR_DOMAIN_DEF_PARSE_SKIP_POST_PARSE; if (migratable) format_flags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE | VIR_DOMAIN_DEF_FORMAT_MIGRATABLE; @@ -25090,7 +25091,8 @@ virDomainDeviceDefCopy(virDomainDeviceDefPtr src, xmlStr = virBufferContentAndReset(&buf); ret = virDomainDeviceDefParse(xmlStr, def, caps, xmlopt, VIR_DOMAIN_DEF_PARSE_INACTIVE | - VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE); + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE | + VIR_DOMAIN_DEF_PARSE_SKIP_POST_PARSE); cleanup: VIR_FREE(xmlStr); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d3af622..13e1319 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3388,7 +3388,8 @@ qemuDomainDefCopy(virQEMUDriverPtr driver, if (!(ret = virDomainDefParseString(xml, caps, driver->xmlopt, VIR_DOMAIN_DEF_PARSE_INACTIVE | - VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE | + VIR_DOMAIN_DEF_PARSE_SKIP_POST_PARSE))) goto cleanup; cleanup: -- 2.8.4
On Tue, Sep 20, 2016 at 15:54:54 +0200, Michal Privoznik wrote:
Looks like nobody tried migrations lately.
This statement sounds a bit too strong for such a narrow use case.
I just did and found interesting bug. On the destination qemu binary was at different path. So I've dumped domain XML I was trying ti migrate and just changed the emulator path. All of a sudden I observed plenty of errors. Problem is that whilst parsing the user provided migration XML (and doing other operations on it), because of parse callbacks we need qemuCaps for the binary. However, we just take the def->emulator and expect it to exist. Well, it doesn't.
I think you're just the first one who tried migrating to a host where QEMU is installed in a different path. Or at least the first one who didn't just make a symlink to make QEMU binary reachable with the same path :-)
Michal Privoznik (9): conf: Introduce virDomainDefPostParseOpaque conf: Introduce virDomainDefParseStringOpaque qemuDomainDefPostParse: Fetch qemuCaps from domain object conf: Extend virDomainDeviceDefPostParse for parseOpaque qemuDomainDeviceDefPostParse: Fetch caps from domain object conf: Extend virDomainDefAssignAddressesCallback for parseOpaque qemuDomainDefAssignAddresses: Fetch caps from domain object domain_conf: Introduce VIR_DOMAIN_DEF_PARSE_SKIP_POST_PARSE conf: Skip post parse callbacks when creating copy
Overall, the series looks OK, except for the new *Opaque functions. They are all internal APIs and I think just adding an extra parameter to the existing functions is much better than introducing new ones with even longer and uglier names. And git grep doesn't even show a lot of callers so the change to the existing prototypes won't be too invasive. And do you plan to actually use vm pointers anywhere in the post-parse callbacks in the future? If it's not the case, I think passing priv->qemuCaps directly would be a bit better. Jirka
On 22.09.2016 15:39, Jiri Denemark wrote:
On Tue, Sep 20, 2016 at 15:54:54 +0200, Michal Privoznik wrote:
Looks like nobody tried migrations lately.
This statement sounds a bit too strong for such a narrow use case.
I just did and found interesting bug. On the destination qemu binary was at different path. So I've dumped domain XML I was trying ti migrate and just changed the emulator path. All of a sudden I observed plenty of errors. Problem is that whilst parsing the user provided migration XML (and doing other operations on it), because of parse callbacks we need qemuCaps for the binary. However, we just take the def->emulator and expect it to exist. Well, it doesn't.
I think you're just the first one who tried migrating to a host where QEMU is installed in a different path. Or at least the first one who didn't just make a symlink to make QEMU binary reachable with the same path :-)
Michal Privoznik (9): conf: Introduce virDomainDefPostParseOpaque conf: Introduce virDomainDefParseStringOpaque qemuDomainDefPostParse: Fetch qemuCaps from domain object conf: Extend virDomainDeviceDefPostParse for parseOpaque qemuDomainDeviceDefPostParse: Fetch caps from domain object conf: Extend virDomainDefAssignAddressesCallback for parseOpaque qemuDomainDefAssignAddresses: Fetch caps from domain object domain_conf: Introduce VIR_DOMAIN_DEF_PARSE_SKIP_POST_PARSE conf: Skip post parse callbacks when creating copy
Overall, the series looks OK, except for the new *Opaque functions. They are all internal APIs and I think just adding an extra parameter to the existing functions is much better than introducing new ones with even longer and uglier names. And git grep doesn't even show a lot of callers so the change to the existing prototypes won't be too invasive.
Ah, okay, I can stick with that approach then. I thought that this is going to be more acceptable to the upstream. Don't really know why now.
And do you plan to actually use vm pointers anywhere in the post-parse callbacks in the future? If it's not the case, I think passing priv->qemuCaps directly would be a bit better.
Well, okay. I think domain objects are more versatile, but if somebody needs them, we can always change that. So let me respin version 2. Michal
participants (2)
-
Jiri Denemark -
Michal Privoznik