On Fri, Feb 25, 2011 at 01:40:44PM +0900, KAMEZAWA Hiroyuki wrote:

...

From fff185c7ae8892cb4d39f7adb90fe9e13cef5638 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa@bluextal.(none)> Date: Thu, 24 Feb 2011 19:02:42 +0900 Subject: [PATCH 2/3] libvirt/qemu - support updating inactive domains.

Now, virsh attach-disk/detach-disk has --persistent option and it can update XML definition of inactive domains. But, it's only supported in Xen.

This patch adds support for attach-disk/detach-disk for qemu.

Note: This patch just allows to modify XML definition of 'inactive' domain. More patches will be required for modify 'active' domain in persistent mode. (modify XML + do hotplug)

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Changelog v2->v3: - clean ups. - handle all VIR_DOMAIN_DEVICE_MODIFY_XXX flags. --- src/qemu/qemu_driver.c | 185 ++++++++++++++++++++++++++++++++++++++++++++--- 1 files changed, 173 insertions(+), 12 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1a7bec9..a2987c4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4097,16 +4097,173 @@ cleanup: return ret; }

-static int qemudDomainAttachDeviceFlags(virDomainPtr dom, +static int qemuDomainFindDiskByName(virDomainDefPtr vmdef, const char *name) +{ + virDomainDiskDefPtr vdisk; + int i; + + for (i = 0; i < vmdef->ndisks; i++) { + vdisk = vmdef->disks[i]; + if (STREQ(vdisk->dst, name)) + return i; + } + return -1; +} +/* + * 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) +{ + virDomainDiskDefPtr disk; + + /* At first, check device confliction */ + switch(newdev->type) { + case VIR_DOMAIN_DEVICE_DISK: + disk = newdev->data.disk; + if (qemuDomainFindDiskByName(vmdef, disk->dst) >= 0) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("target %s already exists."), disk->dst); + return -1; + } + + if (virDomainDiskInsert(vmdef, disk)) /* only failed with OOM */ + return -1; + + if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO && + qemuDomainAssignPCIAddresses(vmdef) < 0) + return -1; + newdev->data.disk = NULL; + break; + default: + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("Sorry, the device is not suppored for now")); + return -1; + } +

trailing spaces

+ return 0; +} + +static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef, + virDomainDeviceDefPtr device) +{ + int x; + virDomainDiskDefPtr disk; + + switch(device->type) { + case VIR_DOMAIN_DEVICE_DISK: + disk = device->data.disk; + x = qemuDomainFindDiskByName(vmdef, disk->dst); + if (x < 0) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("target %s doesn't exist."), disk->dst); + return -1; + } + virDomainDiskRemove(vmdef, x); + break; + default: + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("Sorry, the device is not suppored 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, int flags) +{ + struct qemud_driver *driver; + virDomainDeviceDefPtr device; + virDomainDefPtr vmdef; + virDomainObjPtr vm; + int ret = -1; + + if (!dom || !dom->conn || !dom->name || !xml) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("internal error : %s"), __FUNCTION__); + 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_INVALID_ARG, "%s", + _("Now, cannot modify alive domain and its definition " + "at the same time.")); return -1; }

- return qemudDomainAttachDevice(dom, xml); + driver = dom->conn->privateData; + qemuDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + if (!vm) { + qemuReportError(VIR_ERR_NO_DOMAIN, _("cannot find domain '%s'"), + dom->name); + goto unlock_out; + } + + if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)

trailing space

+ 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; + if (vm) + virDomainObjUnlock(vm); +unlock_out: + 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,

trailing space

+ _("bad flag: %x only MODIFY_LIVE, MODIFY_CONFIG are supported now"), + flags); + + return -1; }

@@ -4320,13 +4477,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.1

ACK with trailing spaces fixed and email address fixed. -- Thanks, Hu Tao

On Fri, 25 Feb 2011 15:08:19 +0800 Hu Tao <hutao@cn.fujitsu.com> wrote:

...

