[libvirt] [PATCHv5 1/3] libivrt/qemu - support persistent modification of devices
Thank you for review and feedbacks. This is v5 onto the latest git tree. Any comments are welcome. ==
From ef85d268a0b553cdb784a1d1916ff8cf171adace Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Wed, 16 Mar 2011 11:35:41 +0900 Subject: [PATCHv5 1/3] libivrt/qemu - support 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. Changelog v4->v5: - fixed error codes at reporting Error. - fixed unlock code after calling qemuDomainObjBeginJobWithDriver() - fixed virDomainFindByUUID() to be virDomainFindByName() - fixed spelling, indent, etc.. - removed unnecessary checks. Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/qemu/qemu_driver.c | 141 +++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 129 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index dac2bf2..dbd5bd3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4130,16 +4130,129 @@ cleanup: return ret; } -static int qemudDomainAttachDeviceFlags(virDomainPtr dom, +/* + * 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 flags) { - if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("cannot modify the persistent configuration of a domain")); + unsigned int attach, unsigned int flags) +{ + struct qemud_driver *driver; + virDomainDeviceDefPtr device; + virDomainDefPtr vmdef; + virDomainObjPtr vm; + int ret = -1; + + if (!xml) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("internal error")); return -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", + _("Now, it's unsupported to update active domain's definition.")); + goto endjob; + } + + vmdef = virDomainObjGetPersistentDef(driver->caps, vm); + + 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 only MODIFY_LIVE, MODIFY_CONFIG are supported now"), + flags); - return qemudDomainAttachDevice(dom, xml); + return -1; } @@ -4354,13 +4467,17 @@ 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 only MODIFY_LIVE, MODIFY_CONFIG are supported now"), + flags); + return -1; } static int qemudDomainGetAutostart(virDomainPtr dom, -- 1.7.4.1
From ddcd9cb499dd73dfc91c9f2cc97c064d6dda17fc Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Wed, 16 Mar 2011 14:05:21 +0900 Subject: [PATCHv5 2/3] libvirt/qemu - support persistent modification of qemu disks
support changes of disks by VIR_DOMAIN_DEVICE_MODIFY_CONFIG for qemu. Changelog v4->v5: - moved some functions to domain_conf.c - added virDomainDefAddImplicitControllers() for ide/scsi - report OOM. - clean up. Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/conf/domain_conf.c | 22 ++++++++++++++++++++++ src/conf/domain_conf.h | 3 ++- src/libvirt_private.syms | 2 ++ src/qemu/qemu_driver.c | 29 +++++++++++++++++++++++++++++ 4 files changed, 55 insertions(+), 1 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 16e1291..1d39481 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 30aeccc..320ca13 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1261,7 +1261,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, @@ -1269,6 +1269,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 c88d934..1761dfb 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 dbd5bd3..d6d7ad0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4137,8 +4137,27 @@ cleanup: static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, virDomainDeviceDefPtr newdev) { + virDomainDiskDefPtr disk; 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; + } + if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) { + if (qemuDomainAssignPCIAddresses(vmdef) < 0) + return -1; + } else if (virDomainDefAddImplicitControllers(vmdef) < 0) + return -1; + newdev->data.disk = NULL; + break; default: qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Sorry, the device is not supported for now")); @@ -4151,7 +4170,17 @@ static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, 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
At 03/16/2011 02:45 PM, KAMEZAWA Hiroyuki Write:
From ddcd9cb499dd73dfc91c9f2cc97c064d6dda17fc Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Wed, 16 Mar 2011 14:05:21 +0900 Subject: [PATCHv5 2/3] libvirt/qemu - support persistent modification of qemu disks
support changes of disks by VIR_DOMAIN_DEVICE_MODIFY_CONFIG for qemu.
Changelog v4->v5: - moved some functions to domain_conf.c - added virDomainDefAddImplicitControllers() for ide/scsi - report OOM. - clean up.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/conf/domain_conf.c | 22 ++++++++++++++++++++++ src/conf/domain_conf.h | 3 ++- src/libvirt_private.syms | 2 ++ src/qemu/qemu_driver.c | 29 +++++++++++++++++++++++++++++ 4 files changed, 55 insertions(+), 1 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 16e1291..1d39481 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 30aeccc..320ca13 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1261,7 +1261,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, @@ -1269,6 +1269,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 c88d934..1761dfb 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 dbd5bd3..d6d7ad0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4137,8 +4137,27 @@ cleanup: static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, virDomainDeviceDefPtr newdev) { + virDomainDiskDefPtr disk;
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; + } + if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) { + if (qemuDomainAssignPCIAddresses(vmdef) < 0) + return -1; + } else if (virDomainDefAddImplicitControllers(vmdef) < 0) + return -1;
When qemuDomainAssignPCIAddresses() or virDomainDefAddImplicitControllers() failed, we should remove disk from vmdef, otherwise, it will cause libvirtd crashed.
+ newdev->data.disk = NULL; + break; default: qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Sorry, the device is not supported for now")); @@ -4151,7 +4170,17 @@ static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, 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"));
On Mon, 21 Mar 2011 16:05:19 +0800 Wen Congyang <wency@cn.fujitsu.com> wrote:
At 03/16/2011 02:45 PM, KAMEZAWA Hiroyuki Write:
From ddcd9cb499dd73dfc91c9f2cc97c064d6dda17fc Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Wed, 16 Mar 2011 14:05:21 +0900 Subject: [PATCHv5 2/3] libvirt/qemu - support persistent modification of qemu disks
support changes of disks by VIR_DOMAIN_DEVICE_MODIFY_CONFIG for qemu.
Changelog v4->v5: - moved some functions to domain_conf.c - added virDomainDefAddImplicitControllers() for ide/scsi - report OOM. - clean up.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/conf/domain_conf.c | 22 ++++++++++++++++++++++ src/conf/domain_conf.h | 3 ++- src/libvirt_private.syms | 2 ++ src/qemu/qemu_driver.c | 29 +++++++++++++++++++++++++++++ 4 files changed, 55 insertions(+), 1 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 16e1291..1d39481 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 30aeccc..320ca13 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1261,7 +1261,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, @@ -1269,6 +1269,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 c88d934..1761dfb 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 dbd5bd3..d6d7ad0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4137,8 +4137,27 @@ cleanup: static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, virDomainDeviceDefPtr newdev) { + virDomainDiskDefPtr disk;
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; + } + if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) { + if (qemuDomainAssignPCIAddresses(vmdef) < 0) + return -1; + } else if (virDomainDefAddImplicitControllers(vmdef) < 0) + return -1;
When qemuDomainAssignPCIAddresses() or virDomainDefAddImplicitControllers() failed, we should remove disk from vmdef, otherwise, it will cause libvirtd crashed.
By returning -1, the vmdef is not saved and no one will see this modified vmdef (because the task is inactive here.) Do I need to care ? Thanks, -Kame
At 03/23/2011 12:39 PM, KAMEZAWA Hiroyuki Write:
On Mon, 21 Mar 2011 16:05:19 +0800 Wen Congyang <wency@cn.fujitsu.com> wrote:
At 03/16/2011 02:45 PM, KAMEZAWA Hiroyuki Write:
From ddcd9cb499dd73dfc91c9f2cc97c064d6dda17fc Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Wed, 16 Mar 2011 14:05:21 +0900 Subject: [PATCHv5 2/3] libvirt/qemu - support persistent modification of qemu disks
support changes of disks by VIR_DOMAIN_DEVICE_MODIFY_CONFIG for qemu.
Changelog v4->v5: - moved some functions to domain_conf.c - added virDomainDefAddImplicitControllers() for ide/scsi - report OOM. - clean up.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/conf/domain_conf.c | 22 ++++++++++++++++++++++ src/conf/domain_conf.h | 3 ++- src/libvirt_private.syms | 2 ++ src/qemu/qemu_driver.c | 29 +++++++++++++++++++++++++++++ 4 files changed, 55 insertions(+), 1 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 16e1291..1d39481 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 30aeccc..320ca13 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1261,7 +1261,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, @@ -1269,6 +1269,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 c88d934..1761dfb 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 dbd5bd3..d6d7ad0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4137,8 +4137,27 @@ cleanup: static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, virDomainDeviceDefPtr newdev) { + virDomainDiskDefPtr disk;
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; + } + if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) { + if (qemuDomainAssignPCIAddresses(vmdef) < 0) + return -1; + } else if (virDomainDefAddImplicitControllers(vmdef) < 0) + return -1;
When qemuDomainAssignPCIAddresses() or virDomainDefAddImplicitControllers() failed, we should remove disk from vmdef, otherwise, it will cause libvirtd crashed.
By returning -1, the vmdef is not saved and no one will see this modified vmdef (because the task is inactive here.)
Do I need to care ?
Yes, you need to care it. By returning -1, the vmdef is not saved to disk. But vmdef is hashed, and we may access it again before libvirtd is restarted. When stoping libvirtd, we will free vmdef, this will cause disk is double freed. Here is the steps to reproduce this program: # cat device.xml <disk type='file' device='disk'> <driver name='qemu' type='qcow2'/> <source file='/var/lib/libvirt/images/test3.img'/> <target dev='vdb' bus='virtio'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> </disk> # virsh attach-device vm1 device.xml --persistent error: Failed to attach device from device.xml error: An error occurred, but the cause is unknown Now, run attach-device again or stop libvirtd. It will cause libvirtd crashed.
Thanks, -Kame
On Wed, 23 Mar 2011 13:15:39 +0800 Wen Congyang <wency@cn.fujitsu.com> wrote:
At 03/23/2011 12:39 PM, KAMEZAWA Hiroyuki Write:
On Mon, 21 Mar 2011 16:05:19 +0800 Wen Congyang <wency@cn.fujitsu.com> wrote:
At 03/16/2011 02:45 PM, KAMEZAWA Hiroyuki Write:
From ddcd9cb499dd73dfc91c9f2cc97c064d6dda17fc Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Wed, 16 Mar 2011 14:05:21 +0900 Subject: [PATCHv5 2/3] libvirt/qemu - support persistent modification of qemu disks
support changes of disks by VIR_DOMAIN_DEVICE_MODIFY_CONFIG for qemu.
Changelog v4->v5: - moved some functions to domain_conf.c - added virDomainDefAddImplicitControllers() for ide/scsi - report OOM. - clean up.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/conf/domain_conf.c | 22 ++++++++++++++++++++++ src/conf/domain_conf.h | 3 ++- src/libvirt_private.syms | 2 ++ src/qemu/qemu_driver.c | 29 +++++++++++++++++++++++++++++ 4 files changed, 55 insertions(+), 1 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 16e1291..1d39481 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 30aeccc..320ca13 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1261,7 +1261,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, @@ -1269,6 +1269,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 c88d934..1761dfb 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 dbd5bd3..d6d7ad0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4137,8 +4137,27 @@ cleanup: static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, virDomainDeviceDefPtr newdev) { + virDomainDiskDefPtr disk;
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; + } + if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) { + if (qemuDomainAssignPCIAddresses(vmdef) < 0) + return -1; + } else if (virDomainDefAddImplicitControllers(vmdef) < 0) + return -1;
When qemuDomainAssignPCIAddresses() or virDomainDefAddImplicitControllers() failed, we should remove disk from vmdef, otherwise, it will cause libvirtd crashed.
By returning -1, the vmdef is not saved and no one will see this modified vmdef (because the task is inactive here.)
Do I need to care ?
Yes, you need to care it. By returning -1, the vmdef is not saved to disk. But vmdef is hashed, and we may access it again before libvirtd is restarted. When stoping libvirtd, we will free vmdef, this will cause disk is double freed.
Here is the steps to reproduce this program: # cat device.xml <disk type='file' device='disk'> <driver name='qemu' type='qcow2'/> <source file='/var/lib/libvirt/images/test3.img'/> <target dev='vdb' bus='virtio'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> </disk> # virsh attach-device vm1 device.xml --persistent error: Failed to attach device from device.xml error: An error occurred, but the cause is unknown
Now, run attach-device again or stop libvirtd. It will cause libvirtd crashed.
Hmm, ok. what I should to unahs and free vmdef ? Thanks, -Kame
At 03/23/2011 01:22 PM, KAMEZAWA Hiroyuki Write:
On Wed, 23 Mar 2011 13:15:39 +0800 Wen Congyang <wency@cn.fujitsu.com> wrote:
At 03/23/2011 12:39 PM, KAMEZAWA Hiroyuki Write:
On Mon, 21 Mar 2011 16:05:19 +0800 Wen Congyang <wency@cn.fujitsu.com> wrote:
At 03/16/2011 02:45 PM, KAMEZAWA Hiroyuki Write:
From ddcd9cb499dd73dfc91c9f2cc97c064d6dda17fc Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Wed, 16 Mar 2011 14:05:21 +0900 Subject: [PATCHv5 2/3] libvirt/qemu - support persistent modification of qemu disks
support changes of disks by VIR_DOMAIN_DEVICE_MODIFY_CONFIG for qemu.
Changelog v4->v5: - moved some functions to domain_conf.c - added virDomainDefAddImplicitControllers() for ide/scsi - report OOM. - clean up.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/conf/domain_conf.c | 22 ++++++++++++++++++++++ src/conf/domain_conf.h | 3 ++- src/libvirt_private.syms | 2 ++ src/qemu/qemu_driver.c | 29 +++++++++++++++++++++++++++++ 4 files changed, 55 insertions(+), 1 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 16e1291..1d39481 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 30aeccc..320ca13 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1261,7 +1261,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, @@ -1269,6 +1269,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 c88d934..1761dfb 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 dbd5bd3..d6d7ad0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4137,8 +4137,27 @@ cleanup: static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, virDomainDeviceDefPtr newdev) { + virDomainDiskDefPtr disk;
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; + } + if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) { + if (qemuDomainAssignPCIAddresses(vmdef) < 0) + return -1; + } else if (virDomainDefAddImplicitControllers(vmdef) < 0) + return -1;
When qemuDomainAssignPCIAddresses() or virDomainDefAddImplicitControllers() failed, we should remove disk from vmdef, otherwise, it will cause libvirtd crashed.
By returning -1, the vmdef is not saved and no one will see this modified vmdef (because the task is inactive here.)
Do I need to care ?
Yes, you need to care it. By returning -1, the vmdef is not saved to disk. But vmdef is hashed, and we may access it again before libvirtd is restarted. When stoping libvirtd, we will free vmdef, this will cause disk is double freed.
Here is the steps to reproduce this program: # cat device.xml <disk type='file' device='disk'> <driver name='qemu' type='qcow2'/> <source file='/var/lib/libvirt/images/test3.img'/> <target dev='vdb' bus='virtio'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> </disk> # virsh attach-device vm1 device.xml --persistent error: Failed to attach device from device.xml error: An error occurred, but the cause is unknown
Now, run attach-device again or stop libvirtd. It will cause libvirtd crashed.
Hmm, ok. what I should to unahs and free vmdef ?
No, we hash vmdef if it is persistent. We should do some cleanup when qemuDomainAssignPCIAddresses() or virDomainDefAddImplicitControllers() failed. For example, calling virDomainDiskRemove() to remove it. Another way to solve this problem is: we only malloc the memory and do not insert the disk to vmdef before calling qemuDomainAssignPCIAddresses() or virDomainDefAddImplicitControllers(), and insert it when this function successed.
Thanks, -Kame
From d69f7c07571c8adfc7720aab77aa1f103eb6e95b Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Wed, 16 Mar 2011 15:39:14 +0900 Subject: [PATCHv5 3/3] libvirt/qemu - support support persistent modification of qemu interfaces.
support changes of interfaces by VIR_DOMAIN_DEVICE_MODIFY_CONFIG for qemu. Changelog v4->v5 - fixed mac handling. - moved some functions to domain_conf.c - clean up. Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/conf/domain_conf.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 5 ++++ src/libvirt_private.syms | 4 +++ src/qemu/qemu_driver.c | 38 +++++++++++++++++++++++++++++++ 4 files changed, 103 insertions(+), 0 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1d39481..9e4dea6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4952,6 +4952,62 @@ int virDomainDiskRemoveByName(virDomainDefPtr def, const char *name) return 0; } +int virDomainNetInsert(virDomainDefPtr def, virDomainNetDefPtr net) +{ + if (VIR_REALLOC_N(def->nets, def->nnets) < 0) + return -1; + def->nets[def->nnets] = net; + def->nnets++; + return 0; +} + +int virDomainNetIndexByMac(virDomainDefPtr def, const unsigned char*mac) +{ + int i; + + for (i = 0; i < def->nnets; i++) + if (memcmp(def->nets[i]->mac, mac, VIR_MAC_BUFLEN)) + return i; + return -1; +} + +int virDomainNetIndexByIfname(virDomainDefPtr def, const char*name) +{ + int i; + + for (i = 0; i < def->nnets; i++) { + if (def->nets[i]->ifname && STREQ(def->nets[i]->ifname, name)) + return i; + } + return -1; +} + +static void virDomainNetRemove(virDomainDefPtr def, size_t i) +{ + if (def->nnets > 1) { + memmove(def->nets + i, + def->nets + i + 1, + sizeof(*def->nets) * (def->nnets - (i + 1))); + def->nnets--; + if (VIR_REALLOC_N(def->nets, def->nnets) < 0) { + /* ignore harmless */ + } + } else { + VIR_FREE(def->nets); + def->nnets = 0; + } +} + +int virDomainNetRemoveByMac(virDomainDefPtr def, const unsigned char *mac) +{ + int i; + + i = virDomainNetIndexByMac(def, mac); + if (i < 0) + return -1; + virDomainNetRemove(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 320ca13..d5154f6 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1271,6 +1271,11 @@ int virDomainDiskDefAssignAddress(virCapsPtr caps, virDomainDiskDefPtr def); void virDomainDiskRemove(virDomainDefPtr def, size_t i); int virDomainDiskRemoveByName(virDomainDefPtr def, const char *name); +int virDomainNetIndexByIfname(virDomainDefPtr def, const char *ifname); +int virDomainNetIndexByMac(virDomainDefPtr def, const unsigned char *mac); +int virDomainNetInsert(virDomainDefPtr dev, virDomainNetDefPtr net); +int virDomainNetRemoveByMac(virDomainDefPtr def, const unsigned char *mac); + int virDomainControllerInsert(virDomainDefPtr def, virDomainControllerDefPtr controller); void virDomainControllerInsertPreAlloced(virDomainDefPtr def, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1761dfb..cdc63db 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -277,6 +277,10 @@ virDomainMemballoonModelTypeFromString; virDomainMemballoonModelTypeToString; virDomainNetDefFree; virDomainNetTypeToString; +virDomainNetIndexByIfname; +virDomainNetIndexByMac; +virDomainNetInsert; +virDomainNetRemoveByMac; virDomainObjAssignDef; virDomainObjSetDefTransient; virDomainObjGetPersistentDef; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d6d7ad0..45b1bf0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4138,6 +4138,7 @@ static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, virDomainDeviceDefPtr newdev) { virDomainDiskDefPtr disk; + virDomainNetDefPtr net; switch(newdev->type) { case VIR_DOMAIN_DEVICE_DISK: @@ -4158,6 +4159,31 @@ static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, return -1; newdev->data.disk = NULL; break; + case VIR_DOMAIN_DEVICE_NET: + net = newdev->data.net; + /* check mac */ + if (virDomainNetIndexByMac(vmdef, net->mac) >= 0) { + char macbuf[VIR_MAC_STRING_BUFLEN]; + + virFormatMacAddr(net->mac, macbuf); + qemuReportError(VIR_ERR_INVALID_ARG, + _("target mac %s already exists."), macbuf); + return -1; + } + /* Sanity check : test ifname confliction. */ + if (net->ifname && virDomainNetIndexByIfname(vmdef, net->ifname) >= 0) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("target NIC %s already exists."), net->ifname); + return -1; + } + if (virDomainNetInsert(vmdef, net)) { + virReportOOMError(); + return -1; + } + if (qemuDomainAssignPCIAddresses(vmdef) < 0) + return -1; + newdev->data.net = NULL; + break; default: qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Sorry, the device is not supported for now")); @@ -4171,6 +4197,7 @@ static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef, virDomainDeviceDefPtr device) { virDomainDiskDefPtr disk; + virDomainNetDefPtr net; switch(device->type) { case VIR_DOMAIN_DEVICE_DISK: @@ -4181,6 +4208,17 @@ static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef, return -1; } break; + case VIR_DOMAIN_DEVICE_NET: + net = device->data.net; + if (virDomainNetRemoveByMac(vmdef, net->mac)) { + char macbuf[VIR_MAC_STRING_BUFLEN]; + + virFormatMacAddr(net->mac, macbuf); + qemuReportError(VIR_ERR_INVALID_ARG, + _("no interface with mac %s"), macbuf); + return -1; + } + break; default: qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Sorry, the device is not supported for now")); -- 1.7.4.1
At 03/16/2011 02:46 PM, KAMEZAWA Hiroyuki Write:
From d69f7c07571c8adfc7720aab77aa1f103eb6e95b Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Wed, 16 Mar 2011 15:39:14 +0900 Subject: [PATCHv5 3/3] libvirt/qemu - support support persistent modification of qemu interfaces.
support changes of interfaces by VIR_DOMAIN_DEVICE_MODIFY_CONFIG for qemu.
Changelog v4->v5 - fixed mac handling. - moved some functions to domain_conf.c - clean up.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/conf/domain_conf.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 5 ++++ src/libvirt_private.syms | 4 +++ src/qemu/qemu_driver.c | 38 +++++++++++++++++++++++++++++++ 4 files changed, 103 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1d39481..9e4dea6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4952,6 +4952,62 @@ int virDomainDiskRemoveByName(virDomainDefPtr def, const char *name) return 0; }
+int virDomainNetInsert(virDomainDefPtr def, virDomainNetDefPtr net) +{ + if (VIR_REALLOC_N(def->nets, def->nnets) < 0) + return -1; + def->nets[def->nnets] = net; + def->nnets++; + return 0; +} + +int virDomainNetIndexByMac(virDomainDefPtr def, const unsigned char*mac) +{ + int i; + + for (i = 0; i < def->nnets; i++) + if (memcmp(def->nets[i]->mac, mac, VIR_MAC_BUFLEN)) + return i; + return -1; +} + +int virDomainNetIndexByIfname(virDomainDefPtr def, const char*name) +{ + int i; + + for (i = 0; i < def->nnets; i++) { + if (def->nets[i]->ifname && STREQ(def->nets[i]->ifname, name)) + return i; + } + return -1; +} + +static void virDomainNetRemove(virDomainDefPtr def, size_t i) +{ + if (def->nnets > 1) { + memmove(def->nets + i, + def->nets + i + 1, + sizeof(*def->nets) * (def->nnets - (i + 1))); + def->nnets--; + if (VIR_REALLOC_N(def->nets, def->nnets) < 0) { + /* ignore harmless */ + } + } else { + VIR_FREE(def->nets); + def->nnets = 0; + } +} + +int virDomainNetRemoveByMac(virDomainDefPtr def, const unsigned char *mac) +{ + int i; + + i = virDomainNetIndexByMac(def, mac); + if (i < 0) + return -1; + virDomainNetRemove(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 320ca13..d5154f6 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1271,6 +1271,11 @@ int virDomainDiskDefAssignAddress(virCapsPtr caps, virDomainDiskDefPtr def); void virDomainDiskRemove(virDomainDefPtr def, size_t i); int virDomainDiskRemoveByName(virDomainDefPtr def, const char *name);
+int virDomainNetIndexByIfname(virDomainDefPtr def, const char *ifname); +int virDomainNetIndexByMac(virDomainDefPtr def, const unsigned char *mac); +int virDomainNetInsert(virDomainDefPtr dev, virDomainNetDefPtr net); +int virDomainNetRemoveByMac(virDomainDefPtr def, const unsigned char *mac); + int virDomainControllerInsert(virDomainDefPtr def, virDomainControllerDefPtr controller); void virDomainControllerInsertPreAlloced(virDomainDefPtr def, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1761dfb..cdc63db 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -277,6 +277,10 @@ virDomainMemballoonModelTypeFromString; virDomainMemballoonModelTypeToString; virDomainNetDefFree; virDomainNetTypeToString; +virDomainNetIndexByIfname; +virDomainNetIndexByMac; +virDomainNetInsert; +virDomainNetRemoveByMac; virDomainObjAssignDef; virDomainObjSetDefTransient; virDomainObjGetPersistentDef; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d6d7ad0..45b1bf0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4138,6 +4138,7 @@ static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, virDomainDeviceDefPtr newdev) { virDomainDiskDefPtr disk; + virDomainNetDefPtr net;
switch(newdev->type) { case VIR_DOMAIN_DEVICE_DISK: @@ -4158,6 +4159,31 @@ static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, return -1; newdev->data.disk = NULL; break; + case VIR_DOMAIN_DEVICE_NET: + net = newdev->data.net; + /* check mac */ + if (virDomainNetIndexByMac(vmdef, net->mac) >= 0) { + char macbuf[VIR_MAC_STRING_BUFLEN]; + + virFormatMacAddr(net->mac, macbuf); + qemuReportError(VIR_ERR_INVALID_ARG, + _("target mac %s already exists."), macbuf); + return -1; + } + /* Sanity check : test ifname confliction. */ + if (net->ifname && virDomainNetIndexByIfname(vmdef, net->ifname) >= 0) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("target NIC %s already exists."), net->ifname); + return -1; + } + if (virDomainNetInsert(vmdef, net)) { + virReportOOMError(); + return -1; + } + if (qemuDomainAssignPCIAddresses(vmdef) < 0) + return -1;
When qemuDomainAssignPCIAddresses() failed, we should remove net from vmdef, otherwise, it will cause libvirtd crashed.
+ newdev->data.net = NULL; + break; default: qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Sorry, the device is not supported for now")); @@ -4171,6 +4197,7 @@ static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef, virDomainDeviceDefPtr device) { virDomainDiskDefPtr disk; + virDomainNetDefPtr net;
switch(device->type) { case VIR_DOMAIN_DEVICE_DISK: @@ -4181,6 +4208,17 @@ static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef, return -1; } break; + case VIR_DOMAIN_DEVICE_NET: + net = device->data.net; + if (virDomainNetRemoveByMac(vmdef, net->mac)) { + char macbuf[VIR_MAC_STRING_BUFLEN]; + + virFormatMacAddr(net->mac, macbuf); + qemuReportError(VIR_ERR_INVALID_ARG, + _("no interface with mac %s"), macbuf); + return -1; + } + break; default: qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Sorry, the device is not supported for now"));
On Mon, 21 Mar 2011 16:48:23 +0800 Wen Congyang <wency@cn.fujitsu.com> wrote:
At 03/16/2011 02:46 PM, KAMEZAWA Hiroyuki Write:
From d69f7c07571c8adfc7720aab77aa1f103eb6e95b Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Wed, 16 Mar 2011 15:39:14 +0900 Subject: [PATCHv5 3/3] libvirt/qemu - support support persistent modification of qemu interfaces.
support changes of interfaces by VIR_DOMAIN_DEVICE_MODIFY_CONFIG for qemu.
Changelog v4->v5 - fixed mac handling. - moved some functions to domain_conf.c - clean up.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/conf/domain_conf.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 5 ++++ src/libvirt_private.syms | 4 +++ src/qemu/qemu_driver.c | 38 +++++++++++++++++++++++++++++++ 4 files changed, 103 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1d39481..9e4dea6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4952,6 +4952,62 @@ int virDomainDiskRemoveByName(virDomainDefPtr def, const char *name) return 0; }
+int virDomainNetInsert(virDomainDefPtr def, virDomainNetDefPtr net) +{ + if (VIR_REALLOC_N(def->nets, def->nnets) < 0) + return -1; + def->nets[def->nnets] = net; + def->nnets++; + return 0; +} + +int virDomainNetIndexByMac(virDomainDefPtr def, const unsigned char*mac) +{ + int i; + + for (i = 0; i < def->nnets; i++) + if (memcmp(def->nets[i]->mac, mac, VIR_MAC_BUFLEN)) + return i; + return -1; +} + +int virDomainNetIndexByIfname(virDomainDefPtr def, const char*name) +{ + int i; + + for (i = 0; i < def->nnets; i++) { + if (def->nets[i]->ifname && STREQ(def->nets[i]->ifname, name)) + return i; + } + return -1; +} + +static void virDomainNetRemove(virDomainDefPtr def, size_t i) +{ + if (def->nnets > 1) { + memmove(def->nets + i, + def->nets + i + 1, + sizeof(*def->nets) * (def->nnets - (i + 1))); + def->nnets--; + if (VIR_REALLOC_N(def->nets, def->nnets) < 0) { + /* ignore harmless */ + } + } else { + VIR_FREE(def->nets); + def->nnets = 0; + } +} + +int virDomainNetRemoveByMac(virDomainDefPtr def, const unsigned char *mac) +{ + int i; + + i = virDomainNetIndexByMac(def, mac); + if (i < 0) + return -1; + virDomainNetRemove(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 320ca13..d5154f6 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1271,6 +1271,11 @@ int virDomainDiskDefAssignAddress(virCapsPtr caps, virDomainDiskDefPtr def); void virDomainDiskRemove(virDomainDefPtr def, size_t i); int virDomainDiskRemoveByName(virDomainDefPtr def, const char *name);
+int virDomainNetIndexByIfname(virDomainDefPtr def, const char *ifname); +int virDomainNetIndexByMac(virDomainDefPtr def, const unsigned char *mac); +int virDomainNetInsert(virDomainDefPtr dev, virDomainNetDefPtr net); +int virDomainNetRemoveByMac(virDomainDefPtr def, const unsigned char *mac); + int virDomainControllerInsert(virDomainDefPtr def, virDomainControllerDefPtr controller); void virDomainControllerInsertPreAlloced(virDomainDefPtr def, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1761dfb..cdc63db 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -277,6 +277,10 @@ virDomainMemballoonModelTypeFromString; virDomainMemballoonModelTypeToString; virDomainNetDefFree; virDomainNetTypeToString; +virDomainNetIndexByIfname; +virDomainNetIndexByMac; +virDomainNetInsert; +virDomainNetRemoveByMac; virDomainObjAssignDef; virDomainObjSetDefTransient; virDomainObjGetPersistentDef; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d6d7ad0..45b1bf0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4138,6 +4138,7 @@ static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, virDomainDeviceDefPtr newdev) { virDomainDiskDefPtr disk; + virDomainNetDefPtr net;
switch(newdev->type) { case VIR_DOMAIN_DEVICE_DISK: @@ -4158,6 +4159,31 @@ static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, return -1; newdev->data.disk = NULL; break; + case VIR_DOMAIN_DEVICE_NET: + net = newdev->data.net; + /* check mac */ + if (virDomainNetIndexByMac(vmdef, net->mac) >= 0) { + char macbuf[VIR_MAC_STRING_BUFLEN]; + + virFormatMacAddr(net->mac, macbuf); + qemuReportError(VIR_ERR_INVALID_ARG, + _("target mac %s already exists."), macbuf); + return -1; + } + /* Sanity check : test ifname confliction. */ + if (net->ifname && virDomainNetIndexByIfname(vmdef, net->ifname) >= 0) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("target NIC %s already exists."), net->ifname); + return -1; + } + if (virDomainNetInsert(vmdef, net)) { + virReportOOMError(); + return -1; + } + if (qemuDomainAssignPCIAddresses(vmdef) < 0) + return -1;
When qemuDomainAssignPCIAddresses() failed, we should remove net from vmdef, otherwise, it will cause libvirtd crashed.
This vmdef will be just freed after this. Do I need care ? Thanks, -Kame
Hi KAMEZAWA-San, I found patch 1 of your series can be shared by code updating devices persistently, with a slight change. (See my patch 1, I think it can be squashed into your patch 1 if you feel good with it) This series adds support for persistently updating devices, however, it is based on KAMEZAWA-San's series. I post it here for an early review and will update it later. Hu Tao (4): Refine parameter `attach' of qemuDomainModifyDevicePersistent Add a new action UPDATE_DEVICE Add qemuDomainUpdateDevice Add support of updating device persistently src/qemu/qemu_driver.c | 75 ++++++++++++++++++++++++++++++++++------------- 1 files changed, 54 insertions(+), 21 deletions(-) -- 1.7.3.1 -- Thanks, Hu Tao
This patch renames parameter `attach' to `action', and adds two macros for it. Then we can easily add more macros in the furture if needed. --- src/qemu/qemu_driver.c | 21 ++++++++++++++++----- 1 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 840d76d..f33882a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4237,9 +4237,15 @@ static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef, return 0; } +/* XXX: better names are requested */ +#define ATTACH_DEVICE 0 /* Attach device persistently, device must not + * exist before attach. */ +#define DETACH_DEVICE 1 /* Detach device persistently, device must exist + * before detach. */ + static int qemuDomainModifyDevicePersistent(virDomainPtr dom, const char *xml, - unsigned int attach, unsigned int flags) + unsigned int action, unsigned int flags) { struct qemud_driver *driver; virDomainDeviceDefPtr device; @@ -4295,10 +4301,15 @@ static int qemuDomainModifyDevicePersistent(virDomainPtr dom, if (!device) goto endjob; - if (attach) + if (action == ATTACH_DEVICE) ret = qemuDomainAttachDevicePersistent(vmdef, device); - else + else if (action == DETACH_DEVICE) ret = qemuDomainDetachDevicePersistent(vmdef, device); + else { + qemuReportError(VIR_ERR_INVALID_ARG, "%s %d", + _("Unknown action"), action); + ret = -1; + } if (!ret) /* save the result */ ret = virDomainSaveConfig(driver->configDir, vmdef); @@ -4320,7 +4331,7 @@ static int qemudDomainAttachDeviceFlags(virDomainPtr dom, unsigned int flags) { if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) - return qemuDomainModifyDevicePersistent(dom, xml, 1, flags); + return qemuDomainModifyDevicePersistent(dom, xml, ATTACH_DEVICE, flags); if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) return qemudDomainAttachDevice(dom, xml); @@ -4546,7 +4557,7 @@ static int qemudDomainDetachDeviceFlags(virDomainPtr dom, unsigned int flags) { if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) - return qemuDomainModifyDevicePersistent(dom, xml, 0, flags); + return qemuDomainModifyDevicePersistent(dom, xml, DETACH_DEVICE, flags); if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) return qemudDomainDetachDevice(dom, xml); -- 1.7.3.1 -- Thanks, Hu Tao
--- src/qemu/qemu_driver.c | 17 +++++++++++++++++ 1 files changed, 17 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f33882a..98595d1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4237,11 +4237,26 @@ static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef, return 0; } +static int qemuDomainUpdateDevicePersistent(virDomainDefPtr vmdef, + virDomainDeviceDefPtr device) +{ + switch(device->type) { + default: + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Sorry, updating device persistently " + "is not supported for now")); + return -1; + } + return 0; +} + /* XXX: better names are requested */ #define ATTACH_DEVICE 0 /* Attach device persistently, device must not * exist before attach. */ #define DETACH_DEVICE 1 /* Detach device persistently, device must exist * before detach. */ +#define UPDATE_DEVICE 2 /* Update device persistently, device must exist + * before update. */ static int qemuDomainModifyDevicePersistent(virDomainPtr dom, const char *xml, @@ -4305,6 +4320,8 @@ static int qemuDomainModifyDevicePersistent(virDomainPtr dom, ret = qemuDomainAttachDevicePersistent(vmdef, device); else if (action == DETACH_DEVICE) ret = qemuDomainDetachDevicePersistent(vmdef, device); + else if (action == UPDATE_DEVICE) + ret = qemuDomainUpdateDevicePersistent(vmdef, device); else { qemuReportError(VIR_ERR_INVALID_ARG, "%s %d", _("Unknown action"), action); -- 1.7.3.1 -- Thanks, Hu Tao
This makes code clean, and prepares for the next patch. --- src/qemu/qemu_driver.c | 39 +++++++++++++++++++++++---------------- 1 files changed, 23 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 98595d1..f432c38 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4360,10 +4360,9 @@ static int qemudDomainAttachDeviceFlags(virDomainPtr dom, return -1; } - -static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, - const char *xml, - unsigned int flags) +static int qemuDomainUpdateDevice(virDomainPtr dom, + const char *xml, + bool force) { struct qemud_driver *driver = dom->conn->privateData; virDomainObjPtr vm; @@ -4371,18 +4370,6 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, virBitmapPtr qemuCaps = NULL; virCgroupPtr cgroup = NULL; int ret = -1; - bool force = (flags & VIR_DOMAIN_DEVICE_MODIFY_FORCE) != 0; - - virCheckFlags(VIR_DOMAIN_DEVICE_MODIFY_CURRENT | - VIR_DOMAIN_DEVICE_MODIFY_LIVE | - VIR_DOMAIN_DEVICE_MODIFY_CONFIG | - VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1); - - if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("cannot modify the persistent configuration of a domain")); - return -1; - } qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -4482,6 +4469,26 @@ cleanup: return ret; } +static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, + const char *xml, + unsigned int flags) +{ + bool force = (flags & VIR_DOMAIN_DEVICE_MODIFY_FORCE) != 0; + + virCheckFlags(VIR_DOMAIN_DEVICE_MODIFY_CURRENT | + VIR_DOMAIN_DEVICE_MODIFY_LIVE | + VIR_DOMAIN_DEVICE_MODIFY_CONFIG | + VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1); + + if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("cannot modify the persistent configuration of a domain")); + return -1; + } + + return qemuDomainUpdateDevice(dom, xml, force); +} + static int qemudDomainDetachDevice(virDomainPtr dom, const char *xml) { -- 1.7.3.1 -- Thanks, Hu Tao
--- src/qemu/qemu_driver.c | 4 +--- 1 files changed, 1 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f432c38..335f5c4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4481,9 +4481,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom, VIR_DOMAIN_DEVICE_MODIFY_FORCE, -1); if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("cannot modify the persistent configuration of a domain")); - return -1; + return qemuDomainModifyDevicePersistent(dom, xml, UPDATE_DEVICE, flags); } return qemuDomainUpdateDevice(dom, xml, force); -- 1.7.3.1 -- Thanks, Hu Tao
At 03/16/2011 02:38 PM, KAMEZAWA Hiroyuki Write:
Thank you for review and feedbacks. This is v5 onto the latest git tree. Any comments are welcome. ==
From ef85d268a0b553cdb784a1d1916ff8cf171adace Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Wed, 16 Mar 2011 11:35:41 +0900 Subject: [PATCHv5 1/3] libivrt/qemu - support 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.
Changelog v4->v5: - fixed error codes at reporting Error. - fixed unlock code after calling qemuDomainObjBeginJobWithDriver() - fixed virDomainFindByUUID() to be virDomainFindByName() - fixed spelling, indent, etc.. - removed unnecessary checks.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/qemu/qemu_driver.c | 141 +++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 129 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index dac2bf2..dbd5bd3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4130,16 +4130,129 @@ cleanup: return ret; }
-static int qemudDomainAttachDeviceFlags(virDomainPtr dom, +/* + * 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,
Indentation is not consistent.
- unsigned int flags) { - if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("cannot modify the persistent configuration of a domain")); + unsigned int attach, unsigned int flags) +{ + struct qemud_driver *driver; + virDomainDeviceDefPtr device; + virDomainDefPtr vmdef; + virDomainObjPtr vm; + int ret = -1; + + if (!xml) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("internal error")); return -1; }
xml can not be null, because remoteDomainAttachDevice()/remoteDomainAttachDeviceFlags() will fail on client when xml is null.
+ /* + * 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", + _("Now, it's unsupported to update active domain's definition."));
Indentation is not consistent.
+ goto endjob; + } + + vmdef = virDomainObjGetPersistentDef(driver->caps, vm); + + if (!vmdef) + goto endjob; + + device = virDomainDeviceDefParse(driver->caps, + vmdef, xml, VIR_DOMAIN_XML_INACTIVE);
Indentation is not consistent.
+ 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 only MODIFY_LIVE, MODIFY_CONFIG are supported now"),
Indentation is not consistent.
+ flags);
- return qemudDomainAttachDevice(dom, xml); + return -1; }
@@ -4354,13 +4467,17 @@ 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 only MODIFY_LIVE, MODIFY_CONFIG are supported now"),
Indentation is not consistent.
+ flags); + return -1; }
static int qemudDomainGetAutostart(virDomainPtr dom,
On Mon, 21 Mar 2011 14:38:08 +0800 Wen Congyang <wency@cn.fujitsu.com> wrote:
At 03/16/2011 02:38 PM, KAMEZAWA Hiroyuki Write:
Thank you for review and feedbacks. This is v5 onto the latest git tree. Any comments are welcome. ==
From ef85d268a0b553cdb784a1d1916ff8cf171adace Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Wed, 16 Mar 2011 11:35:41 +0900 Subject: [PATCHv5 1/3] libivrt/qemu - support 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.
Changelog v4->v5: - fixed error codes at reporting Error. - fixed unlock code after calling qemuDomainObjBeginJobWithDriver() - fixed virDomainFindByUUID() to be virDomainFindByName() - fixed spelling, indent, etc.. - removed unnecessary checks.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/qemu/qemu_driver.c | 141 +++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 129 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index dac2bf2..dbd5bd3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4130,16 +4130,129 @@ cleanup: return ret; }
-static int qemudDomainAttachDeviceFlags(virDomainPtr dom, +/* + * 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,
Indentation is not consistent.
Hmm, will fix.
- unsigned int flags) { - if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("cannot modify the persistent configuration of a domain")); + unsigned int attach, unsigned int flags) +{ + struct qemud_driver *driver; + virDomainDeviceDefPtr device; + virDomainDefPtr vmdef; + virDomainObjPtr vm; + int ret = -1; + + if (!xml) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("internal error")); return -1; }
xml can not be null, because remoteDomainAttachDevice()/remoteDomainAttachDeviceFlags() will fail on client when xml is null.
ok.
+ /* + * 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", + _("Now, it's unsupported to update active domain's definition."));
Indentation is not consistent.
+ goto endjob; + } + + vmdef = virDomainObjGetPersistentDef(driver->caps, vm); + + if (!vmdef) + goto endjob; + + device = virDomainDeviceDefParse(driver->caps, + vmdef, xml, VIR_DOMAIN_XML_INACTIVE);
Indentation is not consistent.
+ 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 only MODIFY_LIVE, MODIFY_CONFIG are supported now"),
Indentation is not consistent.
+ flags);
- return qemudDomainAttachDevice(dom, xml); + return -1; }
@@ -4354,13 +4467,17 @@ 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 only MODIFY_LIVE, MODIFY_CONFIG are supported now"),
Indentation is not consistent.
+ flags); + return -1; }
static int qemudDomainGetAutostart(virDomainPtr dom,
will resend v6. Thanks, -Kame
participants (3)
-
Hu Tao -
KAMEZAWA Hiroyuki -
Wen Congyang