[03/30] storagefile: qcow1: Fix check for empty backing file

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

Commit Message

Cole Robinson Oct. 7, 2019, 9:49 p.m.
>From f772b3d91fd the intention of this code seems to be to set

format=NONE when the image does not have a backing file. However
'buf' here is the whole qcow1 file header. What we want to be
checking is 'res' which is the parsed backing file path.
qcowXGetBackingStore sets this to NULL when there's no backing file.

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:26 p.m. | #1
On 10/7/19 6:49 PM, Cole Robinson wrote:
> >From f772b3d91fd the intention of this code seems to be to set


Is this leading '>' a bogus?

> format=NONE when the image does not have a backing file. However

> 'buf' here is the whole qcow1 file header. What we want to be

> checking is 'res' which is the parsed backing file path.

> qcowXGetBackingStore sets this to NULL when there's no backing file.

>

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

> ---


This is unusual. Either the qcow file header 'buf' is being set to \0 when
there is no backing file (which would make the current code work even
when checking the wrong thing) or this is a bug that flew under the radar
from a long time.


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 1549067c48..016c8f0799 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 == BACKING_STORE_OK && *buf == '\0')

> +    if (ret == BACKING_STORE_OK && !*res)

>           *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/9/19 9:26 PM, Daniel Henrique Barboza wrote:
> 

> 

> On 10/7/19 6:49 PM, Cole Robinson wrote:

>> >From f772b3d91fd the intention of this code seems to be to set

> 

> Is this leading '>' a bogus?


That's a feature of git-send-email so that this line is not mistaken for 
e-mail header line "From: Michal Privoznik ....". When Cole pushes these 
patches, it won't appear in git log, nor it shows in his local branch.

> 

>> format=NONE when the image does not have a backing file. However

>> 'buf' here is the whole qcow1 file header. What we want to be

>> checking is 'res' which is the parsed backing file path.

>> qcowXGetBackingStore sets this to NULL when there's no backing file.

>>

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

>> ---

> 

> This is unusual. Either the qcow file header 'buf' is being set to \0 when

> there is no backing file (which would make the current code work even

> when checking the wrong thing) or this is a bug that flew under the radar

> from a long time.

> 


Since this is for qcow1 I wouldn't be surprised if it is the latter one. 
I mean, does anybody even use qcow1 these days?

Michal

--
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 1549067c48..016c8f0799 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 == BACKING_STORE_OK && *buf == '\0')
+    if (ret == BACKING_STORE_OK && !*res)
         *format = VIR_STORAGE_FILE_NONE;
     return ret;
 }