[libvirt] [PATCH 0/5] libvirt/qemu - allow modification of devices in inactive domain
Now, Xen supports at(de)tach-disk, at(de)tach-interface for inactive domains with --perisitent option. (Using --persisten against live domain will cause error.) Now, virsh attach-xxx/detach-xxx cannot modify inactive domains in qemu and --persistent option just returns error. This series adds support for modification of inactive domain's disk, interfaces. Short description 1 ... clean up for Xen's driver 2 ... add my name to AUTHORS 3 ... add --persistent modification support, just a wrapper function. 4 ... add disk support 5 ... add nics support Thanks, -Kame
From b92569080a25bf0029d637327a87372bff071fae Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 3 Mar 2011 09:20:36 +0900 Subject: [PATCH 1/5] 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 c8350c6..4730683 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4859,8 +4859,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 7f73588..7437930 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
On 03/02/2011 06:09 PM, KAMEZAWA Hiroyuki wrote:
From b92569080a25bf0029d637327a87372bff071fae Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 3 Mar 2011 09:20:36 +0900 Subject: [PATCH 1/5] 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(-)
This patch looks accurate, but lacks justification (why can't all callers continue to call virReportOOMError() on failure)? I'm not sure whether to apply it without knowing why it is needed, as it just looks like code motion churn in isolation. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, 15 Mar 2011 15:38:50 -0600 Eric Blake <eblake@redhat.com> wrote:
On 03/02/2011 06:09 PM, KAMEZAWA Hiroyuki wrote:
From b92569080a25bf0029d637327a87372bff071fae Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 3 Mar 2011 09:20:36 +0900 Subject: [PATCH 1/5] 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(-)
This patch looks accurate, but lacks justification (why can't all callers continue to call virReportOOMError() on failure)? I'm not sure whether to apply it without knowing why it is needed, as it just looks like code motion churn in isolation.
just comes from my experiece. Ok, calls in the caller side. Thanks, -Kame
On Tue, Mar 15, 2011 at 03:38:50PM -0600, Eric Blake wrote:
On 03/02/2011 06:09 PM, KAMEZAWA Hiroyuki wrote:
From b92569080a25bf0029d637327a87372bff071fae Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 3 Mar 2011 09:20:36 +0900 Subject: [PATCH 1/5] 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(-)
This patch looks accurate, but lacks justification (why can't all callers continue to call virReportOOMError() on failure)? I'm not sure
Sure they can, but on the premise that they are sure there is an OOM happens. The semantics of virDomainDiskInsert is not clear here, returns -1 just means a failure but not an OOM. What if virDomainDiskInsert fails for other reasons, is it still correct for the caller to call virReportOOMError()?
whether to apply it without knowing why it is needed, as it just looks like code motion churn in isolation.
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
-- Thanks, Hu Tao
On 03/15/2011 07:13 PM, Hu Tao wrote:
On Tue, Mar 15, 2011 at 03:38:50PM -0600, Eric Blake wrote:
On 03/02/2011 06:09 PM, KAMEZAWA Hiroyuki wrote:
From b92569080a25bf0029d637327a87372bff071fae Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 3 Mar 2011 09:20:36 +0900 Subject: [PATCH 1/5] 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(-)
This patch looks accurate, but lacks justification (why can't all callers continue to call virReportOOMError() on failure)? I'm not sure
Sure they can, but on the premise that they are sure there is an OOM happens. The semantics of virDomainDiskInsert is not clear here, returns -1 just means a failure but not an OOM. What if virDomainDiskInsert fails for other reasons, is it still correct for the caller to call virReportOOMError()?
Right now, it only returns -1 for OOM (it's only a 4-line method, so it's easy to audit that statement for truth), and there's only a single caller. A valid justification would be that later in this patch series you plan on adding code to virDomainDiskInsert that returns -1 for other failures, in which case moving the error reporting into virDomainDiskInsert is absolutely the right thing to do. Or even just arguing the refactoring point of view: a patch that adds 4 lines and subtracts 4 lines is hard to see how it avoids duplicated code, whereas a refactoring that adds 4 lines and subtracts 8 is easier to spot as a useful consolidation. But I'm missing all of that justification in the commit comments - is there a plan for a later patch to be changing semantics? Is it for consistency with other functions in the same file that have similar semantics of reporting OOM on failure rather than delegating to the caller? Is it a refactoring designed to reduce duplication? In other words, code motion just for the sake of it is hard to understand (it doesn't necessarily make it wrong, but certainly delays the review), but code motion with a stated reason is much easier to ack. Even something like "future patches will add more callers to virDomainDiskInsert, so doing the code motion now will reduce code duplication of OOM reporting later" would help. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, Mar 15, 2011 at 07:29:24PM -0600, Eric Blake wrote:
On 03/15/2011 07:13 PM, Hu Tao wrote:
On Tue, Mar 15, 2011 at 03:38:50PM -0600, Eric Blake wrote:
On 03/02/2011 06:09 PM, KAMEZAWA Hiroyuki wrote:
From b92569080a25bf0029d637327a87372bff071fae Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 3 Mar 2011 09:20:36 +0900 Subject: [PATCH 1/5] 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(-)
This patch looks accurate, but lacks justification (why can't all callers continue to call virReportOOMError() on failure)? I'm not sure
Sure they can, but on the premise that they are sure there is an OOM happens. The semantics of virDomainDiskInsert is not clear here, returns -1 just means a failure but not an OOM. What if virDomainDiskInsert fails for other reasons, is it still correct for the caller to call virReportOOMError()?
Right now, it only returns -1 for OOM (it's only a 4-line method, so it's easy to audit that statement for truth), and there's only a single caller. A valid justification would be that later in this patch series you plan on adding code to virDomainDiskInsert that returns -1 for other failures, in which case moving the error reporting into virDomainDiskInsert is absolutely the right thing to do. Or even just arguing the refactoring point of view: a patch that adds 4 lines and subtracts 4 lines is hard to see how it avoids duplicated code, whereas a refactoring that adds 4 lines and subtracts 8 is easier to spot as a useful consolidation.
But I'm missing all of that justification in the commit comments - is there a plan for a later patch to be changing semantics? Is it for consistency with other functions in the same file that have similar semantics of reporting OOM on failure rather than delegating to the caller? Is it a refactoring designed to reduce duplication? In other words, code motion just for the sake of it is hard to understand (it doesn't necessarily make it wrong, but certainly delays the review), but code motion with a stated reason is much easier to ack. Even something like "future patches will add more callers to virDomainDiskInsert, so doing the code motion now will reduce code duplication of OOM reporting later" would help.
There is another call of virDomainDiskInsert() in patch 4 of this series. -- Thanks, Hu Tao
From 85ddb9b00fbd14cf3ead92df659bd568a1d9cd27 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 3 Mar 2011 09:22:45 +0900 Subject: [PATCH 2/5] add my name to AUTHORS
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- AUTHORS | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/AUTHORS b/AUTHORS index fb42662..54a99c6 100644 --- a/AUTHORS +++ b/AUTHORS @@ -156,6 +156,7 @@ Patches have also been contributed by: Michal Novotny <minovotn@redhat.com> Christophe Fergeau <teuf@gnome.org> Markus Groß <gross@univention.de> + KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> [....send patches to get your name here....] -- 1.7.4.1
At 03/03/2011 09:10 AM, KAMEZAWA Hiroyuki Write:
From 85ddb9b00fbd14cf3ead92df659bd568a1d9cd27 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 3 Mar 2011 09:22:45 +0900 Subject: [PATCH 2/5] add my name to AUTHORS
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- AUTHORS | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/AUTHORS b/AUTHORS index fb42662..54a99c6 100644 --- a/AUTHORS +++ b/AUTHORS @@ -156,6 +156,7 @@ Patches have also been contributed by: Michal Novotny <minovotn@redhat.com> Christophe Fergeau <teuf@gnome.org> Markus Groß <gross@univention.de> + KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
The file AUTHORS has contained your name. Why do you add it again?
[....send patches to get your name here....]
On Thu, 03 Mar 2011 11:18:15 +0800 Wen Congyang <wency@cn.fujitsu.com> wrote:
At 03/03/2011 09:10 AM, KAMEZAWA Hiroyuki Write:
From 85ddb9b00fbd14cf3ead92df659bd568a1d9cd27 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 3 Mar 2011 09:22:45 +0900 Subject: [PATCH 2/5] add my name to AUTHORS
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- AUTHORS | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/AUTHORS b/AUTHORS index fb42662..54a99c6 100644 --- a/AUTHORS +++ b/AUTHORS @@ -156,6 +156,7 @@ Patches have also been contributed by: Michal Novotny <minovotn@redhat.com> Christophe Fergeau <teuf@gnome.org> Markus Groß <gross@univention.de> + KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
The file AUTHORS has contained your name. Why do you add it again?
really ??? ....Ah, Kamezawa Hiroyuki is in the list. I usually use KAMEZAWA and my git's signature is KAMEZAWA.. Hmm. Sorry, please ignore this. Thanks, -Kame
Maybe this was what I needed. =
From 310080a8a3a28c9d5f36876971c22eda39600d7a Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 3 Mar 2011 13:45:15 +0900 Subject: [PATCH] rename Kamezawa as KAMEZAWA.
Note: maybe sounds strange but I've used this signature for years. see http://en.wikipedia.org/wiki/Family_name --- AUTHORS | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/AUTHORS b/AUTHORS index fb42662..a6e09f4 100644 --- a/AUTHORS +++ b/AUTHORS @@ -134,7 +134,7 @@ Patches have also been contributed by: Lai Jiangshan <laijs@cn.fujitsu.com> Harsh Prateek Bora <harsh@linux.vnet.ibm.com> John Morrissey <jwm@horde.net> - Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> + KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Wen Congyang <wency@cn.fujitsu.com> Hu Tao <hutao@cn.fujitsu.com> Laurent Lテゥonard <laurent@open-minds.org> -- 1.7.4.1
On 03/02/2011 09:52 PM, KAMEZAWA Hiroyuki wrote:
Maybe this was what I needed. =
From 310080a8a3a28c9d5f36876971c22eda39600d7a Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 3 Mar 2011 13:45:15 +0900 Subject: [PATCH] rename Kamezawa as KAMEZAWA.
I've applied this.
Laurent Lテゥonard
Wow - that's quite the mojibake. Are you handling AUTHORS and your email in UTF-8? For that matter, if you wanted to display your name in native characters instead of a westernized version, we're okay with that too. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Thu, 03 Mar 2011 17:22:57 -0700 Eric Blake <eblake@redhat.com> wrote:
On 03/02/2011 09:52 PM, KAMEZAWA Hiroyuki wrote:
Maybe this was what I needed. =
From 310080a8a3a28c9d5f36876971c22eda39600d7a Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 3 Mar 2011 13:45:15 +0900 Subject: [PATCH] rename Kamezawa as KAMEZAWA.
I've applied this.
Thank you.
Laurent Lテゥonard
Wow - that's quite the mojibake. Are you handling AUTHORS and your email in UTF-8? For that matter, if you wanted to display your name in native characters instead of a westernized version, we're okay with that too.
It seems my environment was not utf8, and fixed :) Name in English is okay. Thank you. -Kame
From f7a997e3cd58cfac81e131afdd20c3691267831d Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 3 Mar 2011 09:43:07 +0900 Subject: [PATCH 3/5] libivrt/qemu - support persistent modification of devices.
Now, virsh attach-***/detach-*** has --persistent option and it can update XML definition of inactive domains. Now, disk and nic are supported in Xen, but none in qemu. This patch adds a function for permanent modification for qemu. Following patches will add disk/network suport. TODO: - finally, need to do hotplug + XML modification for active domain. Changelog v3->v4 - fixed trailing white spaces. - splitted into 2 part and this patch only contains wrappers. Changelog v2->v3: - clean ups. - handle all VIR_DOMAIN_DEVICE_MODIFY_XXX flags. --- src/qemu/qemu_driver.c | 143 ++++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 131 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c9095bb..3248cdc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4099,16 +4099,131 @@ 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) +{ + + /* At first, check device confliction */ + switch(newdev->type) { + default: + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("Sorry, the device is not suppored for now")); + return -1; + } + + return 0; +} + +static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef, + virDomainDeviceDefPtr device) +{ + switch(device->type) { + 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) + 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, + _("bad flag: %x only MODIFY_LIVE, MODIFY_CONFIG are supported now"), + flags); + + return -1; } @@ -4322,13 +4437,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
At 03/03/2011 09:11 AM, KAMEZAWA Hiroyuki Write:
From f7a997e3cd58cfac81e131afdd20c3691267831d Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 3 Mar 2011 09:43:07 +0900 Subject: [PATCH 3/5] libivrt/qemu - support persistent modification of devices.
Now, virsh attach-***/detach-*** has --persistent option and it can update XML definition of inactive domains. Now, disk and nic are supported in Xen, but none in qemu.
This patch adds a function for permanent modification for qemu. Following patches will add disk/network suport.
TODO: - finally, need to do hotplug + XML modification for active domain.
Changelog v3->v4 - fixed trailing white spaces. - splitted into 2 part and this patch only contains wrappers. Changelog v2->v3: - clean ups. - handle all VIR_DOMAIN_DEVICE_MODIFY_XXX flags. --- src/qemu/qemu_driver.c | 143 ++++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 131 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c9095bb..3248cdc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4099,16 +4099,131 @@ 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) +{ + + /* At first, check device confliction */ + switch(newdev->type) { + default: + qemuReportError(VIR_ERR_INVALID_ARG, "%s",
When the device is not supported, we should use VIR_ERR_CONFIG_UNSUPPORTED instead of VIR_ERR_INVALID_ARG.
+ _("Sorry, the device is not suppored for now")); + return -1; + } + + return 0; +} + +static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef, + virDomainDeviceDefPtr device)
The coding style is wrong: virDomainDeviceDefPtr shoulde be left justified with virDomainDefPtr.
+{ + switch(device->type) { + 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)
Why do you modify the type of flag?
+{ + struct qemud_driver *driver; + virDomainDeviceDefPtr device; + virDomainDefPtr vmdef; + virDomainObjPtr vm; + int ret = -1; + + if (!dom || !dom->conn || !dom->name || !xml) {
dom and dom->conn does not need check, they are always not null.
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("internal error : %s"), __FUNCTION__);
Do not use __FUNCTION__ here, the macro qemuReportError will do it.
+ 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",
Please use VIR_ERR_OPERATION_INVALID instead of VIR_ERR_INVALID_ARG.
+ _("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);
You use UUID to search the domain, so the error message should be: no domain with matching uuid '%s'
+ 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; + if (vm) + virDomainObjUnlock(vm);
When qemuDomainObjBeginJobWithDriver() failed, we should unlock the domain. So we should do it when we goto unlock_out.
+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, + _("bad flag: %x only MODIFY_LIVE, MODIFY_CONFIG are supported now"), + flags); + + return -1; }
@@ -4322,13 +4437,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,
On Mon, 14 Mar 2011 13:29:13 +0800 Wen Congyang <wency@cn.fujitsu.com> wrote:
At 03/03/2011 09:11 AM, KAMEZAWA Hiroyuki Write:
From f7a997e3cd58cfac81e131afdd20c3691267831d Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 3 Mar 2011 09:43:07 +0900 Subject: [PATCH 3/5] libivrt/qemu - support persistent modification of devices.
Now, virsh attach-***/detach-*** has --persistent option and it can update XML definition of inactive domains. Now, disk and nic are supported in Xen, but none in qemu.
This patch adds a function for permanent modification for qemu. Following patches will add disk/network suport.
TODO: - finally, need to do hotplug + XML modification for active domain.
Changelog v3->v4 - fixed trailing white spaces. - splitted into 2 part and this patch only contains wrappers. Changelog v2->v3: - clean ups. - handle all VIR_DOMAIN_DEVICE_MODIFY_XXX flags. --- src/qemu/qemu_driver.c | 143 ++++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 131 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c9095bb..3248cdc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4099,16 +4099,131 @@ 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) +{ + + /* At first, check device confliction */ + switch(newdev->type) { + default: + qemuReportError(VIR_ERR_INVALID_ARG, "%s",
When the device is not supported, we should use VIR_ERR_CONFIG_UNSUPPORTED instead of VIR_ERR_INVALID_ARG.
will change.
+ _("Sorry, the device is not suppored for now")); + return -1; + } + + return 0; +} + +static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef, + virDomainDeviceDefPtr device)
The coding style is wrong: virDomainDeviceDefPtr shoulde be left justified with virDomainDefPtr.
Hmm, okay.
+{ + switch(device->type) { + 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)
Why do you modify the type of flag?
my bug.
+{ + struct qemud_driver *driver; + virDomainDeviceDefPtr device; + virDomainDefPtr vmdef; + virDomainObjPtr vm; + int ret = -1; + + if (!dom || !dom->conn || !dom->name || !xml) {
dom and dom->conn does not need check, they are always not null.
Ok, so, Xen code should be fixed. (This is just a copy-n-paste.)
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("internal error : %s"), __FUNCTION__);
Do not use __FUNCTION__ here, the macro qemuReportError will do it.
+ 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",
Please use VIR_ERR_OPERATION_INVALID instead of VIR_ERR_INVALID_ARG.
ok.
+ _("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);
You use UUID to search the domain, so the error message should be: no domain with matching uuid '%s'
ok.
+ 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; + if (vm) + virDomainObjUnlock(vm);
When qemuDomainObjBeginJobWithDriver() failed, we should unlock the domain. So we should do it when we goto unlock_out.
ok.
+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, + _("bad flag: %x only MODIFY_LIVE, MODIFY_CONFIG are supported now"), + flags); + + return -1; }
@@ -4322,13 +4437,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,
Thank you. -Kame
On 03/02/2011 06:11 PM, KAMEZAWA Hiroyuki wrote:
From f7a997e3cd58cfac81e131afdd20c3691267831d Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 3 Mar 2011 09:43:07 +0900 Subject: [PATCH 3/5] libivrt/qemu - support persistent modification of devices.
Now, virsh attach-***/detach-*** has --persistent option and it can update XML definition of inactive domains.
Hmm, looking at the recent additions of other virsh persistent support, the single --persistent flag of virsh attach-{device,disk,interface} are a bit limited; --persistent means the same thing as --config, but we may need to add a counterpart --live option that specifies live only. But cleaning up virsh to better expose the underlying flags is a separate patch; this patch only deals with implementing the flags internally.
Now, disk and nic are supported in Xen, but none in qemu.
It would be nice to get all devices supported, but incremental is better than nothing at all.
This patch adds a function for permanent modification for qemu. Following patches will add disk/network suport.
TODO: - finally, need to do hotplug + XML modification for active domain.
Changelog v3->v4 - fixed trailing white spaces. - splitted into 2 part and this patch only contains wrappers. Changelog v2->v3: - clean ups. - handle all VIR_DOMAIN_DEVICE_MODIFY_XXX flags. --- src/qemu/qemu_driver.c | 143 ++++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 131 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c9095bb..3248cdc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4099,16 +4099,131 @@ 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) +{ + + /* At first, check device confliction */
s/device confliction/for device support/
+ switch(newdev->type) { + default: + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("Sorry, the device is not suppored for now"));
s/suppored/supported/ (twice)
+ +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, 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."));
It shouldn't be too hard to attempt supporting this now - after all, we already did just that with Taku Izumi's patch for virsh setmem. qemudDomainSetMemoryFlags can be a good reference point for how to implement a command that first validates all conditions required by the flags, then modifies the live configuration if requested and still live, then modifies the persistent configuration if requested. I think there have been enough findings already (Wen's feedback has also been useful), as well as quite a bit of upstream churn that will require rebasing aspects of your patch, that I'll wait for v5 of this series; if you use 'git send-email --subject-prefix=PATCHv5', that will make it easier to recognize from the subject line that it is a resend. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, 15 Mar 2011 15:55:51 -0600 Eric Blake <eblake@redhat.com> wrote:
On 03/02/2011 06:11 PM, KAMEZAWA Hiroyuki wrote:
From f7a997e3cd58cfac81e131afdd20c3691267831d Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 3 Mar 2011 09:43:07 +0900 Subject: [PATCH 3/5] libivrt/qemu - support persistent modification of devices.
Now, virsh attach-***/detach-*** has --persistent option and it can update XML definition of inactive domains.
Hmm, looking at the recent additions of other virsh persistent support, the single --persistent flag of virsh attach-{device,disk,interface} are a bit limited; --persistent means the same thing as --config, but we may need to add a counterpart --live option that specifies live only. But cleaning up virsh to better expose the underlying flags is a separate patch; this patch only deals with implementing the flags internally.
yes.
Now, disk and nic are supported in Xen, but none in qemu.
It would be nice to get all devices supported, but incremental is better than nothing at all.
This patch adds a function for permanent modification for qemu. Following patches will add disk/network suport.
TODO: - finally, need to do hotplug + XML modification for active domain.
Changelog v3->v4 - fixed trailing white spaces. - splitted into 2 part and this patch only contains wrappers. Changelog v2->v3: - clean ups. - handle all VIR_DOMAIN_DEVICE_MODIFY_XXX flags. --- src/qemu/qemu_driver.c | 143 ++++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 131 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c9095bb..3248cdc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4099,16 +4099,131 @@ 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) +{ + + /* At first, check device confliction */
s/device confliction/for device support/
will fix.
+ switch(newdev->type) { + default: + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("Sorry, the device is not suppored for now"));
s/suppored/supported/ (twice)
will fix. ....I'll use spell checker.
+ +static int qemuDomainModifyDevicePersistent(virDomainPtr dom, const char *xml,
Indentation is not consistent.
ok.
- 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."));
It shouldn't be too hard to attempt supporting this now - after all, we already did just that with Taku Izumi's patch for virsh setmem. qemudDomainSetMemoryFlags can be a good reference point for how to implement a command that first validates all conditions required by the flags, then modifies the live configuration if requested and still live, then modifies the persistent configuration if requested.
Can we do it in atomic ?
I think there have been enough findings already (Wen's feedback has also been useful), as well as quite a bit of upstream churn that will require rebasing aspects of your patch, that I'll wait for v5 of this series; if you use 'git send-email --subject-prefix=PATCHv5', that will make it easier to recognize from the subject line that it is a resend.
ok, will do. Thanks, -Kame
+ +static int qemuDomainModifyDevicePersistent(virDomainPtr dom, const char *xml,
Indentation is not consistent.
ok.
FYI, if you use vim as your editor, the following configuration can save you a lot of time: set autoindent set smartindent set cindent set tabstop=4 set shiftwidth=4 set expandtab set cinoptions=(0,:0,l1,t0 Just select the code snippet and press `=' will indent the code nicely. -- Thanks, Hu Tao
On 03/15/2011 07:28 PM, Hu Tao wrote:
+ +static int qemuDomainModifyDevicePersistent(virDomainPtr dom, const char *xml,
Indentation is not consistent.
ok.
FYI, if you use vim as your editor, the following configuration can save you a lot of time:
set autoindent set smartindent set cindent set tabstop=4 set shiftwidth=4 set expandtab set cinoptions=(0,:0,l1,t0
Just select the code snippet and press `=' will indent the code nicely.
We already have .dir-locals.el to auto-set indentation for emacs users; does vim support a directory-local configuration file that would be worth checking into libvirt.git to make vim auto-indent with consistent rules for all files found in the libvirt directory tree? [If you haven't guessed, I'm not a vim user] -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, 16 Mar 2011 09:28:46 +0800 Hu Tao <hutao@cn.fujitsu.com> wrote:
+ +static int qemuDomainModifyDevicePersistent(virDomainPtr dom, const char *xml,
Indentation is not consistent.
ok.
FYI, if you use vim as your editor, the following configuration can save you a lot of time:
set autoindent set smartindent set cindent set tabstop=4 set shiftwidth=4 set expandtab set cinoptions=(0,:0,l1,t0
Just select the code snippet and press `=' will indent the code nicely.
Thank you. it's helpful. -Kame
-- Thanks, Hu Tao
From 8d2544bc773a2222c8aa1fdfc5cade20d8c1e958 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 3 Mar 2011 09:53:08 +0900 Subject: [PATCH 4/5] libvirt/qemu - support modification of disks in inactive domain
This patch adds support for persistent modification of disks in inactive domain. Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/qemu/qemu_driver.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 42 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3248cdc..4df6bf9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4099,6 +4099,18 @@ cleanup: return ret; } +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. @@ -4106,9 +4118,26 @@ cleanup: 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")); @@ -4121,7 +4150,20 @@ static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, 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")); -- 1.7.4.1
At 03/03/2011 09:12 AM, KAMEZAWA Hiroyuki Write:
From 8d2544bc773a2222c8aa1fdfc5cade20d8c1e958 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 3 Mar 2011 09:53:08 +0900 Subject: [PATCH 4/5] libvirt/qemu - support modification of disks in inactive domain
This patch adds support for persistent modification of disks in inactive domain.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/qemu/qemu_driver.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 42 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3248cdc..4df6bf9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4099,6 +4099,18 @@ cleanup: return ret; }
+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; +}
This function can be moved into src/conf/domain_conf.c, and it may be used by other driver if this driver supports attach/detach device into inactive domain.
/* * Attach a device given by XML, the change will be persistent * and domain XML definition file is updated. @@ -4106,9 +4118,26 @@ cleanup: 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;
Use virDomainDefAddImplicitControllers() to add any implied controllers which aren't present and used by attached disk.
+ newdev->data.disk = NULL; + break; default: qemuReportError(VIR_ERR_INVALID_ARG, "%s", _("Sorry, the device is not suppored for now")); @@ -4121,7 +4150,20 @@ static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, 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"));
On Mon, 14 Mar 2011 17:31:49 +0800 Wen Congyang <wency@cn.fujitsu.com> wrote:
At 03/03/2011 09:12 AM, KAMEZAWA Hiroyuki Write:
From 8d2544bc773a2222c8aa1fdfc5cade20d8c1e958 Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 3 Mar 2011 09:53:08 +0900 Subject: [PATCH 4/5] libvirt/qemu - support modification of disks in inactive domain
This patch adds support for persistent modification of disks in inactive domain.
Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- src/qemu/qemu_driver.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 42 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3248cdc..4df6bf9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4099,6 +4099,18 @@ cleanup: return ret; }
+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; +}
This function can be moved into src/conf/domain_conf.c, and it may be used by other driver if this driver supports attach/detach device into inactive domain.
ok, will move.
/* * Attach a device given by XML, the change will be persistent * and domain XML definition file is updated. @@ -4106,9 +4118,26 @@ cleanup: 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;
Use virDomainDefAddImplicitControllers() to add any implied controllers which aren't present and used by attached disk.
For ide or scsi ? ok.
+ newdev->data.disk = NULL; + break; default: qemuReportError(VIR_ERR_INVALID_ARG, "%s", _("Sorry, the device is not suppored for now")); @@ -4121,7 +4150,20 @@ static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, 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"));
Thanks, -Kame
From 803cef98bc31a090b876f1774648e317401c806a Mon Sep 17 00:00:00 2001 From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Date: Thu, 3 Mar 2011 10:01:39 +0900 Subject: [PATCH 5/5] libvirt/qemu - allow modification nics of inactive domain
This patch allows --persistent modification of nics with inactive domains in qemu. Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> --- 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 4730683..8ef143c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4928,6 +4928,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 5e63a12..35b4e30 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -249,6 +249,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 4df6bf9..9f14880 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4111,6 +4111,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. @@ -4119,6 +4134,7 @@ static int qemuDomainAttachDevicePersistent(virDomainDefPtr vmdef, virDomainDeviceDefPtr newdev) { virDomainDiskDefPtr disk; + virDomainNetDefPtr net; /* At first, check device confliction */ switch(newdev->type) { @@ -4138,6 +4154,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")); @@ -4152,6 +4185,7 @@ static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef, { int x; virDomainDiskDefPtr disk; + virDomainNetDefPtr net; switch(device->type) { case VIR_DOMAIN_DEVICE_DISK: @@ -4164,6 +4198,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 Thu, 3 Mar 2011 10:07:40 +0900 KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
Now, Xen supports at(de)tach-disk, at(de)tach-interface for inactive domains with --perisitent option. (Using --persisten against live domain will cause error.)
Now, virsh attach-xxx/detach-xxx cannot modify inactive domains in qemu and --persistent option just returns error.
This series adds support for modification of inactive domain's disk, interfaces.
Short description 1 ... clean up for Xen's driver 2 ... add my name to AUTHORS 3 ... add --persistent modification support, just a wrapper function. 4 ... add disk support 5 ... add nics support
What's bad with this patches ? If libvirt never supports modification of XML in virsh *** command, I'll change my direction and send patches to virtinst. Thanks, -Kame
On 03/03/2011 04:40 PM, KAMEZAWA Hiroyuki wrote:
On Thu, 3 Mar 2011 10:07:40 +0900 KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
Now, Xen supports at(de)tach-disk, at(de)tach-interface for inactive domains with --perisitent option. (Using --persisten against live domain will cause error.)
Now, virsh attach-xxx/detach-xxx cannot modify inactive domains in qemu and --persistent option just returns error.
This series adds support for modification of inactive domain's disk, interfaces.
Short description 1 ... clean up for Xen's driver 2 ... add my name to AUTHORS 3 ... add --persistent modification support, just a wrapper function. 4 ... add disk support 5 ... add nics support
What's bad with this patches ?
Personally, I think they are a good addition, but I haven't yet had time to review them closely for potential coding gotchas. Rest assured, it's on my todo list, and either these patches, or a further refined version, will be included in the next libvirt release.
If libvirt never supports modification of XML in virsh *** command, I'll change my direction and send patches to virtinst.
Sorry for the delays; it's been a busy week (I've been swamped trying to debug libvirt crashes, which takes a higher priority on my plate over libvirt feature additions). Don't give up yet; and it's fine to ping us every few days if we continue to drag our feet! -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Thu, 03 Mar 2011 17:06:05 -0700 Eric Blake <eblake@redhat.com> wrote:
On 03/03/2011 04:40 PM, KAMEZAWA Hiroyuki wrote:
On Thu, 3 Mar 2011 10:07:40 +0900 KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
Now, Xen supports at(de)tach-disk, at(de)tach-interface for inactive domains with --perisitent option. (Using --persisten against live domain will cause error.)
Now, virsh attach-xxx/detach-xxx cannot modify inactive domains in qemu and --persistent option just returns error.
This series adds support for modification of inactive domain's disk, interfaces.
Short description 1 ... clean up for Xen's driver 2 ... add my name to AUTHORS 3 ... add --persistent modification support, just a wrapper function. 4 ... add disk support 5 ... add nics support
What's bad with this patches ?
Personally, I think they are a good addition, but I haven't yet had time to review them closely for potential coding gotchas. Rest assured, it's on my todo list, and either these patches, or a further refined version, will be included in the next libvirt release.
If libvirt never supports modification of XML in virsh *** command, I'll change my direction and send patches to virtinst.
Sorry for the delays; it's been a busy week (I've been swamped trying to debug libvirt crashes, which takes a higher priority on my plate over libvirt feature additions). Don't give up yet; and it's fine to ping us every few days if we continue to drag our feet!
Okay, thank you for reply. I should be more patient. BTW, what's crash ? Sorry, -Kame
On 03/03/2011 05:04 PM, KAMEZAWA Hiroyuki wrote:
Sorry for the delays; it's been a busy week (I've been swamped trying to debug libvirt crashes, which takes a higher priority on my plate over libvirt feature additions). Don't give up yet; and it's fine to ping us every few days if we continue to drag our feet!
Okay, thank you for reply. I should be more patient. BTW, what's crash ?
Transient domains and improper locking: https://www.redhat.com/archives/libvir-list/2011-March/msg00112.html Migration failures due to a flaw in qemu exec:command: https://www.redhat.com/archives/libvir-list/2011-March/msg00040.html I'm not the only one that can review and push patches, but I tend to be one of the more active maintainers; thus when I get swamped debugging a tough issue, sometimes that shows in my email response time and subsequent level of community patches pushed into libvirt.git. As long as you don't take it personally :) -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (4)
-
Eric Blake -
Hu Tao -
KAMEZAWA Hiroyuki -
Wen Congyang