storagefile, security: qcow2 data_file support

[libvirt] [PATCH 11/30] storagefile: Rename qcow2GetExtensions 'format' argument

Posted by Cole Robinson 6 years, 4 months ago

To backingFormat, which makes it more clear. Move it to the end of
the argument list which will scale nicer with future patches

Signed-off-by: Cole Robinson <crobinso@redhat.com>
---
 src/util/virstoragefile.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index d6bd38777b..8cd576a463 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -427,9 +427,9 @@ cowGetBackingStore(char **res,
 
 
 static int
-qcow2GetExtensions(int *format,
-                   const char *buf,
-                   size_t buf_size)
+qcow2GetExtensions(const char *buf,
+                   size_t buf_size,
+                   int *backingFormat)
 {
     size_t offset;
     size_t extension_start;
@@ -509,8 +509,8 @@ qcow2GetExtensions(int *format,
         case QCOW2_HDR_EXTENSION_BACKING_FORMAT:
             if (buf[offset+len] != '\0')
                 break;
-            *format = virStorageFileFormatTypeFromString(buf+offset);
-            if (*format <= VIR_STORAGE_FILE_NONE)
+            *backingFormat = virStorageFileFormatTypeFromString(buf+offset);
+            if (*backingFormat <= VIR_STORAGE_FILE_NONE)
                 return -1;
         }
 
@@ -561,7 +561,7 @@ qcowXGetBackingStore(char **res,
     memcpy(*res, buf + offset, size);
     (*res)[size] = '\0';
 
-    if (qcow2GetExtensions(format, buf, buf_size) < 0)
+    if (qcow2GetExtensions(buf, buf_size, format) < 0)
         return BACKING_STORE_INVALID;
 
     return BACKING_STORE_OK;
-- 
2.23.0

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

Re: [libvirt] [PATCH 11/30] storagefile: Rename qcow2GetExtensions 'format' argument

Posted by Daniel Henrique Barboza 6 years, 4 months ago


On 10/7/19 6:49 PM, Cole Robinson wrote:
> To backingFormat, which makes it more clear. Move it to the end of
> the argument list which will scale nicer with future patches
>
> Signed-off-by: Cole Robinson <crobinso@redhat.com>
> ---

This really makes it clearer. I was getting confused about whether
'format' was referring to the image or its backing file in these
patches.



>   src/util/virstoragefile.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index d6bd38777b..8cd576a463 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -427,9 +427,9 @@ cowGetBackingStore(char **res,
>   
>   
>   static int
> -qcow2GetExtensions(int *format,
> -                   const char *buf,
> -                   size_t buf_size)
> +qcow2GetExtensions(const char *buf,
> +                   size_t buf_size,
> +                   int *backingFormat)
>   {
>       size_t offset;
>       size_t extension_start;
> @@ -509,8 +509,8 @@ qcow2GetExtensions(int *format,
>           case QCOW2_HDR_EXTENSION_BACKING_FORMAT:
>               if (buf[offset+len] != '\0')
>                   break;
> -            *format = virStorageFileFormatTypeFromString(buf+offset);
> -            if (*format <= VIR_STORAGE_FILE_NONE)
> +            *backingFormat = virStorageFileFormatTypeFromString(buf+offset);
> +            if (*backingFormat <= VIR_STORAGE_FILE_NONE)
>                   return -1;
>           }
>   
> @@ -561,7 +561,7 @@ qcowXGetBackingStore(char **res,
>       memcpy(*res, buf + offset, size);
>       (*res)[size] = '\0';
>   
> -    if (qcow2GetExtensions(format, buf, buf_size) < 0)
> +    if (qcow2GetExtensions(buf, buf_size, format) < 0)
>           return BACKING_STORE_INVALID;

Not sure if the next patches are changing it or making it obsolete, but 
you can
change 'format' to 'backingFormat' inside qcowXGetBackingStore too.



Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   
>       return BACKING_STORE_OK;

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

Re: [libvirt] [PATCH 11/30] storagefile: Rename qcow2GetExtensions 'format' argument

Posted by Cole Robinson 6 years, 3 months ago

On 10/9/19 5:14 PM, Daniel Henrique Barboza wrote:
> 
> 
> On 10/7/19 6:49 PM, Cole Robinson wrote:
>> To backingFormat, which makes it more clear. Move it to the end of
>> the argument list which will scale nicer with future patches
>>
>> Signed-off-by: Cole Robinson <crobinso@redhat.com>
>> ---
> 
> This really makes it clearer. I was getting confused about whether
> 'format' was referring to the image or its backing file in these
> patches.
> 
> 
> 
>>   src/util/virstoragefile.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>> index d6bd38777b..8cd576a463 100644
>> --- a/src/util/virstoragefile.c
>> +++ b/src/util/virstoragefile.c
>> @@ -427,9 +427,9 @@ cowGetBackingStore(char **res,
>>   static int
>> -qcow2GetExtensions(int *format,
>> -                   const char *buf,
>> -                   size_t buf_size)
>> +qcow2GetExtensions(const char *buf,
>> +                   size_t buf_size,
>> +                   int *backingFormat)
>>   {
>>       size_t offset;
>>       size_t extension_start;
>> @@ -509,8 +509,8 @@ qcow2GetExtensions(int *format,
>>           case QCOW2_HDR_EXTENSION_BACKING_FORMAT:
>>               if (buf[offset+len] != '\0')
>>                   break;
>> -            *format = virStorageFileFormatTypeFromString(buf+offset);
>> -            if (*format <= VIR_STORAGE_FILE_NONE)
>> +            *backingFormat = 
>> virStorageFileFormatTypeFromString(buf+offset);
>> +            if (*backingFormat <= VIR_STORAGE_FILE_NONE)
>>                   return -1;
>>           }
>> @@ -561,7 +561,7 @@ qcowXGetBackingStore(char **res,
>>       memcpy(*res, buf + offset, size);
>>       (*res)[size] = '\0';
>> -    if (qcow2GetExtensions(format, buf, buf_size) < 0)
>> +    if (qcow2GetExtensions(buf, buf_size, format) < 0)
>>           return BACKING_STORE_INVALID;
> 
> Not sure if the next patches are changing it or making it obsolete, but 
> you can
> change 'format' to 'backingFormat' inside qcowXGetBackingStore too.
> 

Later patches don't change it, as you've seen. We could change it, but 
context wise it's less necessary there IMO, and also we should then 
change it for the other GetBackingStore functions as well.

Thanks,
Cole

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