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

Message ID 5b141b336816fb546417579055a8edfe8f5cc0eb.1570482718.git.crobinso@redhat.com
State Accepted
Commit c87784be89343642bda6c3020362173b0936b2da
Headers show
Series
  • storagefile, security: qcow2 data_file support
Related show

Commit Message

Cole Robinson Oct. 7, 2019, 9:49 p.m.
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(-)

-- 
2.23.0

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

Comments

Daniel Henrique Barboza Oct. 9, 2019, 9:14 p.m. | #1
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
Cole Robinson Oct. 11, 2019, 5:47 p.m. | #2
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

Patch

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;