[libvirt] [PATCH v2] qemu: disable external snapshot of readonly disk

Nikolay Shirokovskiy posted 1 patch 5 years, 4 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1541750428-47675-1-git-send-email-nshirokovskiy@virtuozzo.com
Test syntax-check passed
src/qemu/qemu_driver.c | 7 +++++++
1 file changed, 7 insertions(+)
[libvirt] [PATCH v2] qemu: disable external snapshot of readonly disk
Posted by Nikolay Shirokovskiy 5 years, 4 months ago
Disable external snapshot of readonly disk for inactive domains
as this operation is not very useful. As to active domains
such snapshot was not possible before already but error message was
not helpful so now it will be better.

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
---

Diff from v1 [1]
================

- move check to qemuDomainSnapshotPrepareDiskExternal
- disable such snapshot for inactive domain as well


[1]  [PATCH] qemu: snapshot: better error for active external readonly disk
https://www.redhat.com/archives/libvir-list/2018-October/msg01322.html
  continues in
https://www.redhat.com/archives/libvir-list/2018-November/msg00265.html


 src/qemu/qemu_driver.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a52e249..e747a5b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14514,6 +14514,13 @@ qemuDomainSnapshotPrepareDiskExternal(virDomainDiskDefPtr disk,
     int ret = -1;
     struct stat st;
 
+    if (disk->src->readonly) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("external snapshot for readonly disk %s "
+                         "is not supported"), disk->dst);
+        return -1;
+    }
+
     if (qemuTranslateSnapshotDiskSourcePool(snapdisk) < 0)
         return -1;
 
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: disable external snapshot of readonly disk
Posted by John Ferlan 5 years, 3 months ago

On 11/9/18 3:00 AM, Nikolay Shirokovskiy wrote:
> Disable external snapshot of readonly disk for inactive domains
> as this operation is not very useful. As to active domains
> such snapshot was not possible before already but error message was
> not helpful so now it will be better.
> 
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> ---
> 
> Diff from v1 [1]
> ================
> 
> - move check to qemuDomainSnapshotPrepareDiskExternal
> - disable such snapshot for inactive domain as well
> 
> 
> [1]  [PATCH] qemu: snapshot: better error for active external readonly disk
> https://www.redhat.com/archives/libvir-list/2018-October/msg01322.html
>   continues in
> https://www.redhat.com/archives/libvir-list/2018-November/msg00265.html
> 
> 
>  src/qemu/qemu_driver.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 

Do you mind if I "merge" some details from the first patch on this to
generate the following commit message?:

    Disable external snapshot of a readonly disk for domains as
    this operation is not very useful. Such a snapshot is not
    possible for active domains but the error message from QEMU
    is more cryptic:

       error: internal error: unable to execute QEMU command 'transaction':
                           Could not create file: Permission denied

    This error at least makes the error more understandable for
active domains and disallows for inactive domains as well.


Reviewed-by: John Ferlan <jferlan@redhat.com>

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: disable external snapshot of readonly disk
Posted by Nikolay Shirokovskiy 5 years, 3 months ago

On 10.12.2018 20:23, John Ferlan wrote:
> 
> 
> On 11/9/18 3:00 AM, Nikolay Shirokovskiy wrote:
>> Disable external snapshot of readonly disk for inactive domains
>> as this operation is not very useful. As to active domains
>> such snapshot was not possible before already but error message was
>> not helpful so now it will be better.
>>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
>> ---
>>
>> Diff from v1 [1]
>> ================
>>
>> - move check to qemuDomainSnapshotPrepareDiskExternal
>> - disable such snapshot for inactive domain as well
>>
>>
>> [1]  [PATCH] qemu: snapshot: better error for active external readonly disk
>> https://www.redhat.com/archives/libvir-list/2018-October/msg01322.html
>>   continues in
>> https://www.redhat.com/archives/libvir-list/2018-November/msg00265.html
>>
>>
>>  src/qemu/qemu_driver.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
> 
> Do you mind if I "merge" some details from the first patch on this to
> generate the following commit message?:
> 
>     Disable external snapshot of a readonly disk for domains as
>     this operation is not very useful. Such a snapshot is not
>     possible for active domains but the error message from QEMU
>     is more cryptic:
> 
>        error: internal error: unable to execute QEMU command 'transaction':
>                            Could not create file: Permission denied
> 
>     This error at least makes the error more understandable for
> active domains and disallows for inactive domains as well.

Not at all.

> 
> 
> Reviewed-by: John Ferlan <jferlan@redhat.com>
> 

Thanx, pushed.

Nikolay

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list