+static int qemuDomainFindNetByName(virDomainDefPtr vmdef, + const unsigned char *mac) +{ + virDomainNetDefPtr net; + int i; + + for (i = 0; i < vmdef->nnets; i++) { + net = vmdef->nets[i]; + /* For now, only MAC can be the key */ + if (!strcmp((char*)net->mac, (char*)mac))

ack with STREQ and email address fixed.

fixed. ==

From b2ef98f15a973b134cf1d98f6eee08831d134e89 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Fri, 25 Feb 2011 17:19:12 +0900 Subject: [PATCH 3/4] libvirt/qemu - support updating inactive domain interfaces.

Now, virsh attach-disk/detach-disk has --persistent option and it can update XML definition of inactive domains. But, it's only supported in Xen. This patch adds support for attach-disk/detach-disk for qemu. Note: This patch just allows to modify XML definition of 'inactive' domain. More patches will be required for modify 'active' domain in persistent mode. (modify XML + do hotplug) Changelog v3->v3.1 - use STREQ() instead of strcmp. Changelog v2->v3: - clean ups. - handle all VIR_DOMAIN_DEVICE_MODIFY_XXX flags. --- src/conf/domain_conf.c | 24 +++++++++++++++++++++ src/conf/domain_conf.h | 3 ++ src/libvirt_private.syms | 2 + src/qemu/qemu_driver.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 80 insertions(+), 0 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b4193b9..6f28c9a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4926,6 +4926,30 @@ void virDomainDiskRemove(virDomainDefPtr def, size_t i) } } +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; +} + +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 virDomainControllerInsert(virDomainDefPtr def, virDomainControllerDefPtr controller) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 30aeccc..d7d973e 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1270,6 +1270,9 @@ int virDomainDiskDefAssignAddress(virCapsPtr caps, virDomainDiskDefPtr def); void virDomainDiskRemove(virDomainDefPtr def, size_t i); +int virDomainNetInsert(virDomainDefPtr def, virDomainNetDefPtr net); +void virDomainNetRemove(virDomainDefPtr def, size_t i); + int virDomainControllerInsert(virDomainDefPtr def, virDomainControllerDefPtr controller); void virDomainControllerInsertPreAlloced(virDomainDefPtr def, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 66917ca..08c06d9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -246,6 +246,8 @@ virDomainDiskIoTypeToString; virDomainDiskRemove; virDomainDiskTypeFromString; virDomainDiskTypeToString; +virDomainNetInsert; +virDomainNetRemove; virDomainFSDefFree; virDomainFindByID; virDomainFindByName; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 53f3d95..ca6dfb7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4109,6 +4109,21 @@ static int qemuDomainFindDiskByName(virDomainDefPtr vmdef, const char *name) } return -1; } + +static int qemuDomainFindNetByName(virDomainDefPtr vmdef, + const unsigned char *mac) +{ + virDomainNetDefPtr net; + int i; + + for (i = 0; i < vmdef->nnets; i++) { + net = vmdef->nets[i]; + /* For now, only MAC can be the key */ + if (STREQ((char*)net->mac, (char*)mac)) + return i; + } + return -1; +} /* * Attach a device given by XML, the change will be persistent * and domain XML definition file is updated. @@ -4117,6 +4132,7 @@ static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, virDomainDeviceDefPtr newdev) { virDomainDiskDefPtr disk; + virDomainNetDefPtr net; /* At first, check device confliction */ switch(newdev->type) { @@ -4136,6 +4152,23 @@ static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, return -1; newdev->data.disk = NULL; break; + case VIR_DOMAIN_DEVICE_NET: + net = newdev->data.net; + if (qemuDomainFindNetByName(vmdef, net->mac) >= 0) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("target %s already exists."), net->mac); + return -1; + } + + if (virDomainNetInsert(vmdef, net)) { + virReportOOMError(); + return -1; + } + /* always PCI ? */ + if (qemuDomainAssignPCIAddresses(vmdef) < 0) + return -1; + newdev->data.net = NULL; + break; default: qemuReportError(VIR_ERR_INVALID_ARG, "%s", _("Sorry, the device is not suppored for now")); @@ -4150,6 +4183,7 @@ static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef, { int x; virDomainDiskDefPtr disk; + virDomainNetDefPtr net; switch(device->type) { case VIR_DOMAIN_DEVICE_DISK: @@ -4162,6 +4196,23 @@ static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef, } virDomainDiskRemove(vmdef, x); break; + case VIR_DOMAIN_DEVICE_NET: + net = device->data.net; + /* need to find mac address */ + if (net->ifname == NULL && strlen((char*)net->mac) == 0) { + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("interface mac or name must be specified.")); + return -1; + } + net = device->data.net; + x = qemuDomainFindNetByName(vmdef, net->mac); + if (x < 0) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("interface mac: %s doesn't exist."), net->mac); + return -1; + } + virDomainNetRemove(vmdef, x); + break; default: qemuReportError(VIR_ERR_INVALID_ARG, "%s", _("Sorry, the device is not suppored for now")); -- 1.7.4.1

On Fri, 25 Feb 2011 15:07:50 +0800 Hu Tao <hutao@cn.fujitsu.com> wrote:

ACK with email address fixed at From: field.

Ah, yes. I didn't configure this host's git... ==

From 756eaa4230ab5fc8bbfb1b02a24b34dedad473b2 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Fri, 25 Feb 2011 17:13:28 +0900 Subject: [PATCH 1/4] report OOMError in virDomainDiskInsert()

Now, virDomainDiskInsert() returns -1 at memory allocation failure but it should call virReportOOMError() by itself. Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/conf/domain_conf.c | 4 +++- src/xen/xm_internal.c | 4 +--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b97c1f0..b4193b9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4857,8 +4857,10 @@ int virDomainDiskInsert(virDomainDefPtr def, virDomainDiskDefPtr disk) { - if (VIR_REALLOC_N(def->disks, def->ndisks+1) < 0) + if (VIR_REALLOC_N(def->disks, def->ndisks+1) < 0) { + virReportOOMError(); return -1; + } virDomainDiskInsertPreAlloced(def, disk); diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index 27cc387..c3c72fe 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -1385,10 +1385,8 @@ xenXMDomainAttachDeviceFlags(virDomainPtr domain, const char *xml, switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: { - if (virDomainDiskInsert(def, dev->data.disk) < 0) { - virReportOOMError(); + if (virDomainDiskInsert(def, dev->data.disk) < 0) goto cleanup; - } dev->data.disk = NULL; } break; -- 1.7.4.1