[libvirt] [PATCHv6 2/3] libvirt/qemu - support persistent modification of qemu disks

[Wen Congyang]

At 03/24/2011 04:35 PM, KAMEZAWA Hiroyuki Write:
> On Thu, 24 Mar 2011 15:44:05 +0800
> Wen Congyang <wency at cn.fujitsu.com> wrote:
> 
>> At 03/24/2011 09:31 AM, KAMEZAWA Hiroyuki Write:
>>> >From 1d2630896a950b87043c9e77fdbcdc23626098ee Mon Sep 17 00:00:00 2001
>>> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu at jp.fujitsu.com>
>>> Date: Thu, 24 Mar 2011 10:05:18 +0900
>>> Subject: [PATCHv6 2/3] libvirt/qemu - support persistent modification of qemu disks
>>>
>>> support changes of disks by VIR_DOMAIN_DEVICE_MODIFY_CONFIG
>>> for qemu.
>>>
>>> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu at jp.fujitsu.com>
>>>
>>> Changelog v5->v6:
>>>   - fixed pci address/device controller assign failure case.
>>>
>>> Changelog v4->v5:
>>>   - moved some functions to domain_conf.c
>>>   - added virDomainDefAddImplicitControllers() for ide/scsi
>>>   - report OOM.
>>>   - clean up.
>>> ---
>>>  src/conf/domain_conf.c   |   22 +++++++++++++++++++
>>>  src/conf/domain_conf.h   |    3 +-
>>>  src/libvirt_private.syms |    2 +
>>>  src/qemu/qemu_driver.c   |   51 +++++++++++++++++++++++++++++++++++++++++++++-
>>>  4 files changed, 76 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index c637300..1bf8fbe 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 cbd0f98..236ad04 100644
>>> --- a/src/conf/domain_conf.h
>>> +++ b/src/conf/domain_conf.h
>>> @@ -1258,7 +1258,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,
>>> @@ -1266,6 +1266,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 55be36e..9ced196 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 468361b..386b654 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -4140,6 +4140,24 @@ cleanup:
>>>      return ret;
>>>  }
>>>  
>>> +static int qemuDomainDeviceAddressFixup(virDomainDefPtr vmdef, bool pci)
>>> +{
>>> +    if (pci) {
>>> +        if (qemuDomainAssignPCIAddresses(vmdef)) {
>>> +            qemuReportError(VIR_ERR_INVALID_ARG, "%s",
>>> +                            _("device or domain PCI addresses."));
>>> +            return -1;
>>> +        }
>>> +    } else {
>>> +        if (virDomainDefAddImplicitControllers(vmdef) < 0) {
>>> +            qemuReportError(VIR_ERR_INVALID_ARG, "%s",
>>> +                            _("device address or domain device controllers."));
>>> +            return -1;
>>> +        }
>>> +    }
>>
>> Do not report error when qemuDomainAssignPCIAddresses() or virDomainDefAddImplicitControllers()
>> failed, because we have reported the error in these functions.
>>
> 
> Hmm,  I didn't see any error message at failure caused by this...I'll check again.

Sometimes, qemuDomainAssignPCIAddress() failed, and it does not report error.

qemuDomainAssignPCIAddress() calls qemuDomainPCIAddressSetCreate()
that calls virDomainDeviceInfoIterate()
that calls qemuCollectPCIAddress()
that calls virHashAddEntry()
that calls virHashAddOrUpdateEntry()

When the pci address of two drivers are the same, virHashAddOrUpdateEntry() will fail.
    if (found) {
        if (is_update) {
            if (table->dataFree)
                table->dataFree(insert->payload, insert->name);
            insert->payload = userdata;
            return (0);
        } else {
            return (-1);  <==== we do not report error here.
        }
    }

> 
> Thanks,
> -Kame
> 
>