>
> On Fri, Apr 28, 2017 at 07:57:31AM -0400, Frediano Ziglio wrote:
> > >
> > > On Fri, Apr 28, 2017 at 05:25:31AM -0400, Frediano Ziglio wrote:
> > > > >
> > > > > This is intended to hold the fields that only char-device.c has a use
> > > > > for, but for now this only adds the boilerplate for it, the next
> > > > > commit
> > > > > will move the relevant field there.
> > > > >
> > > > > Signed-off-by: Christophe Fergeau <cfergeau@xxxxxxxxxx>
> > > > > ---
> > > > > server/char-device.c | 5 +++++
> > > > > server/char-device.h | 3 +++
> > > > > 2 files changed, 8 insertions(+)
> > > > >
> > > > > diff --git a/server/char-device.c b/server/char-device.c
> > > > > index d32593209..fa137e416 100644
> > > > > --- a/server/char-device.c
> > > > > +++ b/server/char-device.c
> > > > > @@ -31,6 +31,9 @@
> > > > > #define RED_CHAR_DEVICE_WAIT_TOKENS_TIMEOUT 30000
> > > > > #define MAX_POOL_SIZE (10 * 64 * 1024)
> > > > >
> > > > > +struct RedCharDeviceWriteBufferPrivate {
> > > > > +};
> > > > > +
> > > > > typedef struct RedCharDeviceClient RedCharDeviceClient;
> > > > > struct RedCharDeviceClient {
> > > > > RedCharDevice *dev;
> > > > > @@ -144,6 +147,7 @@ static void
> > > > > red_char_device_write_buffer_free(RedCharDeviceWriteBuffer *buf)
> > > > > return;
> > > > >
> > > > > free(buf->buf);
> > > > > + free(buf->priv);
> > > > > free(buf);
> > > > > }
> > > > >
> > > > > @@ -540,6 +544,7 @@ static RedCharDeviceWriteBuffer
> > > > > *__red_char_device_write_buffer_get(
> > > > > dev->priv->cur_pool_size -= ret->buf_size;
> > > > > } else {
> > > > > ret = spice_new0(RedCharDeviceWriteBuffer, 1);
> > > > > + ret->priv = spice_new0(RedCharDeviceWriteBufferPrivate, 1);
> > > > > }
> > > > >
> > > > > spice_assert(!ret->buf_used);
> > > >
> > > > Would not be better if this is always allocated here to have a
> > > > public part and a private one after the public to avoid the
> > > > indirection penalty?
> > > >
> > > > So kind of
> > > >
> > > > struct RedCharDeviceWriteBufferPrivate {
> > > > RedCharDeviceWriteBuffer base;
> > > > .. additional fields here ...
> > > > };
> > > >
> > > > ...
> > > >
> > > > ret = &spice_new0(RedCharDeviceWriteBufferPrivate, 1)->base;
> > >
> > > I considered playing tricks like
> > > ret = spice_malloc0(sizeof(RedCharDeviceWriteBuffer) +
> > > sizeof(RedCharDeviceWriteBufferPrivate));
> > > ret->priv = &ret[1];
> > >
> > > to allocate it, but was not sure it was worth it over the simple version
> > > in this patch.
> > >
> >
> > a priv[1] would save you the indirection too.
> > Note that spice_malloc0(sizeof(RedCharDeviceWriteBuffer) +
> > sizeof(RedCharDeviceWriteBufferPrivate)) could be not portable
> > as it assumes the size of the first structure is a multiple of the
> > alignment of the second.
>
> I don't want to go with
>
> struct RedCharDeviceWriteBufferPrivate {
> RedCharDeviceWriteBuffer base;
> .. additional fields here ...
> };
>
> as this would be very different from most spice-server structures. So
> I'd stick with either the initial version, or the suggestion I made.
>
That's why we should move away from C language...
>
> > > I kept the same order as in the initial struct. However, moving things
> > > around does not seem to make a difference size-wise, padding is added to
> > > the
> > > end rather than just after the first member.
> > >
> >
> > Yes, but usually you add fields at the end so you'll use the padding,
> > paying attention to middle holes is harder and if you want to add
> > 2 fields you would have to split the patch filling the hole and adding
> > another field which could be bad if the 2 fields are logically related.
>
> Ok, I moved it.
>
> Christophe
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel