[libvirt] [PATCH] qemuDomainUpdateDeviceConfig: Allow startupPolicy update, yet again

[Michal Privoznik]

On 14.09.2015 14:00, Michal Privoznik wrote:
> On 14.09.2015 13:24, Peter Krempa wrote:
>> On Mon, Sep 14, 2015 at 13:02:49 +0200, Michal Privoznik wrote:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1159219
>>>
>>> So, in 11e058ca589808bd I've tried to make UpdateDevice update
>>> startupPolicy too. And it worked well until somebody came around
>>> and pushed d0dc6c036914da which accidentally removed my
>>> contribution. Redo my commit.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>>> ---
>>>  src/qemu/qemu_driver.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index 91eb661..03ef972 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -8353,6 +8353,7 @@ qemuDomainUpdateDeviceConfig(virQEMUCapsPtr qemuCaps,
>>>           * We allow updating src/type//driverType/cachemode/
>>>           */
>>>          orig->cachemode = disk->cachemode;
>>> +        orig->startupPolicy = disk->startupPolicy;
>>
>> This also might make sense to be updated in the "live" case to
>> "requisite" setting for migration.
> 
> I think not only that. In fact all three options make sense on live case
> too. mandatory - I want to fail migration if disk is not present on dst,
> requisite is the same as optional in this case, since if domain is
> started up, it means that disk is there, it's present. But I might want
> to drop it during migration.
> 
> Okay, I'll repost.
> 

Hm, now that I dig into the code, it's not going to be as trivial as this one. So far in the live update we allow only disk->src to change. We even have a function to check that and reject changing anything else: virDomainDiskDiffersSourceOnly (not to mention, that function does not check many fields).

Anyway, I am not sure what approach to take. Maybe I could turn the virDomainDiskDiffersSourceOnly into qemuDomainDiskChangeSupported which would reject updating all the unsupported fields within virDomainDiskDef. But then I'm going to need virDomainDiskDiffersSource or how to name it, that will return true if disk source needs updating. Then I'm going to need "a function" (bare 'if' would be enough) to check on startupPolicy change. Something like following:

static int
qemuDomainChangeDiskMediaLive()
{

   switch (disk->device) {
    case VIR_DOMAIN_DISK_DEVICE_CDROM:
    case VIR_DOMAIN_DISK_DEVICE_FLOPPY:
        if (!(orig_disk = virDomainDiskFindByBusAndDst(vm->def,
                                                       disk->bus, disk->dst)))
            goto end;

        if (!qemuDomainDiskChangeSupported(disk, orig_disk)
            goto end;

        if (virDomainDiskDiffersSource(disk, orig_disk) {
           change_media();
        }

        if (disk->startupPolicy != orig_disk) {
           orig_disk->startupPolicy = disk->startupPolicy;
        }

        ret = 0;
   }
}


My question is, does this sound reasonable? Can this patch be taken in, while I'm working on the live part?

Michal