[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [Patch v7 1/4] libvirt/qemu - persistent modification of domain.



At 03/29/2011 12:32 PM, KAMEZAWA Hiroyuki Write:
> On Tue, 29 Mar 2011 11:24:23 +0800
> Wen Congyang <wency cn fujitsu com> wrote:
> 
>> At 03/25/2011 04:10 PM, KAMEZAWA Hiroyuki Write:
>>> This is v7. Dropped patches for Nics and added 2 sanity checks and
>>> show correct error messages.
>>>
>>> =
>>> >From 948597237bd9ecfc5c7343fd30efdca37733274e Mon Sep 17 00:00:00 2001
>>> From: KAMEZAWA Hiroyuki <kamezawa hiroyu jp fujitsu com>
>>> Date: Fri, 25 Mar 2011 16:59:55 +0900
>>> Subject: [PATCHv7 1/4] libvirt/qemu - persistent modification of devices.
>>>
>>> Now, qemudDomainAttachDeviceFlags() and qemudDomainDetachDeviceFlags()
>>> doesn't support VIR_DOMAIN_DEVICE_MODIFY_CONFIG. By this, virsh's
>>> at(de)tatch-device --persistent cannot modify qemu config.
>>> (Xen allows it.)
>>>
>>> This patch is a base patch for adding support of devices in
>>> step by step manner. Following patches will add some device
>>> support.
>>>
>>> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa hiroyu jp fujitsu com>
>>> ---
>>>  src/qemu/qemu_driver.c |  138 +++++++++++++++++++++++++++++++++++++++++++-----
>>>  1 files changed, 125 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index af897ad..a4fb1cd 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -4144,16 +4144,125 @@ cleanup:
>>>      return ret;
>>>  }
>>>  
>>> -static int qemudDomainAttachDeviceFlags(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"));
>>> +/*
>>> + * 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)
>>> +{
>>> +
>>> +    switch(newdev->type) {
>>> +    default:
>>> +        qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> +                        _("Sorry, the device is not supported for now"));
>>> +        return -1;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int qemuDomainDetachDevicePersistent(virDomainDefPtr vmdef,
>>> +                                            virDomainDeviceDefPtr device)
>>> +{
>>> +    switch(device->type) {
>>> +    default:
>>> +        qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> +                        _("Sorry, the device is not supported for now"));
>>>          return -1;
>>>      }
>>> +    return 0;
>>> +}
>>> +
>>> +static int qemuDomainModifyDevicePersistent(virDomainPtr dom,
>>> +                                            const char *xml,
>>> +                                            unsigned int attach,
>>> +                                            unsigned int flags)
>>> +{
>>> +    struct qemud_driver *driver;
>>> +    virDomainDeviceDefPtr device;
>>> +    virDomainDefPtr vmdef;
>>> +    virDomainObjPtr vm;
>>> +    int ret = -1;
>>> +
>>> +    /*
>>> +     * When both of MODIFY_CONFIG and MODIFY_LIVE is specified at the same,
>>> +     * time return error for now. We should support this later.
>>
>> s/at the same, time/at the same time,/
>>
> will fix.
> 
> 
>>> +     */
>>> +    if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) {
>>> +        qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
>>> +                        _("Now, cannot modify alive domain and its definition "
>>
>> You use 'alive domain' here, but you use 'active domain' below.
>> The message should be consistent.
>>
> 
> will fix.
> 
> 
>>> +                          "at the same time."));
>>> +        return -1;
>>> +    }
>>> +
>>> +    driver = dom->conn->privateData;
>>> +    qemuDriverLock(driver);
>>> +    vm = virDomainFindByName(&driver->domains, dom->name);
>>> +    if (!vm) {
>>> +        qemuReportError(VIR_ERR_NO_DOMAIN, _("no domain with name : '%s'"),
>>> +                        dom->name);
>>> +        goto unlock_out;
>>> +    }
>>> +
>>> +    if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
>>> +        goto unlock_out;
>>> +
>>> +    if (virDomainObjIsActive(vm)) {
>>> +        /*
>>> +         * 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_OPERATION_INVALID, "%s",
>>> +                        _("unsupported to update active domain's definition."));
>>> +        goto endjob;
>>> +    }
>>> +
>>> +    vmdef = virDomainObjGetPersistentDef(driver->caps, vm);
>>>  
>>> -    return qemudDomainAttachDevice(dom, xml);
>>> +    if (!vmdef)
>>> +        goto endjob;
>>> +
>>> +    device = virDomainDeviceDefParse(driver->caps,
>>> +                                     vmdef, xml, VIR_DOMAIN_XML_INACTIVE);
>>> +    if (!device)
>>> +        goto endjob;
>>> +
>>> +    if (attach)
>>> +        ret = qemuDomainAttachDevicePersistent(vmdef, device);
>>> +    else
>>> +        ret = qemuDomainDetachDevicePersistent(vmdef, device);
>>> +
>>> +    if (!ret) /* save the result */
>>> +        ret = virDomainSaveConfig(driver->configDir, vmdef);
>>
>> vmdef may be modified even if ret is not 0, but you do not update
>> the xml file. I do not find any problem about this, but it may be
>> better to update the xml file when ret is not 0.
>>
> 
> Hmm ? I'll add a spec. on qemuDomainAt(De)tachDevicePersistent() to
> never update vmdef when return !0. Is it ok ?

No.
In patch 2, the function qemuDomainDeviceAddressFixup() may modify vmdef
and return -1:
=============
+static int qemuDomainDeviceAddressFixup(virDomainDefPtr vmdef, bool pci)
+{
+    if (!pci && virDomainDefAddImplicitControllers(vmdef))
+        return -1;
+    /* added controller requires PCI address */
+    return qemuDomainAssignPCIAddresses(vmdef);
+}
+
=============
The function virDomainDefAddImplicitControllers() may modify vmdef. If 
qemuDomainAssignPCIAddresses() failed, there is no way to rollback the
operation that virDomainDefAddImplicitControllers() do.

> 
> 
> 
>>> +
>>> +    virDomainDeviceDefFree(device);
>>> +
>>> +endjob:
>>> +    if (qemuDomainObjEndJob(vm) == 0)
>>> +        vm = NULL;
>>> +unlock_out:
>>> +    if (vm)
>>> +        virDomainObjUnlock(vm);
>>> +    qemuDriverUnlock(driver);
>>> +    return ret;
>>> +}
>>> +
>>> +static int qemudDomainAttachDeviceFlags(virDomainPtr dom,
>>> +                                        const char *xml,
>>> +                                        unsigned int flags)
>>> +{
>>> +    if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG)
>>> +        return qemuDomainModifyDevicePersistent(dom, xml, 1, flags);
>>> +
>>> +    if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE)
>>> +        return qemudDomainAttachDevice(dom, xml);
>>> +
>>> +    qemuReportError(VIR_ERR_INVALID_ARG,
>>> +                    _("bad flag: %x not supported"), flags);
>>
>> If the flags is VIR_DOMAIN_DEVICE_MODIFY_LIVE | unsupported_flags, we do not
>> report error here.
>>
>> You can use the macro virCheckFlags to check it.
>>
> 
> Hm, ok. will see it.
> 
> Thanks,
> -Kame
> 
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]