[PATCH] qemu: Revoke access to mirror on failed blockcopy

[Michal Privoznik]

On 4/15/20 2:09 PM, Pavel Mores wrote:
> On Tue, Apr 14, 2020 at 11:32:08AM +0200, Michal Privoznik wrote:
>> When preparing to do a blockcopy, the mirror image is modified so
>> that QEMU can access it. For instance, the mirror has seclabels
>> set, if it is a NVMe disk it is detached from the host and so on.
>> And usually, the restore is done upon successful finish of the
>> blockcopy operation. But, if something fails then we need to
>> explicitly revoke the access to the mirror image (and thus
>> reattach NVMe disk back to the host).
>>
>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1822538
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>   src/qemu/qemu_driver.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 31f199fdef..9475235f01 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -17950,6 +17950,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
>>       virDomainDiskDefPtr disk = NULL;
>>       int ret = -1;
>>       bool need_unlink = false;
>> +    bool need_revoke = false;
>>       g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
>>       const char *format = NULL;
>>       bool mirror_reuse = !!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT);
>> @@ -18124,6 +18125,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
>>   
>>       if (qemuDomainStorageSourceChainAccessAllow(driver, vm, mirror) < 0)
>>           goto endjob;
>> +    need_revoke = true;
>>   
>>       if (blockdev) {
>>           if (mirror_reuse) {
>> @@ -18240,6 +18242,8 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm,
>>           if (crdata)
>>               qemuBlockStorageSourceAttachRollback(priv->mon, crdata->srcdata[0]);
>>           ignore_value(qemuDomainObjExitMonitor(driver, vm));
>> +        if (need_revoke)
>> +            qemuDomainStorageSourceChainAccessRevoke(driver, vm, mirror);
>>       }
>>       if (need_unlink && virStorageFileUnlink(mirror) < 0)
>>           VIR_WARN("%s", _("unable to remove just-created copy target"));
> 
> Is the revocation code correctly placed though?  I mean, it seems to follow
> from the patch that we need to revoke as soon as
> qemuDomainStorageSourceChainAccessAllow() succeeds.  However, the revocation
> call is conditioned by there being 'data' or 'crdata' (among other things).
> 
> What happens if qemuDomainStorageSourceChainAccessAllow() succeeds but the
> first subsequent attempt to retrieve 'data' or 'crdata' (whichever comes first)
> fails?  These failures are handled by 'goto endjob' which reaches the clean-up
> section apparently with both 'data' and 'crdata' still being NULL but
> 'need_revoke' true. If the above is correct, that would mean the 'if
> (need_revoke)' code is never reached and no revocation performed.
> 
> What am I missing?

Nothing, my patch is broken. I will send v2.

Michal