[Qemu-devel] [PATCH] qcow2: Fix segfault in qcow2_invalidate_cache

[Qemu-devel] [PATCH] qcow2: Fix segfault in qcow2_invalidate_cache

[Kevin Wolf]

Need to pass an options QDict to qcow2_open() now. This fixes a segfault
on the migration target with qcow2.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c | 12 ++++++++++--
 block/qcow2.h |  3 +++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index b90dd9e..7fcdc62 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -29,6 +29,7 @@
 #include "block/qcow2.h"
 #include "qemu/error-report.h"
 #include "qapi/qmp/qerror.h"
+#include "qapi/qmp/qbool.h"
 #include "trace.h"
 
 /*
@@ -520,7 +521,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags)
         goto fail;
     }
 
-    s->use_lazy_refcounts = qemu_opt_get_bool(opts, "lazy_refcounts",
+    s->use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
         (s->compatible_features & QCOW2_COMPAT_LAZY_REFCOUNTS));
 
     qemu_opts_del(opts);
@@ -934,6 +935,7 @@ static void qcow2_invalidate_cache(BlockDriverState *bs)
     AES_KEY aes_encrypt_key;
     AES_KEY aes_decrypt_key;
     uint32_t crypt_method = 0;
+    QDict *options;
 
     /*
      * Backing files are read-only which makes all of their metadata immutable,
@@ -948,8 +950,14 @@ static void qcow2_invalidate_cache(BlockDriverState *bs)
 
     qcow2_close(bs);
 
+    options = qdict_new();
+    qdict_put(options, QCOW2_OPT_LAZY_REFCOUNTS,
+              qbool_from_int(s->use_lazy_refcounts));
+
     memset(s, 0, sizeof(BDRVQcowState));
-    qcow2_open(bs, NULL, flags);
+    qcow2_open(bs, options, flags);
+
+    QDECREF(options);
 
     if (crypt_method) {
         s->crypt_method = crypt_method;
diff --git a/block/qcow2.h b/block/qcow2.h
index 103abdb..e4b5e11 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -58,6 +58,9 @@
 
 #define DEFAULT_CLUSTER_SIZE 65536
 
+
+#define QCOW2_OPT_LAZY_REFCOUNTS "lazy_refcounts"
+
 typedef struct QCowHeader {
     uint32_t magic;
     uint32_t version;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH] qcow2: Fix segfault in qcow2_invalidate_cache

[Paolo Bonzini]

Il 18/03/2013 13:10, Kevin Wolf ha scritto:
> Need to pass an options QDict to qcow2_open() now. This fixes a segfault
> on the migration target with qcow2.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.c | 12 ++++++++++--
>  block/qcow2.h |  3 +++
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index b90dd9e..7fcdc62 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -29,6 +29,7 @@
>  #include "block/qcow2.h"
>  #include "qemu/error-report.h"
>  #include "qapi/qmp/qerror.h"
> +#include "qapi/qmp/qbool.h"
>  #include "trace.h"
>  
>  /*
> @@ -520,7 +521,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags)
>          goto fail;
>      }
>  
> -    s->use_lazy_refcounts = qemu_opt_get_bool(opts, "lazy_refcounts",
> +    s->use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
>          (s->compatible_features & QCOW2_COMPAT_LAZY_REFCOUNTS));

Why not add

   s->use_lazy_refcounts ||

to the default, and just use an empty QDict in qcow2_invalidate_cache?

Paolo

>  
>      qemu_opts_del(opts);
> @@ -934,6 +935,7 @@ static void qcow2_invalidate_cache(BlockDriverState *bs)
>      AES_KEY aes_encrypt_key;
>      AES_KEY aes_decrypt_key;
>      uint32_t crypt_method = 0;
> +    QDict *options;
>  
>      /*
>       * Backing files are read-only which makes all of their metadata immutable,
> @@ -948,8 +950,14 @@ static void qcow2_invalidate_cache(BlockDriverState *bs)
>  
>      qcow2_close(bs);
>  
> +    options = qdict_new();
> +    qdict_put(options, QCOW2_OPT_LAZY_REFCOUNTS,
> +              qbool_from_int(s->use_lazy_refcounts));
> +
>      memset(s, 0, sizeof(BDRVQcowState));
> -    qcow2_open(bs, NULL, flags);
> +    qcow2_open(bs, options, flags);
> +
> +    QDECREF(options);
>  
>      if (crypt_method) {
>          s->crypt_method = crypt_method;
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 103abdb..e4b5e11 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -58,6 +58,9 @@
>  
>  #define DEFAULT_CLUSTER_SIZE 65536
>  
> +
> +#define QCOW2_OPT_LAZY_REFCOUNTS "lazy_refcounts"
> +
>  typedef struct QCowHeader {
>      uint32_t magic;
>      uint32_t version;
> 

Re: [Qemu-devel] [PATCH] qcow2: Fix segfault in qcow2_invalidate_cache

[Kevin Wolf]

Am 18.03.2013 um 13:40 hat Paolo Bonzini geschrieben:
> Il 18/03/2013 13:10, Kevin Wolf ha scritto:
> > Need to pass an options QDict to qcow2_open() now. This fixes a segfault
> > on the migration target with qcow2.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/qcow2.c | 12 ++++++++++--
> >  block/qcow2.h |  3 +++
> >  2 files changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index b90dd9e..7fcdc62 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -29,6 +29,7 @@
> >  #include "block/qcow2.h"
> >  #include "qemu/error-report.h"
> >  #include "qapi/qmp/qerror.h"
> > +#include "qapi/qmp/qbool.h"
> >  #include "trace.h"
> >  
> >  /*
> > @@ -520,7 +521,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags)
> >          goto fail;
> >      }
> >  
> > -    s->use_lazy_refcounts = qemu_opt_get_bool(opts, "lazy_refcounts",
> > +    s->use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
> >          (s->compatible_features & QCOW2_COMPAT_LAZY_REFCOUNTS));
> 
> Why not add
> 
>    s->use_lazy_refcounts ||
> 
> to the default, and just use an empty QDict in qcow2_invalidate_cache?

I think it's better not to depend on any previous state in qcow2_open(),
but start from scratch with all options directly passed. Otherwise it
would become easy to lose the overview over the data flow.

Also, qcow2_invalidate_cache clears the state so that all other
information in s is properly reset:

    memset(s, 0, sizeof(BDRVQcowState));

So s->use_lazy_refcounts doesn't even have it's old value any more.

Kevin