1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH v2 0/8] Couple of storage driver improvements
  4. View patch

[libvirt] [PATCH v2 5/8] storageVolWipePattern: Don't take shortcut to refreshPool()

Michal Privoznik posted 8 patches 6 years, 11 months ago
There is a newer version of this series
[libvirt] [PATCH v2 5/8] storageVolWipePattern: Don't take shortcut to refreshPool()
Posted by Michal Privoznik 6 years, 11 months ago 
In d16f803d780 we've tried to solve an issue that after wiping an
image its format might have changed (e.g. from qcow2 to raw) but
libvirt wasn't probing the image format. We fixed this by calling
virStorageBackendRefreshVolTargetUpdate() which is what
refreshPool() would end up calling. But this shortcut is not good
enough because the function is called only for local types of
volumes (like dir, fs, netfs). But now that more backends support
volume wiping we have to call the function with more caution.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/storage/storage_driver.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 98be434005..cefffdfd64 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -2525,11 +2525,15 @@ storageVolWipePattern(virStorageVolPtr vol,
     if (rc < 0)
         goto cleanup;
 
-    /* Instead of using the refreshVol, since much changes on the target
-     * volume, let's update using the same function as refreshPool would
-     * use when it discovers a volume. The only failure to capture is -1,
-     * we can ignore -2. */
-    if (virStorageBackendRefreshVolTargetUpdate(voldef) == -1)
+    /* For local volumes, Instead of using the refreshVol, since
+     * much changes on the target volume, let's update using the
+     * same function as refreshPool would use when it discovers a
+     * volume. The only failure to capture is -1, we can ignore
+     * -2. */
+    if ((backend->type == VIR_STORAGE_POOL_DIR ||
+         backend->type == VIR_STORAGE_POOL_FS ||
+         backend->type == VIR_STORAGE_POOL_NETFS) &&
+        virStorageBackendRefreshVolTargetUpdate(voldef) == -1)
         goto cleanup;
 
     ret = 0;
-- 
2.19.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 5/8] storageVolWipePattern: Don't take shortcut to refreshPool()
Posted by Pavel Hrdina 6 years, 10 months ago 
On Wed, Mar 06, 2019 at 03:59:15PM +0100, Michal Privoznik wrote:
> In d16f803d780 we've tried to solve an issue that after wiping an
> image its format might have changed (e.g. from qcow2 to raw) but
> libvirt wasn't probing the image format. We fixed this by calling
> virStorageBackendRefreshVolTargetUpdate() which is what
> refreshPool() would end up calling. But this shortcut is not good
> enough because the function is called only for local types of
> volumes (like dir, fs, netfs). But now that more backends support
> volume wiping we have to call the function with more caution.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/storage/storage_driver.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index 98be434005..cefffdfd64 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -2525,11 +2525,15 @@ storageVolWipePattern(virStorageVolPtr vol,
>      if (rc < 0)
>          goto cleanup;
>  
> -    /* Instead of using the refreshVol, since much changes on the target
> -     * volume, let's update using the same function as refreshPool would
> -     * use when it discovers a volume. The only failure to capture is -1,
> -     * we can ignore -2. */
> -    if (virStorageBackendRefreshVolTargetUpdate(voldef) == -1)
> +    /* For local volumes, Instead of using the refreshVol, since
> +     * much changes on the target volume, let's update using the
> +     * same function as refreshPool would use when it discovers a
> +     * volume. The only failure to capture is -1, we can ignore
> +     * -2. */
> +    if ((backend->type == VIR_STORAGE_POOL_DIR ||
> +         backend->type == VIR_STORAGE_POOL_FS ||
> +         backend->type == VIR_STORAGE_POOL_NETFS) &&
> +        virStorageBackendRefreshVolTargetUpdate(voldef) == -1)

virStorageBackendRefreshLocal() is used for vstorage as well and
vstorage supports wiping so this should probably include vstorage.

I don't like this ugly hack, but I guess there is no better and easy
way how to refactor it.

Pavel

>          goto cleanup;
>  
>      ret = 0;
> -- 
> 2.19.2
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 5/8] storageVolWipePattern: Don't take shortcut to refreshPool()
Posted by Michal Prívozník 6 years, 10 months ago 
On 3/15/19 3:14 PM, Pavel Hrdina wrote:
> On Wed, Mar 06, 2019 at 03:59:15PM +0100, Michal Privoznik wrote:
>> In d16f803d780 we've tried to solve an issue that after wiping an
>> image its format might have changed (e.g. from qcow2 to raw) but
>> libvirt wasn't probing the image format. We fixed this by calling
>> virStorageBackendRefreshVolTargetUpdate() which is what
>> refreshPool() would end up calling. But this shortcut is not good
>> enough because the function is called only for local types of
>> volumes (like dir, fs, netfs). But now that more backends support
>> volume wiping we have to call the function with more caution.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/storage/storage_driver.c | 14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
>> index 98be434005..cefffdfd64 100644
>> --- a/src/storage/storage_driver.c
>> +++ b/src/storage/storage_driver.c
>> @@ -2525,11 +2525,15 @@ storageVolWipePattern(virStorageVolPtr vol,
>>      if (rc < 0)
>>          goto cleanup;
>>  
>> -    /* Instead of using the refreshVol, since much changes on the target
>> -     * volume, let's update using the same function as refreshPool would
>> -     * use when it discovers a volume. The only failure to capture is -1,
>> -     * we can ignore -2. */
>> -    if (virStorageBackendRefreshVolTargetUpdate(voldef) == -1)
>> +    /* For local volumes, Instead of using the refreshVol, since
>> +     * much changes on the target volume, let's update using the
>> +     * same function as refreshPool would use when it discovers a
>> +     * volume. The only failure to capture is -1, we can ignore
>> +     * -2. */
>> +    if ((backend->type == VIR_STORAGE_POOL_DIR ||
>> +         backend->type == VIR_STORAGE_POOL_FS ||
>> +         backend->type == VIR_STORAGE_POOL_NETFS) &&
>> +        virStorageBackendRefreshVolTargetUpdate(voldef) == -1)
> 
> virStorageBackendRefreshLocal() is used for vstorage as well and
> vstorage supports wiping so this should probably include vstorage.

Ah, good point. I'm adding it here on the list.

> 
> I don't like this ugly hack, but I guess there is no better and easy
> way how to refactor it.

Right. I don't see any neither.

Michal

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.