Re: [libvirt] [PATCH 1/2] libvirt/qemu - support updating inactive domains (take 2)

On Thu, Feb 24, 2011 at 01:06:36PM +0900, KAMEZAWA Hiroyuki wrote: > >From d6c65102224c493f67008b6521b0115d798f5a67 Mon Sep 17 00:00:00 2001 > From: KAMEZAWA Hiroyuki <kamezawa(a)bluextal.(none)&gt; > Date: Thu, 24 Feb 2011 12:14:25 +0900 > Subject: [PATCH 1/2] 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. > > Changelog v1->v2: > - fixed TABs > - fixed PCI address assignment for VIRTIO driver. > > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu(a)jp.fujitsu.com&gt; > --- > src/qemu/qemu_driver.c | 168 +++++++++++++++++++++++++++++++++++++++++++++--- > 1 files changed, 158 insertions(+), 10 deletions(-) > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 8b15a3e..e75478a 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -4085,14 +4085,164 @@ 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 (!strcmp(vdisk->dst, name)) Maybe STREQ() is better here as other libvirt code uses STREQ() rather than strcmp().

> + 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)) { > + virReportOOMError(); I think it's better to report OOM error by virDomainDiskInsert() itself, otherwise every caller has to call virReportOOMError() when fails.

> + return -1; > + } > + if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) { > + /* > + * Address/Drive information is NULL. If virtio, PCI address > + * shoule be fixed. Other devices as IDE, SCSI...will get ID > + * automatically > + */ > + qemuDomainAssignPCIAddresses(vmdef); > + > + } > + newdev->data.disk = NULL; > + break; > + 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) > +{ > + 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) > +{ > + struct qemud_driver *driver; > + virDomainDeviceDefPtr device; > + virDomainDefPtr vmdef; > + virDomainObjPtr vm; > + int ret = -1; > + > + if (!dom || !dom->conn || !dom->name || !xml) { > + qemuReportError(VIR_ERR_INVALID_ARG, > + _("internal error : %s"), __FUNCTION__); > + return -1; > + } > + > + if (dom->conn->flags & VIR_CONNECT_RO) > return -1; No need to check the RO flag here: it should be and is done in virDomainAttachDevice().

> + > + 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) { > + /* > + * 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_INVALID_ARG, > + _("cannot update alive domain : %s"), __FUNCTION__); I think we can silently fail here. Currently this function (qemuDomainModifyDevicePersistent) is called only for modifying inactive domains.

> + 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); > + if (ret < 0) > + goto out; > + } else { > + ret = qemuDomainDetachDevicePersistent(vmdef, device); > + if (ret < 0) > + goto out; > } This can be refined as: if (attach) ret = qemuDomainAttachDevicePersistent(vmdef, device); else ret = qemuDomainDetachDevicePersistent(vmdef, device); if (ret < 0) goto out;