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

On Wed, 23 Mar 2011 13:15:39 +0800 Wen Congyang <wency(a)cn.fujitsu.com&gt; wrote: > 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. > Hmm, ok. what I should to unahs and free vmdef ?

Thanks, -Kame