[Spice-devel] [PATCH spice-protocol v3] stats: Avoid holes in SpiceStat structure

[Frediano Ziglio]

> 
> On Fri, Jan 04, 2019 at 08:23:15AM +0000, Frediano Ziglio wrote:
> > The SpiceStat structure can be 20 or 24 bytes depending on alignment.
> > Being a memory mapped structure potentially used with lockless access,
> > it is not good to have it unaligned.
> > The current tool that reads this memory mapped file (reds_stats) is
> 
> This is more "The only tool that we know of", maybe some people built

I'll update the comment.

> other (potentially private/internal) tools, maybe not. So such changes
> should be approached with care.. Since this is not a 'core' spice
> structure, and I'd expect it's probably quickly quite limited when
> someone starts using it, so this particular change is probably fine.
> My understanding is that this change is mandatory for reds_stats to work
> on Windows?
> 

More than mandatory is better to have.
In theory you can have spice-server writing with a given ABI and the
tool reading using the other.  Although this is unlikely there are 2 cases:
- you used a saved file (you can copy in some way the file in /dev/shm)
  on another machine;
- you have spice-server and the tool compiled with different ABI (for
  instance one x86 and the other x64 which can run on the same machine).
Ignoring the endian there are basically 2 ABI of the file.
Nobody complained about the issue (or maybe nobody realized it) until
I noted it. Than I proposed the patch to make reds_stat support both ABIs
using the "file sizeof" trick. As we mainly (only?) support x86 and x64
having 2 ABIs on the same machine cause confusion, I wanted to have a
similar patch also for Linux.

Now... Windows. What the difference between Unix(Linux?) and Windows
for mapped files? On Windows there's no /dev/shm, you can either
create a file on the physical filesystem or create a "file mapping"
object not attached to a file (to map a file you have to create a
file mapping attached to a file and then map the file mapping).
I would personally go for a file mapping without a physical file,
file systems are not famous to be fast on Windows (note: kernel
objects on Windows can have names which could be used to share between
processes). However there's no way to get the size of the file mapping
object so the "file sizeof" trick in reds_stat is not possible.
As spice-server does not support Windows platform making sure
that there's a single file ABI avoids having to use this trick.

We could bump also the file version but it seems overwhelming.

> 
> Christophe
> 
> > able to detect if the structure is either 20 or 24 bytes and act
> > accordingly so changing this structure won't affect the behaviour
> > (unless you have an old tool but as they are usually packaged together
> > this is quite improbable).
> > This will also help on Windows as in that system it is not possible for
> > reds_stats to implement the same discovery trick implemented on Linux.
> > On Windows it is not possible to read the size of the file mapping
> > (on Windows to implement shared memory you can use a file mapping
> > not associated to a file).
> > The structure will change layout only on 32-bit architectures which is
> > not recommended nowadays (the 64-bit platforms that we support align
> > 64-bit integers to 64 bits).
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  spice/stats.h | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > Changes since v2:
> > - typos and grammar.
> > 
> > Changes since v1:
> > - update commit message and comment.
> > 
> > diff --git a/spice/stats.h b/spice/stats.h
> > index d4c45ab..44c6482 100644
> > --- a/spice/stats.h
> > +++ b/spice/stats.h
> > @@ -66,6 +66,8 @@ typedef struct SpiceStat {
> >      uint32_t generation;
> >      uint32_t num_of_nodes;
> >      uint32_t root_index;
> > +    /* to avoid holes in the structure on some platforms */
> > +    uint32_t padding;
> >      SpiceStatNode nodes[];
> >  } SpiceStat;
> >