Re: [libvirt] [PATCHv5 2/3] libvirt/qemu - support persistent modification of qemu disks

At 03/23/2011 12:39 PM, KAMEZAWA Hiroyuki Write: > On Mon, 21 Mar 2011 16:05:19 +0800 > Wen Congyang <wency(a)cn.fujitsu.com&gt; 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(a)jp.fujitsu.com&gt; >>> 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(a)jp.fujitsu.com&gt; >>> --- >>> 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.