[02/30] storagefile: qcow1: Check for BACKING_STORE_OK

Message ID 0a700c71217660dfd47ee117e174635aefb5618a.1570482718.git.crobinso@redhat.com
State Accepted
Commit 9f508ec7ca490640cb03e7fb92e8f1b42a41d3b7
Headers show
Series
  • storagefile, security: qcow2 data_file support
Related show

Commit Message

Cole Robinson Oct. 7, 2019, 9:49 p.m.
Check explicitly for BACKING_STORE_OK and not its 0 value

Signed-off-by: Cole Robinson <crobinso@redhat.com>

---
 src/util/virstoragefile.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
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, 7:20 p.m. | #1
On 10/7/19 6:49 PM, Cole Robinson wrote:
> Check explicitly for BACKING_STORE_OK and not its 0 value

>

> Signed-off-by: Cole Robinson <crobinso@redhat.com>

> ---


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


>   src/util/virstoragefile.c | 2 +-

>   1 file changed, 1 insertion(+), 1 deletion(-)

>

> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c

> index 51726006e7..1549067c48 100644

> --- a/src/util/virstoragefile.c

> +++ b/src/util/virstoragefile.c

> @@ -578,7 +578,7 @@ qcow1GetBackingStore(char **res,

>        * used to store backing format */

>       *format = VIR_STORAGE_FILE_AUTO;

>       ret = qcowXGetBackingStore(res, NULL, buf, buf_size, false);

> -    if (ret == 0 && *buf == '\0')

> +    if (ret == BACKING_STORE_OK && *buf == '\0')

>           *format = VIR_STORAGE_FILE_NONE;

>       return ret;

>   }


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Michal Prívozník Oct. 11, 2019, 1:05 p.m. | #2
On 10/7/19 11:49 PM, Cole Robinson wrote:
> Check explicitly for BACKING_STORE_OK and not its 0 value

> 

> Signed-off-by: Cole Robinson <crobinso@redhat.com>

> ---

>   src/util/virstoragefile.c | 2 +-

>   1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c

> index 51726006e7..1549067c48 100644

> --- a/src/util/virstoragefile.c

> +++ b/src/util/virstoragefile.c

> @@ -578,7 +578,7 @@ qcow1GetBackingStore(char **res,

>        * used to store backing format */

>       *format = VIR_STORAGE_FILE_AUTO;

>       ret = qcowXGetBackingStore(res, NULL, buf, buf_size, false);

> -    if (ret == 0 && *buf == '\0')

> +    if (ret == BACKING_STORE_OK && *buf == '\0')

>           *format = VIR_STORAGE_FILE_NONE;

>       return ret;

>   }

> 


We can make qcowXGetBackingStore() return the enum type instead of plain 
int. But that can be done in a follow up (trivial) patch. When doing 
that, both qcow1GetBackingStore() and qcow2GetBackingStore() and also 
getBackingStore() callback can use the same tretement then.

// after seeing future patches

Ah, you're removing some functions, but you get the idea.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson Oct. 11, 2019, 7:19 p.m. | #3
On 10/11/19 9:05 AM, Michal Privoznik wrote:
> On 10/7/19 11:49 PM, Cole Robinson wrote:
>> Check explicitly for BACKING_STORE_OK and not its 0 value
>>
>> Signed-off-by: Cole Robinson <crobinso@redhat.com>
>> ---
>>   src/util/virstoragefile.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>> index 51726006e7..1549067c48 100644
>> --- a/src/util/virstoragefile.c
>> +++ b/src/util/virstoragefile.c
>> @@ -578,7 +578,7 @@ qcow1GetBackingStore(char **res,
>>        * used to store backing format */
>>       *format = VIR_STORAGE_FILE_AUTO;
>>       ret = qcowXGetBackingStore(res, NULL, buf, buf_size, false);
>> -    if (ret == 0 && *buf == '\0')
>> +    if (ret == BACKING_STORE_OK && *buf == '\0')
>>           *format = VIR_STORAGE_FILE_NONE;
>>       return ret;
>>   }
>>
> 
> We can make qcowXGetBackingStore() return the enum type instead of plain 
> int. But that can be done in a follow up (trivial) patch. When doing 
> that, both qcow1GetBackingStore() and qcow2GetBackingStore() and also 
> getBackingStore() callback can use the same tretement then.
> 
> // after seeing future patches
> 
> Ah, you're removing some functions, but you get the idea.
> 

FYI I've pushed the series now. I think after everything is applied 
there isn't anything left to do here regarding your suggestion? But 
correct me if I'm wrong

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 51726006e7..1549067c48 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -578,7 +578,7 @@  qcow1GetBackingStore(char **res,
      * used to store backing format */
     *format = VIR_STORAGE_FILE_AUTO;
     ret = qcowXGetBackingStore(res, NULL, buf, buf_size, false);
-    if (ret == 0 && *buf == '\0')
+    if (ret == BACKING_STORE_OK && *buf == '\0')
         *format = VIR_STORAGE_FILE_NONE;
     return ret;
 }