Re: [libvirt] [PATCH 3/5] libvirt/qemu - support persistent modification of devices.

On 03/02/2011 06:11 PM, KAMEZAWA Hiroyuki wrote: >>From f7a997e3cd58cfac81e131afdd20c3691267831d Mon Sep 17 00:00:00 2001 > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com&gt; > 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.