[libvirt] [Patch v7 1/4] libvirt/qemu - persistent modification of domain.
This is v7. Dropped patches for Nics and added 2 sanity checks and show correct error messages. =
From 948597237bd9ecfc5c7343fd30efdca37733274e Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Fri, 25 Mar 2011 16:59:55 +0900 Subject: [PATCHv7 1/4] libvirt/qemu - persistent modification of devices.
Now, qemudDomainAttachDeviceFlags() and qemudDomainDetachDeviceFlags() doesn't support VIR_DOMAIN_DEVICE_MODIFY_CONFIG. By this, virsh's at(de)tatch-device --persistent cannot modify qemu config. (Xen allows it.) This patch is a base patch for adding support of devices in step by step manner. Following patches will add some device support. Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/qemu/qemu_driver.c | 138 +++++++++++++++++++++++++++++++++++++++++++----- 1 files changed, 125 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index af897ad..a4fb1cd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4144,16 +4144,125 @@ cleanup: return ret; } -static int qemudDomainAttachDeviceFlags(virDomainPtr dom, - const char *xml, - unsigned int flags) { - if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("cannot modify the persistent configuration of a domain")); +/* + * Attach a device given by XML, the change will be persistent + * and domain XML definition file is updated. + */ +static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, + virDomainDeviceDefPtr newdev) +{ + + switch(newdev->type) { + default: + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Sorry, the device is not supported for now")); + return -1; + } + + return 0; +} + +static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef, + virDomainDeviceDefPtr device) +{ + switch(device->type) { + default: + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Sorry, the device is not supported for now")); return -1; } + return 0; +} + +static int qemuDomainModifyDevicePersistent(virDomainPtr dom, + const char *xml, + unsigned int attach, + unsigned int flags) +{ + struct qemud_driver *driver; + virDomainDeviceDefPtr device; + virDomainDefPtr vmdef; + virDomainObjPtr vm; + int ret = -1; + + /* + * When both of MODIFY_CONFIG and MODIFY_LIVE is specified at the same, + * time return error for now. We should support this later. + */ + if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Now, cannot modify alive domain and its definition " + "at the same time.")); + return -1; + } + + driver = dom->conn->privateData; + qemuDriverLock(driver); + vm = virDomainFindByName(&driver->domains, dom->name); + if (!vm) { + qemuReportError(VIR_ERR_NO_DOMAIN, _("no domain with name : '%s'"), + dom->name); + goto unlock_out; + } + + if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) + goto unlock_out; + + if (virDomainObjIsActive(vm)) { + /* + * For now, just allow updating inactive domains. Further development + * will allow updating both active domain and its config file at + * the same time. + */ + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("unsupported to update active domain's definition.")); + goto endjob; + } + + vmdef = virDomainObjGetPersistentDef(driver->caps, vm); - return qemudDomainAttachDevice(dom, xml); + if (!vmdef) + goto endjob; + + device = virDomainDeviceDefParse(driver->caps, + vmdef, xml, VIR_DOMAIN_XML_INACTIVE); + if (!device) + goto endjob; + + if (attach) + ret = qemuDomainAttachDevicePersistent(vmdef, device); + else + ret = qemuDomainDetachDevicePersistent(vmdef, device); + + if (!ret) /* save the result */ + ret = virDomainSaveConfig(driver->configDir, vmdef); + + virDomainDeviceDefFree(device); + +endjob: + if (qemuDomainObjEndJob(vm) == 0) + vm = NULL; +unlock_out: + if (vm) + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + return ret; +} + +static int qemudDomainAttachDeviceFlags(virDomainPtr dom, + const char *xml, + unsigned int flags) +{ + if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) + return qemuDomainModifyDevicePersistent(dom, xml, 1, flags); + + if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) + return qemudDomainAttachDevice(dom, xml); + + qemuReportError(VIR_ERR_INVALID_ARG, + _("bad flag: %x not supported"), flags); + + return -1; } @@ -4368,13 +4477,16 @@ cleanup: static int qemudDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, unsigned int flags) { - if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("cannot modify the persistent configuration of a domain")); - return -1; - } - return qemudDomainDetachDevice(dom, xml); + if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) + return qemuDomainModifyDevicePersistent(dom, xml, 0, flags); + + if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) + return qemudDomainDetachDevice(dom, xml); + + qemuReportError(VIR_ERR_INVALID_ARG, + _("bad flag: %x not supported now"), flags); + return -1; } static int qemudDomainGetAutostart(virDomainPtr dom, -- 1.7.4.1
From 4da9bdc62bb2b26610eb3b1ea8776d90af04dabc Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Fri, 25 Mar 2011 17:08:36 +0900 Subject: [PATCHv7 2/4] libvirt/qemu - support persistent disk modification
support changes of disks by VIR_DOMAIN_DEVICE_MODIFY_CONFIG for qemu. Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Changelog: v6->v7 - removed error message, which seems redundunt. (another patch will add new checks.) - Fixed a bug which PCI address is not assigned to a new controller. --- src/conf/domain_conf.c | 22 ++++++++++++++++++++++ src/conf/domain_conf.h | 3 ++- src/libvirt_private.syms | 2 ++ src/qemu/qemu_driver.c | 41 ++++++++++++++++++++++++++++++++++++++++- 4 files changed, 66 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b681dc3..3e3f342 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4859,6 +4859,19 @@ virVirtualPortProfileFormat(virBufferPtr buf, virBufferVSprintf(buf, "%s</virtualport>\n", indent); } +int virDomainDiskIndexByName(virDomainDefPtr def, const char *name) +{ + virDomainDiskDefPtr vdisk; + int i; + + for (i = 0; i < def->ndisks; i++) { + vdisk = def->disks[i]; + if (STREQ(vdisk->dst, name)) + return i; + } + return -1; +} + int virDomainDiskInsert(virDomainDefPtr def, virDomainDiskDefPtr disk) { @@ -4930,6 +4943,15 @@ void virDomainDiskRemove(virDomainDefPtr def, size_t i) } } +int virDomainDiskRemoveByName(virDomainDefPtr def, const char *name) +{ + int i = virDomainDiskIndexByName(def, name); + if (i < 0) + return -1; + virDomainDiskRemove(def, i); + return 0; +} + int virDomainControllerInsert(virDomainDefPtr def, virDomainControllerDefPtr controller) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1e8223f..b791714 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1255,7 +1255,7 @@ int virDomainCpuSetParse(const char **str, int maxcpu); char *virDomainCpuSetFormat(char *cpuset, int maxcpu); - +int virDomainDiskIndexByName(virDomainDefPtr def, const char *name); int virDomainDiskInsert(virDomainDefPtr def, virDomainDiskDefPtr disk); void virDomainDiskInsertPreAlloced(virDomainDefPtr def, @@ -1263,6 +1263,7 @@ void virDomainDiskInsertPreAlloced(virDomainDefPtr def, int virDomainDiskDefAssignAddress(virCapsPtr caps, virDomainDiskDefPtr def); void virDomainDiskRemove(virDomainDefPtr def, size_t i); +int virDomainDiskRemoveByName(virDomainDefPtr def, const char *name); int virDomainControllerInsert(virDomainDefPtr def, virDomainControllerDefPtr controller); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b4b6c63..7459c40 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -243,11 +243,13 @@ virDomainDiskDefFree; virDomainDiskDeviceTypeToString; virDomainDiskErrorPolicyTypeFromString; virDomainDiskErrorPolicyTypeToString; +virDomainDiskIndexByName; virDomainDiskInsert; virDomainDiskInsertPreAlloced; virDomainDiskIoTypeFromString; virDomainDiskIoTypeToString; virDomainDiskRemove; +virDomainDiskRemoveByName; virDomainDiskTypeFromString; virDomainDiskTypeToString; virDomainFSDefFree; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a4fb1cd..4d3b416 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4144,6 +4144,14 @@ cleanup: return ret; } +static int qemuDomainDeviceAddressFixup(virDomainDefPtr vmdef, bool pci) +{ + if (!pci && virDomainDefAddImplicitControllers(vmdef)) + return -1; + /* added controller requires PCI address */ + return qemuDomainAssignPCIAddresses(vmdef); +} + /* * Attach a device given by XML, the change will be persistent * and domain XML definition file is updated. @@ -4151,21 +4159,52 @@ cleanup: static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, virDomainDeviceDefPtr newdev) { + virDomainDiskDefPtr disk; + bool pci; + int ret; switch(newdev->type) { + case VIR_DOMAIN_DEVICE_DISK: + disk = newdev->data.disk; + if (virDomainDiskIndexByName(vmdef, disk->dst) >= 0) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("target %s already exists."), disk->dst); + return -1; + } + if (virDomainDiskInsert(vmdef, disk)) { + virReportOOMError(); + return -1; + } + pci = (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO); + ret = qemuDomainDeviceAddressFixup(vmdef, pci); + if (ret < 0) + virDomainDiskRemoveByName(vmdef, disk->dst); + else + newdev->data.disk = NULL; + break; default: qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Sorry, the device is not supported for now")); return -1; } - return 0; + return ret; } static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef, virDomainDeviceDefPtr device) { + virDomainDiskDefPtr disk; + switch(device->type) { + case VIR_DOMAIN_DEVICE_DISK: + disk = device->data.disk; + if (virDomainDiskRemoveByName(vmdef, disk->dst)) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("no target device %s"), disk->dst); + return -1; + } + break; default: qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Sorry, the device is not supported for now")); -- 1.7.4.1
From 638341bdf3eaac824e36d265e134608279750049 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Fri, 25 Mar 2011 17:10:58 +0900 Subject: [PATCHv7 3/4] libvirt/qemu - check address confliction before addition.
qemuDomainAttachDevicePersistent() calls qemuDomainAssignPCIAddresses() and virDomainDefAddImplicitControllers() at the end of its call. But PCI/Drive address confliction checks are PCI - confliction will be found but error report is not verbose. Drive - never done. For example, you can add following (unusual) 2 devices without errors. <disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/test3.img'/> <target dev="sdx" bus='scsi'/> <address type='drive' controller='0' bus='0' unit='0'/> </disk> <disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/test3.img'/> <target dev="sdy" bus='scsi'/> <address type='drive' controller='0' bus='0' unit='0'/> </disk> It's better to check drive address confliction before addition. Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/conf/domain_conf.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 2 + src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 9 +++++++ 4 files changed, 71 insertions(+), 0 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3e3f342..4a54f62 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1287,6 +1287,65 @@ void virDomainDefClearDeviceAliases(virDomainDefPtr def) virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearAlias, NULL); } +static int virDomainDeviceAddressMatch(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, + void *opaque) +{ + virDomainDeviceInfoPtr checked = opaque; + /* skip to check confliction of alias */ + if (info->type != checked->type) + return 0; + if (info->alias && checked->alias && strcmp(info->alias, checked->alias)) + return -1; + if (!memcmp(&info->addr, &checked->addr, sizeof(info->addr))) + return -1; + return 0; +} + +int virDomainDefFindDeviceAddressConflict(virDomainDefPtr def, + virDomainDeviceDefPtr dev) +{ + virDomainDeviceInfoPtr checked; + + switch (dev->type) { + case VIR_DOMAIN_DEVICE_DISK: + checked = &dev->data.disk->info; + break; + case VIR_DOMAIN_DEVICE_FS: + checked = &dev->data.fs->info; + break; + case VIR_DOMAIN_DEVICE_NET: + checked = &dev->data.net->info; + break; + case VIR_DOMAIN_DEVICE_INPUT: + checked = &dev->data.input->info; + break; + case VIR_DOMAIN_DEVICE_SOUND: + checked = &dev->data.sound->info; + break; + case VIR_DOMAIN_DEVICE_VIDEO: + checked = &dev->data.video->info; + break; + case VIR_DOMAIN_DEVICE_HOSTDEV: + checked = &dev->data.hostdev->info; + break; + case VIR_DOMAIN_DEVICE_WATCHDOG: + checked = &dev->data.watchdog->info; + break; + case VIR_DOMAIN_DEVICE_CONTROLLER: + checked = &dev->data.controller->info; + break; + case VIR_DOMAIN_DEVICE_GRAPHICS: /* has no address info */ + return 0; + default: + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Unknown device type")); + return -1; + } + if (checked->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + return 0; + return virDomainDeviceInfoIterate(def, virDomainDeviceAddressMatch, checked); +} /* Generate a string representation of a device address * @param address Device address to stringify diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index b791714..53ccf32 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1194,6 +1194,8 @@ int virDomainDeviceInfoIsSet(virDomainDeviceInfoPtr info); void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info); void virDomainDefClearPCIAddresses(virDomainDefPtr def); void virDomainDefClearDeviceAliases(virDomainDefPtr def); +int virDomainDefFindDeviceAddressConflict(virDomainDefPtr def, + virDomainDeviceDefPtr dev); typedef int (*virDomainDeviceInfoCallback)(virDomainDefPtr def, virDomainDeviceInfoPtr dev, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7459c40..ffdf9cf 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -220,6 +220,7 @@ virDomainCpuSetParse; virDomainDefAddImplicitControllers; virDomainDefClearDeviceAliases; virDomainDefClearPCIAddresses; +virDomainDefFindDeviceAddressConflict; virDomainDefFormat; virDomainDefFree; virDomainDefParseFile; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4d3b416..e746576 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4144,6 +4144,9 @@ cleanup: return ret; } +/* + * Checking device's definition meets qemu's (and qemu drivre's) limitation. + */ static int qemuDomainDeviceAddressFixup(virDomainDefPtr vmdef, bool pci) { if (!pci && virDomainDefAddImplicitControllers(vmdef)) @@ -4163,6 +4166,12 @@ static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, bool pci; int ret; + if (virDomainDefFindDeviceAddressConflict(vmdef, newdev)) { + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("the device address already in use")); + return -1; + } + switch(newdev->type) { case VIR_DOMAIN_DEVICE_DISK: disk = newdev->data.disk; -- 1.7.4.1
On Fri, Mar 25, 2011 at 05:17:03PM +0900, KAMEZAWA Hiroyuki wrote:
From 638341bdf3eaac824e36d265e134608279750049 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Fri, 25 Mar 2011 17:10:58 +0900 Subject: [PATCHv7 3/4] libvirt/qemu - check address confliction before addition.
qemuDomainAttachDevicePersistent() calls qemuDomainAssignPCIAddresses() and virDomainDefAddImplicitControllers() at the end of its call.
But PCI/Drive address confliction checks are PCI - confliction will be found but error report is not verbose. Drive - never done.
For example, you can add following (unusual) 2 devices without errors.
<disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/test3.img'/> <target dev="sdx" bus='scsi'/> <address type='drive' controller='0' bus='0' unit='0'/> </disk>
<disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/test3.img'/> <target dev="sdy" bus='scsi'/> <address type='drive' controller='0' bus='0' unit='0'/> </disk>
It's better to check drive address confliction before addition.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/conf/domain_conf.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 2 + src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 9 +++++++ 4 files changed, 71 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3e3f342..4a54f62 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1287,6 +1287,65 @@ void virDomainDefClearDeviceAliases(virDomainDefPtr def) virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearAlias, NULL); }
+static int virDomainDeviceAddressMatch(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, + void *opaque) +{ + virDomainDeviceInfoPtr checked = opaque; + /* skip to check confliction of alias */ + if (info->type != checked->type) + return 0; + if (info->alias && checked->alias && strcmp(info->alias, checked->alias))
!STREQ instead of strcmp
+ return -1; + if (!memcmp(&info->addr, &checked->addr, sizeof(info->addr)))
Is it safe to memcmp an union like this? In the cases members of an union are of different sizes, and we intent to memcmp an union member that has a smaller size than the other members, then data in space not used by the union member to be compared is also compared. This is not a desired result.
+ return -1; + return 0; +} +
-- Thanks, Hu Tao
On Tue, 29 Mar 2011 10:41:57 +0800 Hu Tao <hutao@cn.fujitsu.com> wrote:
On Fri, Mar 25, 2011 at 05:17:03PM +0900, KAMEZAWA Hiroyuki wrote:
From 638341bdf3eaac824e36d265e134608279750049 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Fri, 25 Mar 2011 17:10:58 +0900 Subject: [PATCHv7 3/4] libvirt/qemu - check address confliction before addition.
qemuDomainAttachDevicePersistent() calls qemuDomainAssignPCIAddresses() and virDomainDefAddImplicitControllers() at the end of its call.
But PCI/Drive address confliction checks are PCI - confliction will be found but error report is not verbose. Drive - never done.
For example, you can add following (unusual) 2 devices without errors.
<disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/test3.img'/> <target dev="sdx" bus='scsi'/> <address type='drive' controller='0' bus='0' unit='0'/> </disk>
<disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/test3.img'/> <target dev="sdy" bus='scsi'/> <address type='drive' controller='0' bus='0' unit='0'/> </disk>
It's better to check drive address confliction before addition.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/conf/domain_conf.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 2 + src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 9 +++++++ 4 files changed, 71 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3e3f342..4a54f62 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1287,6 +1287,65 @@ void virDomainDefClearDeviceAliases(virDomainDefPtr def) virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearAlias, NULL); }
+static int virDomainDeviceAddressMatch(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, + void *opaque) +{ + virDomainDeviceInfoPtr checked = opaque; + /* skip to check confliction of alias */ + if (info->type != checked->type) + return 0; + if (info->alias && checked->alias && strcmp(info->alias, checked->alias))
!STREQ instead of strcmp
ok.
+ return -1; + if (!memcmp(&info->addr, &checked->addr, sizeof(info->addr)))
Is it safe to memcmp an union like this? In the cases members of an union are of different sizes, and we intent to memcmp an union member that has a smaller size than the other members, then data in space not used by the union member to be compared is also compared. This is not a desired result.
As far as I checked, it's zero cleared at allocation. Hmm, making this function bigger ? Thanks, -Kame
On Tue, Mar 29, 2011 at 11:40:44AM +0900, KAMEZAWA Hiroyuki wrote:
On Tue, 29 Mar 2011 10:41:57 +0800 Hu Tao <hutao@cn.fujitsu.com> wrote:
On Fri, Mar 25, 2011 at 05:17:03PM +0900, KAMEZAWA Hiroyuki wrote:
From 638341bdf3eaac824e36d265e134608279750049 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Fri, 25 Mar 2011 17:10:58 +0900 Subject: [PATCHv7 3/4] libvirt/qemu - check address confliction before addition.
qemuDomainAttachDevicePersistent() calls qemuDomainAssignPCIAddresses() and virDomainDefAddImplicitControllers() at the end of its call.
But PCI/Drive address confliction checks are PCI - confliction will be found but error report is not verbose. Drive - never done.
For example, you can add following (unusual) 2 devices without errors.
<disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/test3.img'/> <target dev="sdx" bus='scsi'/> <address type='drive' controller='0' bus='0' unit='0'/> </disk>
<disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/test3.img'/> <target dev="sdy" bus='scsi'/> <address type='drive' controller='0' bus='0' unit='0'/> </disk>
It's better to check drive address confliction before addition.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/conf/domain_conf.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 2 + src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 9 +++++++ 4 files changed, 71 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3e3f342..4a54f62 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1287,6 +1287,65 @@ void virDomainDefClearDeviceAliases(virDomainDefPtr def) virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearAlias, NULL); }
+static int virDomainDeviceAddressMatch(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, + void *opaque) +{ + virDomainDeviceInfoPtr checked = opaque; + /* skip to check confliction of alias */ + if (info->type != checked->type) + return 0; + if (info->alias && checked->alias && strcmp(info->alias, checked->alias))
!STREQ instead of strcmp
ok.
+ return -1; + if (!memcmp(&info->addr, &checked->addr, sizeof(info->addr)))
Is it safe to memcmp an union like this? In the cases members of an union are of different sizes, and we intent to memcmp an union member that has a smaller size than the other members, then data in space not used by the union member to be compared is also compared. This is not a desired result.
As far as I checked, it's zero cleared at allocation. Hmm, making this function bigger ?
Yes it is safe if zero-cleared. Not worth to make this function complicated. -- Thanks, Hu Tao
At 03/25/2011 04:17 PM, KAMEZAWA Hiroyuki Write:
From 638341bdf3eaac824e36d265e134608279750049 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Fri, 25 Mar 2011 17:10:58 +0900 Subject: [PATCHv7 3/4] libvirt/qemu - check address confliction before addition.
qemuDomainAttachDevicePersistent() calls qemuDomainAssignPCIAddresses() and virDomainDefAddImplicitControllers() at the end of its call.
But PCI/Drive address confliction checks are PCI - confliction will be found but error report is not verbose. Drive - never done.
For example, you can add following (unusual) 2 devices without errors.
<disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/test3.img'/> <target dev="sdx" bus='scsi'/> <address type='drive' controller='0' bus='0' unit='0'/> </disk>
<disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/test3.img'/> <target dev="sdy" bus='scsi'/> <address type='drive' controller='0' bus='0' unit='0'/> </disk>
It's better to check drive address confliction before addition.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/conf/domain_conf.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 2 + src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 9 +++++++ 4 files changed, 71 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3e3f342..4a54f62 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1287,6 +1287,65 @@ void virDomainDefClearDeviceAliases(virDomainDefPtr def) virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearAlias, NULL); }
+static int virDomainDeviceAddressMatch(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, + void *opaque) +{ + virDomainDeviceInfoPtr checked = opaque; + /* skip to check confliction of alias */ + if (info->type != checked->type) + return 0; + if (info->alias && checked->alias && strcmp(info->alias, checked->alias)) + return -1; + if (!memcmp(&info->addr, &checked->addr, sizeof(info->addr))) + return -1; + return 0; +} + +int virDomainDefFindDeviceAddressConflict(virDomainDefPtr def, + virDomainDeviceDefPtr dev) +{ + virDomainDeviceInfoPtr checked; + + switch (dev->type) { + case VIR_DOMAIN_DEVICE_DISK: + checked = &dev->data.disk->info; + break; + case VIR_DOMAIN_DEVICE_FS: + checked = &dev->data.fs->info; + break; + case VIR_DOMAIN_DEVICE_NET: + checked = &dev->data.net->info; + break; + case VIR_DOMAIN_DEVICE_INPUT: + checked = &dev->data.input->info; + break; + case VIR_DOMAIN_DEVICE_SOUND: + checked = &dev->data.sound->info; + break; + case VIR_DOMAIN_DEVICE_VIDEO: + checked = &dev->data.video->info; + break; + case VIR_DOMAIN_DEVICE_HOSTDEV: + checked = &dev->data.hostdev->info; + break; + case VIR_DOMAIN_DEVICE_WATCHDOG: + checked = &dev->data.watchdog->info; + break; + case VIR_DOMAIN_DEVICE_CONTROLLER: + checked = &dev->data.controller->info; + break; + case VIR_DOMAIN_DEVICE_GRAPHICS: /* has no address info */ + return 0; + default: + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Unknown device type")); + return -1;
You report error here, but you report error in caller(qemuDomainAttachDevicePersistent()) again.
+ } + if (checked->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + return 0; + return virDomainDeviceInfoIterate(def, virDomainDeviceAddressMatch, checked); +}
/* Generate a string representation of a device address * @param address Device address to stringify diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index b791714..53ccf32 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1194,6 +1194,8 @@ int virDomainDeviceInfoIsSet(virDomainDeviceInfoPtr info); void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info); void virDomainDefClearPCIAddresses(virDomainDefPtr def); void virDomainDefClearDeviceAliases(virDomainDefPtr def); +int virDomainDefFindDeviceAddressConflict(virDomainDefPtr def, + virDomainDeviceDefPtr dev);
typedef int (*virDomainDeviceInfoCallback)(virDomainDefPtr def, virDomainDeviceInfoPtr dev, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7459c40..ffdf9cf 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -220,6 +220,7 @@ virDomainCpuSetParse; virDomainDefAddImplicitControllers; virDomainDefClearDeviceAliases; virDomainDefClearPCIAddresses; +virDomainDefFindDeviceAddressConflict; virDomainDefFormat; virDomainDefFree; virDomainDefParseFile; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4d3b416..e746576 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4144,6 +4144,9 @@ cleanup: return ret; }
+/* + * Checking device's definition meets qemu's (and qemu drivre's) limitation. + */
Is this comment in correct place? You do not modify this function. If this comment is for this function, it should be merged into patch 2. According to the comment's content, it is not for this function.
static int qemuDomainDeviceAddressFixup(virDomainDefPtr vmdef, bool pci) { if (!pci && virDomainDefAddImplicitControllers(vmdef)) @@ -4163,6 +4166,12 @@ static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, bool pci; int ret;
+ if (virDomainDefFindDeviceAddressConflict(vmdef, newdev)) { + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("the device address already in use")); + return -1; + } + switch(newdev->type) { case VIR_DOMAIN_DEVICE_DISK: disk = newdev->data.disk;
On Tue, 29 Mar 2011 13:09:37 +0800 Wen Congyang <wency@cn.fujitsu.com> wrote:
At 03/25/2011 04:17 PM, KAMEZAWA Hiroyuki Write:
From 638341bdf3eaac824e36d265e134608279750049 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Fri, 25 Mar 2011 17:10:58 +0900 Subject: [PATCHv7 3/4] libvirt/qemu - check address confliction before addition.
qemuDomainAttachDevicePersistent() calls qemuDomainAssignPCIAddresses() and virDomainDefAddImplicitControllers() at the end of its call.
But PCI/Drive address confliction checks are PCI - confliction will be found but error report is not verbose. Drive - never done.
For example, you can add following (unusual) 2 devices without errors.
<disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/test3.img'/> <target dev="sdx" bus='scsi'/> <address type='drive' controller='0' bus='0' unit='0'/> </disk>
<disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/test3.img'/> <target dev="sdy" bus='scsi'/> <address type='drive' controller='0' bus='0' unit='0'/> </disk>
It's better to check drive address confliction before addition.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/conf/domain_conf.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 2 + src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 9 +++++++ 4 files changed, 71 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3e3f342..4a54f62 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1287,6 +1287,65 @@ void virDomainDefClearDeviceAliases(virDomainDefPtr def) virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearAlias, NULL); }
+static int virDomainDeviceAddressMatch(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, + void *opaque) +{ + virDomainDeviceInfoPtr checked = opaque; + /* skip to check confliction of alias */ + if (info->type != checked->type) + return 0; + if (info->alias && checked->alias && strcmp(info->alias, checked->alias)) + return -1; + if (!memcmp(&info->addr, &checked->addr, sizeof(info->addr))) + return -1; + return 0; +} + +int virDomainDefFindDeviceAddressConflict(virDomainDefPtr def, + virDomainDeviceDefPtr dev) +{ + virDomainDeviceInfoPtr checked; + + switch (dev->type) { + case VIR_DOMAIN_DEVICE_DISK: + checked = &dev->data.disk->info; + break; + case VIR_DOMAIN_DEVICE_FS: + checked = &dev->data.fs->info; + break; + case VIR_DOMAIN_DEVICE_NET: + checked = &dev->data.net->info; + break; + case VIR_DOMAIN_DEVICE_INPUT: + checked = &dev->data.input->info; + break; + case VIR_DOMAIN_DEVICE_SOUND: + checked = &dev->data.sound->info; + break; + case VIR_DOMAIN_DEVICE_VIDEO: + checked = &dev->data.video->info; + break; + case VIR_DOMAIN_DEVICE_HOSTDEV: + checked = &dev->data.hostdev->info; + break; + case VIR_DOMAIN_DEVICE_WATCHDOG: + checked = &dev->data.watchdog->info; + break; + case VIR_DOMAIN_DEVICE_CONTROLLER: + checked = &dev->data.controller->info; + break; + case VIR_DOMAIN_DEVICE_GRAPHICS: /* has no address info */ + return 0; + default: + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Unknown device type")); + return -1;
You report error here, but you report error in caller(qemuDomainAttachDevicePersistent()) again.
"unknown device type" is an internal/"should never happen" error rathar than errors reported in the caller. I think this error should be reported. Internal error implies bug.
+ } + if (checked->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + return 0; + return virDomainDeviceInfoIterate(def, virDomainDeviceAddressMatch, checked); +}
/* Generate a string representation of a device address * @param address Device address to stringify diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index b791714..53ccf32 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1194,6 +1194,8 @@ int virDomainDeviceInfoIsSet(virDomainDeviceInfoPtr info); void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info); void virDomainDefClearPCIAddresses(virDomainDefPtr def); void virDomainDefClearDeviceAliases(virDomainDefPtr def); +int virDomainDefFindDeviceAddressConflict(virDomainDefPtr def, + virDomainDeviceDefPtr dev);
typedef int (*virDomainDeviceInfoCallback)(virDomainDefPtr def, virDomainDeviceInfoPtr dev, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7459c40..ffdf9cf 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -220,6 +220,7 @@ virDomainCpuSetParse; virDomainDefAddImplicitControllers; virDomainDefClearDeviceAliases; virDomainDefClearPCIAddresses; +virDomainDefFindDeviceAddressConflict; virDomainDefFormat; virDomainDefFree; virDomainDefParseFile; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4d3b416..e746576 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4144,6 +4144,9 @@ cleanup: return ret; }
+/* + * Checking device's definition meets qemu's (and qemu drivre's) limitation. + */
Is this comment in correct place? You do not modify this function. If this comment is for this function, it should be merged into patch 2. According to the comment's content, it is not for this function.
will check again. I'm not a good git user ;( Thanks, -Kame
At 03/29/2011 01:13 PM, KAMEZAWA Hiroyuki Write:
On Tue, 29 Mar 2011 13:09:37 +0800 Wen Congyang <wency@cn.fujitsu.com> wrote:
At 03/25/2011 04:17 PM, KAMEZAWA Hiroyuki Write:
From 638341bdf3eaac824e36d265e134608279750049 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Fri, 25 Mar 2011 17:10:58 +0900 Subject: [PATCHv7 3/4] libvirt/qemu - check address confliction before addition.
qemuDomainAttachDevicePersistent() calls qemuDomainAssignPCIAddresses() and virDomainDefAddImplicitControllers() at the end of its call.
But PCI/Drive address confliction checks are PCI - confliction will be found but error report is not verbose. Drive - never done.
For example, you can add following (unusual) 2 devices without errors.
<disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/test3.img'/> <target dev="sdx" bus='scsi'/> <address type='drive' controller='0' bus='0' unit='0'/> </disk>
<disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/test3.img'/> <target dev="sdy" bus='scsi'/> <address type='drive' controller='0' bus='0' unit='0'/> </disk>
It's better to check drive address confliction before addition.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/conf/domain_conf.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 2 + src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 9 +++++++ 4 files changed, 71 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3e3f342..4a54f62 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1287,6 +1287,65 @@ void virDomainDefClearDeviceAliases(virDomainDefPtr def) virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearAlias, NULL); }
+static int virDomainDeviceAddressMatch(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, + void *opaque) +{ + virDomainDeviceInfoPtr checked = opaque; + /* skip to check confliction of alias */ + if (info->type != checked->type) + return 0; + if (info->alias && checked->alias && strcmp(info->alias, checked->alias)) + return -1; + if (!memcmp(&info->addr, &checked->addr, sizeof(info->addr))) + return -1; + return 0; +} + +int virDomainDefFindDeviceAddressConflict(virDomainDefPtr def, + virDomainDeviceDefPtr dev) +{ + virDomainDeviceInfoPtr checked; + + switch (dev->type) { + case VIR_DOMAIN_DEVICE_DISK: + checked = &dev->data.disk->info; + break; + case VIR_DOMAIN_DEVICE_FS: + checked = &dev->data.fs->info; + break; + case VIR_DOMAIN_DEVICE_NET: + checked = &dev->data.net->info; + break; + case VIR_DOMAIN_DEVICE_INPUT: + checked = &dev->data.input->info; + break; + case VIR_DOMAIN_DEVICE_SOUND: + checked = &dev->data.sound->info; + break; + case VIR_DOMAIN_DEVICE_VIDEO: + checked = &dev->data.video->info; + break; + case VIR_DOMAIN_DEVICE_HOSTDEV: + checked = &dev->data.hostdev->info; + break; + case VIR_DOMAIN_DEVICE_WATCHDOG: + checked = &dev->data.watchdog->info; + break; + case VIR_DOMAIN_DEVICE_CONTROLLER: + checked = &dev->data.controller->info; + break; + case VIR_DOMAIN_DEVICE_GRAPHICS: /* has no address info */ + return 0; + default: + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Unknown device type")); + return -1;
You report error here, but you report error in caller(qemuDomainAttachDevicePersistent()) again.
"unknown device type" is an internal/"should never happen" error rathar than errors reported in the caller.
I think this error should be reported. Internal error implies bug.
Yes, this should not happen. If it happens, the error message will be overwritten by the caller. We can report error here if virDomainDeviceInfoIterate() returns -1, and the caller do not report error.
+ } + if (checked->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) + return 0; + return virDomainDeviceInfoIterate(def, virDomainDeviceAddressMatch, checked); +}
/* Generate a string representation of a device address * @param address Device address to stringify diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index b791714..53ccf32 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1194,6 +1194,8 @@ int virDomainDeviceInfoIsSet(virDomainDeviceInfoPtr info); void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info); void virDomainDefClearPCIAddresses(virDomainDefPtr def); void virDomainDefClearDeviceAliases(virDomainDefPtr def); +int virDomainDefFindDeviceAddressConflict(virDomainDefPtr def, + virDomainDeviceDefPtr dev);
typedef int (*virDomainDeviceInfoCallback)(virDomainDefPtr def, virDomainDeviceInfoPtr dev, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7459c40..ffdf9cf 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -220,6 +220,7 @@ virDomainCpuSetParse; virDomainDefAddImplicitControllers; virDomainDefClearDeviceAliases; virDomainDefClearPCIAddresses; +virDomainDefFindDeviceAddressConflict; virDomainDefFormat; virDomainDefFree; virDomainDefParseFile; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4d3b416..e746576 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4144,6 +4144,9 @@ cleanup: return ret; }
+/* + * Checking device's definition meets qemu's (and qemu drivre's) limitation. + */
Is this comment in correct place? You do not modify this function. If this comment is for this function, it should be merged into patch 2. According to the comment's content, it is not for this function.
will check again. I'm not a good git user ;(
Thanks, -Kame
On Tue, 29 Mar 2011 13:30:03 +0800 Wen Congyang <wency@cn.fujitsu.com> wrote:
At 03/29/2011 01:13 PM, KAMEZAWA Hiroyuki Write:
On Tue, 29 Mar 2011 13:09:37 +0800 Wen Congyang <wency@cn.fujitsu.com> wrote:
At 03/25/2011 04:17 PM, KAMEZAWA Hiroyuki Write:
From 638341bdf3eaac824e36d265e134608279750049 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Fri, 25 Mar 2011 17:10:58 +0900 Subject: [PATCHv7 3/4] libvirt/qemu - check address confliction before addition.
qemuDomainAttachDevicePersistent() calls qemuDomainAssignPCIAddresses() and virDomainDefAddImplicitControllers() at the end of its call.
But PCI/Drive address confliction checks are PCI - confliction will be found but error report is not verbose. Drive - never done.
For example, you can add following (unusual) 2 devices without errors.
<disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/test3.img'/> <target dev="sdx" bus='scsi'/> <address type='drive' controller='0' bus='0' unit='0'/> </disk>
<disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/test3.img'/> <target dev="sdy" bus='scsi'/> <address type='drive' controller='0' bus='0' unit='0'/> </disk>
It's better to check drive address confliction before addition.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/conf/domain_conf.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 2 + src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 9 +++++++ 4 files changed, 71 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3e3f342..4a54f62 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1287,6 +1287,65 @@ void virDomainDefClearDeviceAliases(virDomainDefPtr def) virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearAlias, NULL); }
+static int virDomainDeviceAddressMatch(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, + void *opaque) +{ + virDomainDeviceInfoPtr checked = opaque; + /* skip to check confliction of alias */ + if (info->type != checked->type) + return 0; + if (info->alias && checked->alias && strcmp(info->alias, checked->alias)) + return -1; + if (!memcmp(&info->addr, &checked->addr, sizeof(info->addr))) + return -1; + return 0; +} + +int virDomainDefFindDeviceAddressConflict(virDomainDefPtr def, + virDomainDeviceDefPtr dev) +{ + virDomainDeviceInfoPtr checked; + + switch (dev->type) { + case VIR_DOMAIN_DEVICE_DISK: + checked = &dev->data.disk->info; + break; + case VIR_DOMAIN_DEVICE_FS: + checked = &dev->data.fs->info; + break; + case VIR_DOMAIN_DEVICE_NET: + checked = &dev->data.net->info; + break; + case VIR_DOMAIN_DEVICE_INPUT: + checked = &dev->data.input->info; + break; + case VIR_DOMAIN_DEVICE_SOUND: + checked = &dev->data.sound->info; + break; + case VIR_DOMAIN_DEVICE_VIDEO: + checked = &dev->data.video->info; + break; + case VIR_DOMAIN_DEVICE_HOSTDEV: + checked = &dev->data.hostdev->info; + break; + case VIR_DOMAIN_DEVICE_WATCHDOG: + checked = &dev->data.watchdog->info; + break; + case VIR_DOMAIN_DEVICE_CONTROLLER: + checked = &dev->data.controller->info; + break; + case VIR_DOMAIN_DEVICE_GRAPHICS: /* has no address info */ + return 0; + default: + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Unknown device type")); + return -1;
You report error here, but you report error in caller(qemuDomainAttachDevicePersistent()) again.
"unknown device type" is an internal/"should never happen" error rathar than errors reported in the caller.
I think this error should be reported. Internal error implies bug.
Yes, this should not happen. If it happens, the error message will be overwritten by the caller. We can report error here if virDomainDeviceInfoIterate() returns -1, and the caller do not report error.
Is that make usability of this function ? If we report error, this function cannot be used for simple sanity check of pci addresses. Hmm, I'll replace this with DEBUG message. ok and never add error report here. Ok ? Thanks, -Kame
At 03/29/2011 01:31 PM, KAMEZAWA Hiroyuki Write:
On Tue, 29 Mar 2011 13:30:03 +0800 Wen Congyang <wency@cn.fujitsu.com> wrote:
At 03/29/2011 01:13 PM, KAMEZAWA Hiroyuki Write:
On Tue, 29 Mar 2011 13:09:37 +0800 Wen Congyang <wency@cn.fujitsu.com> wrote:
At 03/25/2011 04:17 PM, KAMEZAWA Hiroyuki Write:
From 638341bdf3eaac824e36d265e134608279750049 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Fri, 25 Mar 2011 17:10:58 +0900 Subject: [PATCHv7 3/4] libvirt/qemu - check address confliction before addition.
qemuDomainAttachDevicePersistent() calls qemuDomainAssignPCIAddresses() and virDomainDefAddImplicitControllers() at the end of its call.
But PCI/Drive address confliction checks are PCI - confliction will be found but error report is not verbose. Drive - never done.
For example, you can add following (unusual) 2 devices without errors.
<disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/test3.img'/> <target dev="sdx" bus='scsi'/> <address type='drive' controller='0' bus='0' unit='0'/> </disk>
<disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/var/lib/libvirt/images/test3.img'/> <target dev="sdy" bus='scsi'/> <address type='drive' controller='0' bus='0' unit='0'/> </disk>
It's better to check drive address confliction before addition.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/conf/domain_conf.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 2 + src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 9 +++++++ 4 files changed, 71 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3e3f342..4a54f62 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1287,6 +1287,65 @@ void virDomainDefClearDeviceAliases(virDomainDefPtr def) virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearAlias, NULL); }
+static int virDomainDeviceAddressMatch(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, + void *opaque) +{ + virDomainDeviceInfoPtr checked = opaque; + /* skip to check confliction of alias */ + if (info->type != checked->type) + return 0; + if (info->alias && checked->alias && strcmp(info->alias, checked->alias)) + return -1; + if (!memcmp(&info->addr, &checked->addr, sizeof(info->addr))) + return -1; + return 0; +} + +int virDomainDefFindDeviceAddressConflict(virDomainDefPtr def, + virDomainDeviceDefPtr dev) +{ + virDomainDeviceInfoPtr checked; + + switch (dev->type) { + case VIR_DOMAIN_DEVICE_DISK: + checked = &dev->data.disk->info; + break; + case VIR_DOMAIN_DEVICE_FS: + checked = &dev->data.fs->info; + break; + case VIR_DOMAIN_DEVICE_NET: + checked = &dev->data.net->info; + break; + case VIR_DOMAIN_DEVICE_INPUT: + checked = &dev->data.input->info; + break; + case VIR_DOMAIN_DEVICE_SOUND: + checked = &dev->data.sound->info; + break; + case VIR_DOMAIN_DEVICE_VIDEO: + checked = &dev->data.video->info; + break; + case VIR_DOMAIN_DEVICE_HOSTDEV: + checked = &dev->data.hostdev->info; + break; + case VIR_DOMAIN_DEVICE_WATCHDOG: + checked = &dev->data.watchdog->info; + break; + case VIR_DOMAIN_DEVICE_CONTROLLER: + checked = &dev->data.controller->info; + break; + case VIR_DOMAIN_DEVICE_GRAPHICS: /* has no address info */ + return 0; + default: + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Unknown device type")); + return -1;
You report error here, but you report error in caller(qemuDomainAttachDevicePersistent()) again.
"unknown device type" is an internal/"should never happen" error rathar than errors reported in the caller.
I think this error should be reported. Internal error implies bug.
Yes, this should not happen. If it happens, the error message will be overwritten by the caller. We can report error here if virDomainDeviceInfoIterate() returns -1, and the caller do not report error.
Is that make usability of this function ? If we report error, this function cannot be used for simple sanity check of pci addresses.
Hmm, I'll replace this with DEBUG message. ok and never add error report here. Ok ?
Sounds good. But use VIR_ERR instead of VIR_DEBUG, as it implies a bug.
Thanks, -Kame
From 8423316cbd8e4efa4240cc73ac65955d87cb9921 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Fri, 25 Mar 2011 17:13:05 +0900 Subject: [PATCHv7 4/4] libvirt/qemu - verify new device at attaching persistent
When adding a device by attach-device in --persistent mode, the user can pass invalid devices. For example, XX <disk type='file' device='disk'> <driver name='qemu' type='qcow2'/> <source file='/var/lib/libvirt/images/test3.img'/> <target dev='hde' bus='ide'/> </disk> XX When adding this, 2nd IDE controller will be added but qemu only supports 1 controller. It's better to check this kinds of qemu/qemu_driver limitation. (This can be added by virsh edit...) Check we can make a command line or not at adding device will be a consistent verification with future qemu driver updates. Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/qemu/qemu_driver.c | 35 +++++++++++++++++++++++++++++++++++ 1 files changed, 35 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e746576..482f762 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4156,6 +4156,38 @@ static int qemuDomainDeviceAddressFixup(virDomainDefPtr vmdef, bool pci) } /* + * Check the device definition meet's qemu/qemu_drriver's requirements. + */ +static int qemuDeviceVerifyDefinition(virDomainDefPtr vmdef, + virDomainDeviceDefPtr dev) +{ + virBitmapPtr qemuCaps = NULL; + int ret = -1; + char *optstr; + + if (qemuCapsExtractVersionInfo(vmdef->emulator, vmdef->os.arch, + NULL, + &qemuCaps)) + goto out; + switch(dev->type) { + case VIR_DOMAIN_DEVICE_DISK: + /* for verify, check we can create qemu command line or not */ + optstr = qemuBuildDriveStr(dev->data.disk, 0, qemuCaps); + if (!optstr) + goto free_out; + ret = 0; + VIR_FREE(optstr); + /* now, don't need to check qemuBuildDriveDevStr() */ + break; + default: + break; + } +free_out: + qemuCapsFree(qemuCaps); +out: + return ret; +} +/* * Attach a device given by XML, the change will be persistent * and domain XML definition file is updated. */ @@ -4166,6 +4198,9 @@ static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, bool pci; int ret; + if (qemuDeviceVerifyDefinition(vmdef, newdev)) + return -1; + if (virDomainDefFindDeviceAddressConflict(vmdef, newdev)) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", _("the device address already in use")); -- 1.7.4.1
On Fri, Mar 25, 2011 at 05:10:12PM +0900, KAMEZAWA Hiroyuki wrote:
This is v7. Dropped patches for Nics and added 2 sanity checks and show correct error messages.
This series looks good to me(except some minor problems in patch 3). Eric, will you have some time to have a review on this? -- Thanks, Hu Tao
At 03/25/2011 04:10 PM, KAMEZAWA Hiroyuki Write:
This is v7. Dropped patches for Nics and added 2 sanity checks and show correct error messages.
=
From 948597237bd9ecfc5c7343fd30efdca37733274e Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Fri, 25 Mar 2011 16:59:55 +0900 Subject: [PATCHv7 1/4] libvirt/qemu - persistent modification of devices.
Now, qemudDomainAttachDeviceFlags() and qemudDomainDetachDeviceFlags() doesn't support VIR_DOMAIN_DEVICE_MODIFY_CONFIG. By this, virsh's at(de)tatch-device --persistent cannot modify qemu config. (Xen allows it.)
This patch is a base patch for adding support of devices in step by step manner. Following patches will add some device support.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/qemu/qemu_driver.c | 138 +++++++++++++++++++++++++++++++++++++++++++----- 1 files changed, 125 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index af897ad..a4fb1cd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4144,16 +4144,125 @@ cleanup: return ret; }
-static int qemudDomainAttachDeviceFlags(virDomainPtr dom, - const char *xml, - unsigned int flags) { - if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("cannot modify the persistent configuration of a domain")); +/* + * Attach a device given by XML, the change will be persistent + * and domain XML definition file is updated. + */ +static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, + virDomainDeviceDefPtr newdev) +{ + + switch(newdev->type) { + default: + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Sorry, the device is not supported for now")); + return -1; + } + + return 0; +} + +static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef, + virDomainDeviceDefPtr device) +{ + switch(device->type) { + default: + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Sorry, the device is not supported for now")); return -1; } + return 0; +} + +static int qemuDomainModifyDevicePersistent(virDomainPtr dom, + const char *xml, + unsigned int attach, + unsigned int flags) +{ + struct qemud_driver *driver; + virDomainDeviceDefPtr device; + virDomainDefPtr vmdef; + virDomainObjPtr vm; + int ret = -1; + + /* + * When both of MODIFY_CONFIG and MODIFY_LIVE is specified at the same, + * time return error for now. We should support this later.
s/at the same, time/at the same time,/
+ */ + if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Now, cannot modify alive domain and its definition "
You use 'alive domain' here, but you use 'active domain' below. The message should be consistent.
+ "at the same time.")); + return -1; + } + + driver = dom->conn->privateData; + qemuDriverLock(driver); + vm = virDomainFindByName(&driver->domains, dom->name); + if (!vm) { + qemuReportError(VIR_ERR_NO_DOMAIN, _("no domain with name : '%s'"), + dom->name); + goto unlock_out; + } + + if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) + goto unlock_out; + + if (virDomainObjIsActive(vm)) { + /* + * For now, just allow updating inactive domains. Further development + * will allow updating both active domain and its config file at + * the same time. + */ + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("unsupported to update active domain's definition.")); + goto endjob; + } + + vmdef = virDomainObjGetPersistentDef(driver->caps, vm);
- return qemudDomainAttachDevice(dom, xml); + if (!vmdef) + goto endjob; + + device = virDomainDeviceDefParse(driver->caps, + vmdef, xml, VIR_DOMAIN_XML_INACTIVE); + if (!device) + goto endjob; + + if (attach) + ret = qemuDomainAttachDevicePersistent(vmdef, device); + else + ret = qemuDomainDetachDevicePersistent(vmdef, device); + + if (!ret) /* save the result */ + ret = virDomainSaveConfig(driver->configDir, vmdef);
vmdef may be modified even if ret is not 0, but you do not update the xml file. I do not find any problem about this, but it may be better to update the xml file when ret is not 0.
+ + virDomainDeviceDefFree(device); + +endjob: + if (qemuDomainObjEndJob(vm) == 0) + vm = NULL; +unlock_out: + if (vm) + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + return ret; +} + +static int qemudDomainAttachDeviceFlags(virDomainPtr dom, + const char *xml, + unsigned int flags) +{ + if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) + return qemuDomainModifyDevicePersistent(dom, xml, 1, flags); + + if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) + return qemudDomainAttachDevice(dom, xml); + + qemuReportError(VIR_ERR_INVALID_ARG, + _("bad flag: %x not supported"), flags);
If the flags is VIR_DOMAIN_DEVICE_MODIFY_LIVE | unsupported_flags, we do not report error here. You can use the macro virCheckFlags to check it.
+ + return -1; }
@@ -4368,13 +4477,16 @@ cleanup: static int qemudDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, unsigned int flags) { - if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("cannot modify the persistent configuration of a domain")); - return -1; - }
- return qemudDomainDetachDevice(dom, xml); + if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) + return qemuDomainModifyDevicePersistent(dom, xml, 0, flags); + + if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) + return qemudDomainDetachDevice(dom, xml); + + qemuReportError(VIR_ERR_INVALID_ARG, + _("bad flag: %x not supported now"), flags); + return -1;
the same as above
}
static int qemudDomainGetAutostart(virDomainPtr dom,
On Tue, 29 Mar 2011 11:24:23 +0800 Wen Congyang <wency@cn.fujitsu.com> wrote:
At 03/25/2011 04:10 PM, KAMEZAWA Hiroyuki Write:
This is v7. Dropped patches for Nics and added 2 sanity checks and show correct error messages.
=
From 948597237bd9ecfc5c7343fd30efdca37733274e Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Fri, 25 Mar 2011 16:59:55 +0900 Subject: [PATCHv7 1/4] libvirt/qemu - persistent modification of devices.
Now, qemudDomainAttachDeviceFlags() and qemudDomainDetachDeviceFlags() doesn't support VIR_DOMAIN_DEVICE_MODIFY_CONFIG. By this, virsh's at(de)tatch-device --persistent cannot modify qemu config. (Xen allows it.)
This patch is a base patch for adding support of devices in step by step manner. Following patches will add some device support.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/qemu/qemu_driver.c | 138 +++++++++++++++++++++++++++++++++++++++++++----- 1 files changed, 125 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index af897ad..a4fb1cd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4144,16 +4144,125 @@ cleanup: return ret; }
-static int qemudDomainAttachDeviceFlags(virDomainPtr dom, - const char *xml, - unsigned int flags) { - if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("cannot modify the persistent configuration of a domain")); +/* + * Attach a device given by XML, the change will be persistent + * and domain XML definition file is updated. + */ +static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, + virDomainDeviceDefPtr newdev) +{ + + switch(newdev->type) { + default: + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Sorry, the device is not supported for now")); + return -1; + } + + return 0; +} + +static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef, + virDomainDeviceDefPtr device) +{ + switch(device->type) { + default: + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Sorry, the device is not supported for now")); return -1; } + return 0; +} + +static int qemuDomainModifyDevicePersistent(virDomainPtr dom, + const char *xml, + unsigned int attach, + unsigned int flags) +{ + struct qemud_driver *driver; + virDomainDeviceDefPtr device; + virDomainDefPtr vmdef; + virDomainObjPtr vm; + int ret = -1; + + /* + * When both of MODIFY_CONFIG and MODIFY_LIVE is specified at the same, + * time return error for now. We should support this later.
s/at the same, time/at the same time,/
will fix.
+ */ + if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Now, cannot modify alive domain and its definition "
You use 'alive domain' here, but you use 'active domain' below. The message should be consistent.
will fix.
+ "at the same time.")); + return -1; + } + + driver = dom->conn->privateData; + qemuDriverLock(driver); + vm = virDomainFindByName(&driver->domains, dom->name); + if (!vm) { + qemuReportError(VIR_ERR_NO_DOMAIN, _("no domain with name : '%s'"), + dom->name); + goto unlock_out; + } + + if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) + goto unlock_out; + + if (virDomainObjIsActive(vm)) { + /* + * For now, just allow updating inactive domains. Further development + * will allow updating both active domain and its config file at + * the same time. + */ + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("unsupported to update active domain's definition.")); + goto endjob; + } + + vmdef = virDomainObjGetPersistentDef(driver->caps, vm);
- return qemudDomainAttachDevice(dom, xml); + if (!vmdef) + goto endjob; + + device = virDomainDeviceDefParse(driver->caps, + vmdef, xml, VIR_DOMAIN_XML_INACTIVE); + if (!device) + goto endjob; + + if (attach) + ret = qemuDomainAttachDevicePersistent(vmdef, device); + else + ret = qemuDomainDetachDevicePersistent(vmdef, device); + + if (!ret) /* save the result */ + ret = virDomainSaveConfig(driver->configDir, vmdef);
vmdef may be modified even if ret is not 0, but you do not update the xml file. I do not find any problem about this, but it may be better to update the xml file when ret is not 0.
Hmm ? I'll add a spec. on qemuDomainAt(De)tachDevicePersistent() to never update vmdef when return !0. Is it ok ?
+ + virDomainDeviceDefFree(device); + +endjob: + if (qemuDomainObjEndJob(vm) == 0) + vm = NULL; +unlock_out: + if (vm) + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + return ret; +} + +static int qemudDomainAttachDeviceFlags(virDomainPtr dom, + const char *xml, + unsigned int flags) +{ + if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) + return qemuDomainModifyDevicePersistent(dom, xml, 1, flags); + + if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) + return qemudDomainAttachDevice(dom, xml); + + qemuReportError(VIR_ERR_INVALID_ARG, + _("bad flag: %x not supported"), flags);
If the flags is VIR_DOMAIN_DEVICE_MODIFY_LIVE | unsupported_flags, we do not report error here.
You can use the macro virCheckFlags to check it.
Hm, ok. will see it. Thanks, -Kame
At 03/29/2011 12:32 PM, KAMEZAWA Hiroyuki Write:
On Tue, 29 Mar 2011 11:24:23 +0800 Wen Congyang <wency@cn.fujitsu.com> wrote:
At 03/25/2011 04:10 PM, KAMEZAWA Hiroyuki Write:
This is v7. Dropped patches for Nics and added 2 sanity checks and show correct error messages.
=
From 948597237bd9ecfc5c7343fd30efdca37733274e Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Fri, 25 Mar 2011 16:59:55 +0900 Subject: [PATCHv7 1/4] libvirt/qemu - persistent modification of devices.
Now, qemudDomainAttachDeviceFlags() and qemudDomainDetachDeviceFlags() doesn't support VIR_DOMAIN_DEVICE_MODIFY_CONFIG. By this, virsh's at(de)tatch-device --persistent cannot modify qemu config. (Xen allows it.)
This patch is a base patch for adding support of devices in step by step manner. Following patches will add some device support.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/qemu/qemu_driver.c | 138 +++++++++++++++++++++++++++++++++++++++++++----- 1 files changed, 125 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index af897ad..a4fb1cd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4144,16 +4144,125 @@ cleanup: return ret; }
-static int qemudDomainAttachDeviceFlags(virDomainPtr dom, - const char *xml, - unsigned int flags) { - if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("cannot modify the persistent configuration of a domain")); +/* + * Attach a device given by XML, the change will be persistent + * and domain XML definition file is updated. + */ +static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, + virDomainDeviceDefPtr newdev) +{ + + switch(newdev->type) { + default: + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Sorry, the device is not supported for now")); + return -1; + } + + return 0; +} + +static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef, + virDomainDeviceDefPtr device) +{ + switch(device->type) { + default: + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Sorry, the device is not supported for now")); return -1; } + return 0; +} + +static int qemuDomainModifyDevicePersistent(virDomainPtr dom, + const char *xml, + unsigned int attach, + unsigned int flags) +{ + struct qemud_driver *driver; + virDomainDeviceDefPtr device; + virDomainDefPtr vmdef; + virDomainObjPtr vm; + int ret = -1; + + /* + * When both of MODIFY_CONFIG and MODIFY_LIVE is specified at the same, + * time return error for now. We should support this later.
s/at the same, time/at the same time,/
will fix.
+ */ + if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Now, cannot modify alive domain and its definition "
You use 'alive domain' here, but you use 'active domain' below. The message should be consistent.
will fix.
+ "at the same time.")); + return -1; + } + + driver = dom->conn->privateData; + qemuDriverLock(driver); + vm = virDomainFindByName(&driver->domains, dom->name); + if (!vm) { + qemuReportError(VIR_ERR_NO_DOMAIN, _("no domain with name : '%s'"), + dom->name); + goto unlock_out; + } + + if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) + goto unlock_out; + + if (virDomainObjIsActive(vm)) { + /* + * For now, just allow updating inactive domains. Further development + * will allow updating both active domain and its config file at + * the same time. + */ + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("unsupported to update active domain's definition.")); + goto endjob; + } + + vmdef = virDomainObjGetPersistentDef(driver->caps, vm);
- return qemudDomainAttachDevice(dom, xml); + if (!vmdef) + goto endjob; + + device = virDomainDeviceDefParse(driver->caps, + vmdef, xml, VIR_DOMAIN_XML_INACTIVE); + if (!device) + goto endjob; + + if (attach) + ret = qemuDomainAttachDevicePersistent(vmdef, device); + else + ret = qemuDomainDetachDevicePersistent(vmdef, device); + + if (!ret) /* save the result */ + ret = virDomainSaveConfig(driver->configDir, vmdef);
vmdef may be modified even if ret is not 0, but you do not update the xml file. I do not find any problem about this, but it may be better to update the xml file when ret is not 0.
Hmm ? I'll add a spec. on qemuDomainAt(De)tachDevicePersistent() to never update vmdef when return !0. Is it ok ?
No. In patch 2, the function qemuDomainDeviceAddressFixup() may modify vmdef and return -1: ============= +static int qemuDomainDeviceAddressFixup(virDomainDefPtr vmdef, bool pci) +{ + if (!pci && virDomainDefAddImplicitControllers(vmdef)) + return -1; + /* added controller requires PCI address */ + return qemuDomainAssignPCIAddresses(vmdef); +} + ============= The function virDomainDefAddImplicitControllers() may modify vmdef. If qemuDomainAssignPCIAddresses() failed, there is no way to rollback the operation that virDomainDefAddImplicitControllers() do.
+ + virDomainDeviceDefFree(device); + +endjob: + if (qemuDomainObjEndJob(vm) == 0) + vm = NULL; +unlock_out: + if (vm) + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + return ret; +} + +static int qemudDomainAttachDeviceFlags(virDomainPtr dom, + const char *xml, + unsigned int flags) +{ + if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) + return qemuDomainModifyDevicePersistent(dom, xml, 1, flags); + + if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) + return qemudDomainAttachDevice(dom, xml); + + qemuReportError(VIR_ERR_INVALID_ARG, + _("bad flag: %x not supported"), flags);
If the flags is VIR_DOMAIN_DEVICE_MODIFY_LIVE | unsupported_flags, we do not report error here.
You can use the macro virCheckFlags to check it.
Hm, ok. will see it.
Thanks, -Kame
On Tue, 29 Mar 2011 13:17:55 +0800 Wen Congyang <wency@cn.fujitsu.com> wrote:
At 03/29/2011 12:32 PM, KAMEZAWA Hiroyuki Write:
On Tue, 29 Mar 2011 11:24:23 +0800 Wen Congyang <wency@cn.fujitsu.com> wrote:
Hmm ? I'll add a spec. on qemuDomainAt(De)tachDevicePersistent() to never update vmdef when return !0. Is it ok ?
No. In patch 2, the function qemuDomainDeviceAddressFixup() may modify vmdef and return -1: ============= +static int qemuDomainDeviceAddressFixup(virDomainDefPtr vmdef, bool pci) +{ + if (!pci && virDomainDefAddImplicitControllers(vmdef)) + return -1; + /* added controller requires PCI address */ + return qemuDomainAssignPCIAddresses(vmdef); +} + ============= The function virDomainDefAddImplicitControllers() may modify vmdef. If qemuDomainAssignPCIAddresses() failed, there is no way to rollback the operation that virDomainDefAddImplicitControllers() do.
Of course, I'll add a rollback. That's a bug in patch'2', not here. Thanks, -Kame
participants (3)
-
Hu Tao -
KAMEZAWA Hiroyuki -
Wen Congyang