[libvirt] [PATCH 0/3] Check boot order on device attach
https://bugzilla.redhat.com/show_bug.cgi?id=1007754 When attaching a new device, we need to check if its boot order configuration is compatible with current domain definition. Jiri Denemark (3): Fix usage of virDomainDefCompatibleDevice Pass action to virDomainDefCompatibleDevice Check boot order on device attach src/conf/domain_conf.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 10 +++++- src/lxc/lxc_driver.c | 32 ++++++++++++------- src/qemu/qemu_driver.c | 32 ++++++++++++------- 4 files changed, 133 insertions(+), 26 deletions(-) -- 1.9.1
A device needs to be checked for compatibility with the domain definition it corresponds to. Specifically, for VIR_DOMAIN_AFFECT_CONFIG case we should check against persistent def rather than active def. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/lxc/lxc_driver.c | 20 +++++++++++--------- src/qemu/qemu_driver.c | 20 +++++++++++--------- 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 1ae04c5..ca82fd2 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -5015,13 +5015,14 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (virDomainDefCompatibleDevice(vm->def, dev) < 0) - goto cleanup; - /* Make a copy for updated domain. */ vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); if (!vmdef) goto cleanup; + + if (virDomainDefCompatibleDevice(vmdef, dev) < 0) + goto cleanup; + if ((ret = lxcDomainAttachDeviceConfig(vmdef, dev)) < 0) goto cleanup; } @@ -5141,13 +5142,14 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (virDomainDefCompatibleDevice(vm->def, dev) < 0) - goto cleanup; - /* Make a copy for updated domain. */ vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); if (!vmdef) goto cleanup; + + if (virDomainDefCompatibleDevice(vmdef, dev) < 0) + goto cleanup; + if ((ret = lxcDomainUpdateDeviceConfig(vmdef, dev)) < 0) goto cleanup; } @@ -5251,14 +5253,14 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (virDomainDefCompatibleDevice(vm->def, dev) < 0) - goto cleanup; - /* Make a copy for updated domain. */ vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); if (!vmdef) goto cleanup; + if (virDomainDefCompatibleDevice(vmdef, dev) < 0) + goto cleanup; + if ((ret = lxcDomainDetachDeviceConfig(vmdef, dev)) < 0) goto cleanup; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2707bec..8ec8912 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6946,13 +6946,14 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, goto cleanup; if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (virDomainDefCompatibleDevice(vm->def, dev) < 0) - goto endjob; - /* Make a copy for updated domain. */ vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); if (!vmdef) goto endjob; + + if (virDomainDefCompatibleDevice(vmdef, dev) < 0) + goto endjob; + if ((ret = qemuDomainAttachDeviceConfig(qemuCaps, vmdef, dev)) < 0) goto endjob; } @@ -7089,14 +7090,14 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, goto cleanup; if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (virDomainDefCompatibleDevice(vm->def, dev) < 0) - goto endjob; - /* Make a copy for updated domain. */ vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); if (!vmdef) goto endjob; + if (virDomainDefCompatibleDevice(vmdef, dev) < 0) + goto endjob; + if ((ret = qemuDomainUpdateDeviceConfig(qemuCaps, vmdef, dev)) < 0) goto endjob; } @@ -7228,13 +7229,14 @@ static int qemuDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, goto cleanup; if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (virDomainDefCompatibleDevice(vm->def, dev) < 0) - goto endjob; - /* Make a copy for updated domain. */ vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt); if (!vmdef) goto endjob; + + if (virDomainDefCompatibleDevice(vmdef, dev) < 0) + goto endjob; + if ((ret = qemuDomainDetachDeviceConfig(vmdef, dev)) < 0) goto endjob; } -- 1.9.1
On 03/20/2014 08:42 AM, Jiri Denemark wrote:
A device needs to be checked for compatibility with the domain definition it corresponds to. Specifically, for VIR_DOMAIN_AFFECT_CONFIG case we should check against persistent def rather than active def.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/lxc/lxc_driver.c | 20 +++++++++++--------- src/qemu/qemu_driver.c | 20 +++++++++++--------- 2 files changed, 22 insertions(+), 18 deletions(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
When checking compatibility of a device with a domain definition, we should know what we're going to do with the device. Because we may need to check for different things when we're attaching a new device versus detaching an existing device. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/conf/domain_conf.c | 6 +++++- src/conf/domain_conf.h | 9 ++++++++- src/lxc/lxc_driver.c | 18 ++++++++++++------ src/qemu/qemu_driver.c | 18 ++++++++++++------ 4 files changed, 37 insertions(+), 14 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 89aa52c..ecfec0d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -17825,8 +17825,12 @@ virDomainDeviceIsUSB(virDomainDeviceDefPtr dev) int virDomainDefCompatibleDevice(virDomainDefPtr def, - virDomainDeviceDefPtr dev) + virDomainDeviceDefPtr dev, + virDomainDeviceAction action) { + if (action != VIR_DOMAIN_DEVICE_ACTION_ATTACH) + return 0; + if (!virDomainDefHasUSB(def) && STRNEQ(def->os.type, "exe") && virDomainDeviceIsUSB(dev)) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 27f07e6..d5d5fd3 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2407,8 +2407,15 @@ int virDomainNetDefFormat(virBufferPtr buf, virDomainNetDefPtr def, unsigned int flags); +typedef enum { + VIR_DOMAIN_DEVICE_ACTION_ATTACH, + VIR_DOMAIN_DEVICE_ACTION_DETACH, + VIR_DOMAIN_DEVICE_ACTION_UPDATE, +} virDomainDeviceAction; + int virDomainDefCompatibleDevice(virDomainDefPtr def, - virDomainDeviceDefPtr dev); + virDomainDeviceDefPtr dev, + virDomainDeviceAction action); int virDomainVcpuPinAdd(virDomainVcpuPinDefPtr **vcpupin_list, size_t *nvcpupin, diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index ca82fd2..109132e 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -5020,7 +5020,8 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom, if (!vmdef) goto cleanup; - if (virDomainDefCompatibleDevice(vmdef, dev) < 0) + if (virDomainDefCompatibleDevice(vmdef, dev, + VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0) goto cleanup; if ((ret = lxcDomainAttachDeviceConfig(vmdef, dev)) < 0) @@ -5028,7 +5029,8 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (virDomainDefCompatibleDevice(vm->def, dev_copy) < 0) + if (virDomainDefCompatibleDevice(vm->def, dev_copy, + VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0) goto cleanup; if ((ret = lxcDomainAttachDeviceLive(dom->conn, driver, vm, dev_copy)) < 0) @@ -5147,7 +5149,8 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom, if (!vmdef) goto cleanup; - if (virDomainDefCompatibleDevice(vmdef, dev) < 0) + if (virDomainDefCompatibleDevice(vmdef, dev, + VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0) goto cleanup; if ((ret = lxcDomainUpdateDeviceConfig(vmdef, dev)) < 0) @@ -5155,7 +5158,8 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (virDomainDefCompatibleDevice(vm->def, dev_copy) < 0) + if (virDomainDefCompatibleDevice(vm->def, dev_copy, + VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0) goto cleanup; virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", @@ -5258,7 +5262,8 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom, if (!vmdef) goto cleanup; - if (virDomainDefCompatibleDevice(vmdef, dev) < 0) + if (virDomainDefCompatibleDevice(vmdef, dev, + VIR_DOMAIN_DEVICE_ACTION_DETACH) < 0) goto cleanup; if ((ret = lxcDomainDetachDeviceConfig(vmdef, dev)) < 0) @@ -5266,7 +5271,8 @@ static int lxcDomainDetachDeviceFlags(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (virDomainDefCompatibleDevice(vm->def, dev_copy) < 0) + if (virDomainDefCompatibleDevice(vm->def, dev_copy, + VIR_DOMAIN_DEVICE_ACTION_DETACH) < 0) goto cleanup; if ((ret = lxcDomainDetachDeviceLive(driver, vm, dev_copy)) < 0) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8ec8912..dec216c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6951,7 +6951,8 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, if (!vmdef) goto endjob; - if (virDomainDefCompatibleDevice(vmdef, dev) < 0) + if (virDomainDefCompatibleDevice(vmdef, dev, + VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0) goto endjob; if ((ret = qemuDomainAttachDeviceConfig(qemuCaps, vmdef, dev)) < 0) @@ -6959,7 +6960,8 @@ static int qemuDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, } if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (virDomainDefCompatibleDevice(vm->def, dev_copy) < 0) + if (virDomainDefCompatibleDevice(vm->def, dev_copy, + VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0) goto endjob; if ((ret = qemuDomainAttachDeviceLive(vm, dev_copy, dom)) < 0) @@ -7095,7 +7097,8 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, if (!vmdef) goto endjob; - if (virDomainDefCompatibleDevice(vmdef, dev) < 0) + if (virDomainDefCompatibleDevice(vmdef, dev, + VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0) goto endjob; if ((ret = qemuDomainUpdateDeviceConfig(qemuCaps, vmdef, dev)) < 0) @@ -7103,7 +7106,8 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (virDomainDefCompatibleDevice(vm->def, dev_copy) < 0) + if (virDomainDefCompatibleDevice(vm->def, dev_copy, + VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0) goto endjob; if ((ret = qemuDomainUpdateDeviceLive(dom->conn, vm, dev_copy, dom, force)) < 0) @@ -7234,7 +7238,8 @@ static int qemuDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, if (!vmdef) goto endjob; - if (virDomainDefCompatibleDevice(vmdef, dev) < 0) + if (virDomainDefCompatibleDevice(vmdef, dev, + VIR_DOMAIN_DEVICE_ACTION_DETACH) < 0) goto endjob; if ((ret = qemuDomainDetachDeviceConfig(vmdef, dev)) < 0) @@ -7242,7 +7247,8 @@ static int qemuDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, } if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (virDomainDefCompatibleDevice(vm->def, dev_copy) < 0) + if (virDomainDefCompatibleDevice(vm->def, dev_copy, + VIR_DOMAIN_DEVICE_ACTION_DETACH) < 0) goto endjob; if ((ret = qemuDomainDetachDeviceLive(vm, dev_copy, dom)) < 0) -- 1.9.1
On 03/20/2014 08:42 AM, Jiri Denemark wrote:
When checking compatibility of a device with a domain definition, we should know what we're going to do with the device. Because we may need to check for different things when we're attaching a new device versus detaching an existing device.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/conf/domain_conf.c | 6 +++++- src/conf/domain_conf.h | 9 ++++++++- src/lxc/lxc_driver.c | 18 ++++++++++++------ src/qemu/qemu_driver.c | 18 ++++++++++++------ 4 files changed, 37 insertions(+), 14 deletions(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
https://bugzilla.redhat.com/show_bug.cgi?id=1007754 When attaching a new device, we need to check if its boot order configuration is compatible with current domain definition. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/conf/domain_conf.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 1 + 2 files changed, 80 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ecfec0d..05e9d3a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2565,6 +2565,53 @@ int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info, return 0; } +virDomainDeviceInfoPtr +virDomainDeviceGetInfo(virDomainDeviceDefPtr device) +{ + switch ((virDomainDeviceType) device->type) { + case VIR_DOMAIN_DEVICE_DISK: + return &device->data.disk->info; + case VIR_DOMAIN_DEVICE_FS: + return &device->data.fs->info; + case VIR_DOMAIN_DEVICE_NET: + return &device->data.net->info; + case VIR_DOMAIN_DEVICE_INPUT: + return &device->data.input->info; + case VIR_DOMAIN_DEVICE_SOUND: + return &device->data.sound->info; + case VIR_DOMAIN_DEVICE_VIDEO: + return &device->data.video->info; + case VIR_DOMAIN_DEVICE_HOSTDEV: + return device->data.hostdev->info; + case VIR_DOMAIN_DEVICE_WATCHDOG: + return &device->data.watchdog->info; + case VIR_DOMAIN_DEVICE_CONTROLLER: + return &device->data.controller->info; + case VIR_DOMAIN_DEVICE_HUB: + return &device->data.hub->info; + case VIR_DOMAIN_DEVICE_REDIRDEV: + return &device->data.redirdev->info; + case VIR_DOMAIN_DEVICE_SMARTCARD: + return &device->data.smartcard->info; + case VIR_DOMAIN_DEVICE_CHR: + return &device->data.chr->info; + case VIR_DOMAIN_DEVICE_MEMBALLOON: + return &device->data.memballoon->info; + case VIR_DOMAIN_DEVICE_NVRAM: + return &device->data.nvram->info; + case VIR_DOMAIN_DEVICE_RNG: + return &device->data.rng->info; + + /* The following devices do not contain virDomainDeviceInfo */ + case VIR_DOMAIN_DEVICE_LEASE: + case VIR_DOMAIN_DEVICE_GRAPHICS: + case VIR_DOMAIN_DEVICE_LAST: + case VIR_DOMAIN_DEVICE_NONE: + break; + } + return NULL; +} + static bool virDomainDeviceInfoIsSet(virDomainDeviceInfoPtr info, unsigned int flags) { @@ -17823,11 +17870,30 @@ virDomainDeviceIsUSB(virDomainDeviceDefPtr dev) return false; } +static int +virDomainDeviceInfoCheckBootIndex(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceDefPtr device ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, + void *opaque) +{ + virDomainDeviceInfoPtr newinfo = opaque; + + if (info->bootIndex == newinfo->bootIndex) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("boot order %d is already used by another device"), + newinfo->bootIndex); + return -1; + } + return 0; +} + int virDomainDefCompatibleDevice(virDomainDefPtr def, virDomainDeviceDefPtr dev, virDomainDeviceAction action) { + virDomainDeviceInfoPtr info = virDomainDeviceGetInfo(dev); + if (action != VIR_DOMAIN_DEVICE_ACTION_ATTACH) return 0; @@ -17840,6 +17906,19 @@ virDomainDefCompatibleDevice(virDomainDefPtr def, return -1; } + if (info && info->bootIndex > 0) { + if (def->os.nBootDevs > 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("per-device boot elements cannot be used" + " together with os/boot elements")); + return -1; + } + if (virDomainDeviceInfoIterate(def, + virDomainDeviceInfoCheckBootIndex, + info) < 0) + return -1; + } + return 0; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index d5d5fd3..bf12414 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2291,6 +2291,7 @@ virDomainDeviceDefPtr virDomainDeviceDefCopy(virDomainDeviceDefPtr src, virDomainXMLOptionPtr xmlopt); int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info, int type); +virDomainDeviceInfoPtr virDomainDeviceGetInfo(virDomainDeviceDefPtr device); int virDomainDeviceInfoCopy(virDomainDeviceInfoPtr dst, virDomainDeviceInfoPtr src); void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info); -- 1.9.1
On 03/20/2014 08:42 AM, Jiri Denemark wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1007754
When attaching a new device, we need to check if its boot order configuration is compatible with current domain definition.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/conf/domain_conf.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 1 + 2 files changed, 80 insertions(+)
+virDomainDeviceInfoPtr +virDomainDeviceGetInfo(virDomainDeviceDefPtr device) +{
I probably would have split this helper function into its own patch - it may be handy in other situations, where we might want to backport just this function and a future use of it. But the overall patch is small enough that I don't mind keeping it as one patch to minimize your churn. ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, Mar 20, 2014 at 14:05:15 -0600, Eric Blake wrote:
On 03/20/2014 08:42 AM, Jiri Denemark wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1007754
When attaching a new device, we need to check if its boot order configuration is compatible with current domain definition.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/conf/domain_conf.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 1 + 2 files changed, 80 insertions(+)
+virDomainDeviceInfoPtr +virDomainDeviceGetInfo(virDomainDeviceDefPtr device) +{
I probably would have split this helper function into its own patch - it may be handy in other situations, where we might want to backport just this function and a future use of it. But the overall patch is small enough that I don't mind keeping it as one patch to minimize your churn.
It's a good idea to split it anyway so I did that. And I forgot to add the new API to libvirt_private.syms so I did that now.
ACK
Split, fixed and pushed, thanks. Jirka
participants (2)
-
Eric Blake -
Jiri Denemark