[Spice-devel,RFC] QXLDevSurfaceCreate: reorder fields to avoid holes on 64-bit arches.

Submitted by Alon Levy on May 22, 2012, 2:27 p.m.

Details

Message ID 1337696826-13447-1-git-send-email-alevy@redhat.com
State Rejected
Headers show

Not browsing as part of any series.

Commit Message

Alon Levy May 22, 2012, 2:27 p.m.
The hole remains uninitialized variable when allocating an instance of
QXLDevSurfaceCreate on the stack and setting each field individually,
producing an uninteresting valgrind warning. The reordering fixes this.
It also requires bumping the version of the server.
---
What's the suggested course here? just ignore this and either add memset in
qemu or just pretend I don't see the uninitialized value in valgrind, or push
this and what - raise the primary version number (and assume all users actually
check for the exact primary and not for a larger then relation..) ?

 server/spice.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/server/spice.h b/server/spice.h
index 77aec92..ef2c6d3 100644
--- a/server/spice.h
+++ b/server/spice.h
@@ -203,6 +203,7 @@  struct QXLDevMemSlot {
 };
 
 struct QXLDevSurfaceCreate {
+    uint64_t mem;
     uint32_t width;
     uint32_t height;
     int32_t stride;
@@ -211,7 +212,6 @@  struct QXLDevSurfaceCreate {
     uint32_t mouse_mode;
     uint32_t flags;
     uint32_t type;
-    uint64_t mem;
     uint32_t group_id;
 };
 

Comments

Hi,

Why not simply memset the struct to 0 before initializing its fields,
that should make valgrind happy and is a good idea in general (as it
puts 0 rather then random crap in fields which you forget to init).

Regards,

Hans


On 05/22/2012 04:27 PM, Alon Levy wrote:
> The hole remains uninitialized variable when allocating an instance of
> QXLDevSurfaceCreate on the stack and setting each field individually,
> producing an uninteresting valgrind warning. The reordering fixes this.
> It also requires bumping the version of the server.
> ---
> What's the suggested course here? just ignore this and either add memset in
> qemu or just pretend I don't see the uninitialized value in valgrind, or push
> this and what - raise the primary version number (and assume all users actually
> check for the exact primary and not for a larger then relation..) ?
>
>   server/spice.h |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/server/spice.h b/server/spice.h
> index 77aec92..ef2c6d3 100644
> --- a/server/spice.h
> +++ b/server/spice.h
> @@ -203,6 +203,7 @@ struct QXLDevMemSlot {
>   };
>
>   struct QXLDevSurfaceCreate {
> +    uint64_t mem;
>       uint32_t width;
>       uint32_t height;
>       int32_t stride;
> @@ -211,7 +212,6 @@ struct QXLDevSurfaceCreate {
>       uint32_t mouse_mode;
>       uint32_t flags;
>       uint32_t type;
> -    uint64_t mem;
>       uint32_t group_id;
>   };
>
On Tue, May 22, 2012 at 04:30:33PM +0200, Hans de Goede wrote:
> Hi,
> 
> Why not simply memset the struct to 0 before initializing its fields,
> that should make valgrind happy and is a good idea in general (as it
> puts 0 rather then random crap in fields which you forget to init).

Sure, that works, but it's ugly. Then again all this churn is ugly too.
Lesser evil I guess.

> 
> Regards,
> 
> Hans
> 
> 
> On 05/22/2012 04:27 PM, Alon Levy wrote:
> >The hole remains uninitialized variable when allocating an instance of
> >QXLDevSurfaceCreate on the stack and setting each field individually,
> >producing an uninteresting valgrind warning. The reordering fixes this.
> >It also requires bumping the version of the server.
> >---
> >What's the suggested course here? just ignore this and either add memset in
> >qemu or just pretend I don't see the uninitialized value in valgrind, or push
> >this and what - raise the primary version number (and assume all users actually
> >check for the exact primary and not for a larger then relation..) ?
> >
> >  server/spice.h |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/server/spice.h b/server/spice.h
> >index 77aec92..ef2c6d3 100644
> >--- a/server/spice.h
> >+++ b/server/spice.h
> >@@ -203,6 +203,7 @@ struct QXLDevMemSlot {
> >  };
> >
> >  struct QXLDevSurfaceCreate {
> >+    uint64_t mem;
> >      uint32_t width;
> >      uint32_t height;
> >      int32_t stride;
> >@@ -211,7 +212,6 @@ struct QXLDevSurfaceCreate {
> >      uint32_t mouse_mode;
> >      uint32_t flags;
> >      uint32_t type;
> >-    uint64_t mem;
> >      uint32_t group_id;
> >  };
> >