Re: [Spice-devel] [PATCH spice-server v3 06/23] spicevmc: Use GLib memory functions

2017-09-25 Thread Jonathon Jongsma 
Acked-by: Jonathon Jongsma 

On Mon, 2017-09-25 at 09:25 +0100, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/spicevmc.c | 14 +++---
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> Changes since v2:
> - move RedPipeItem stuff in proper patch.
> 
> diff --git a/server/spicevmc.c b/server/spicevmc.c
> index 6b9b96fc8..f56f50eda 100644
> --- a/server/spicevmc.c
> +++ b/server/spicevmc.c
> @@ -437,18 +437,18 @@ static void red_port_init_item_free(struct
> RedPipeItem *base)
>  {
>  RedPortInitPipeItem *item = SPICE_UPCAST(RedPortInitPipeItem,
> base);
>  
> -free(item->name);
> -free(item);
> +g_free(item->name);
> +g_free(item);
>  }
>  
>  static void spicevmc_port_send_init(RedChannelClient *rcc)
>  {
>  RedVmcChannel *channel =
> RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
>  SpiceCharDeviceInstance *sin = channel->chardev_sin;
> -RedPortInitPipeItem *item = spice_new(RedPortInitPipeItem, 1);
> +RedPortInitPipeItem *item = g_new(RedPortInitPipeItem, 1);
>  
>  red_pipe_item_init_full(>base,
> RED_PIPE_ITEM_TYPE_PORT_INIT, red_port_init_item_free);
> -item->name = strdup(sin->portname);
> +item->name = g_strdup(sin->portname);
>  item->opened = channel->port_opened;
>  red_channel_client_pipe_add_push(rcc, >base);
>  }
> @@ -589,7 +589,7 @@ static bool
> spicevmc_red_channel_client_handle_message(RedChannelClient *rcc,
> uint32_t
> size,
> void *msg)
>  {
> -/* NOTE: *msg free by free() (when cb to
> spicevmc_red_channel_release_msg_rcv_buf
> +/* NOTE: *msg free by g_free() (when cb to
> spicevmc_red_channel_release_msg_rcv_buf
>   * with the compressed msg type) */
>  RedVmcChannel *channel;
>  SpiceCharDeviceInterface *sif;
> @@ -646,7 +646,7 @@ static uint8_t
> *spicevmc_red_channel_alloc_msg_rcv_buf(RedChannelClient *rcc,
>  }
>  
>  default:
> -return spice_malloc(size);
> +return g_malloc(size);
>  }
>  
>  }
> @@ -665,7 +665,7 @@ static void
> spicevmc_red_channel_release_msg_rcv_buf(RedChannelClient *rcc,
>  break;
>  }
>  default:
> -free(msg);
> +g_free(msg);
>  }
>  }
>  
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-common v2 1/3] Make the compiler work out better way to write unaligned memory

2017-09-25 Thread Christophe Fergeau 
On Mon, Sep 25, 2017 at 09:04:16AM -0400, Frediano Ziglio wrote:
> Yes, intentional and consistent with previous code.
> The usage of unsigned for BYTESWAP avoid sign extension during arithmetic.

If BYTESWAP has such issues (and I don't think it does), then this should be 
fixed.
ack on being consistent with what was done before.

Christophe
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-common v2 1/3] Make the compiler work out better way to write unaligned memory

2017-09-25 Thread Frediano Ziglio 
> 
> On Mon, Sep 25, 2017 at 09:04:16AM -0400, Frediano Ziglio wrote:
> > Yes, intentional and consistent with previous code.
> > The usage of unsigned for BYTESWAP avoid sign extension during arithmetic.
> 
> If BYTESWAP has such issues (and I don't think it does), then this should be
> fixed.
> ack on being consistent with what was done before.
> 
> Christophe
> 

Looking at macro sources do not seem to have such issue.

OT: Looking at macro sources why don't we use asm/swab.h or byteswap.h on Linux?
They provide support for more architectures. Or something like

--- a/spice/macros.h
+++ b/spice/macros.h
@@ -248,7 +248,11 @@
 
 /* Arch specific stuff for speed
  */
-#if defined (__GNUC__) && (__GNUC__ >= 2) && defined (__OPTIMIZE__)
+#if defined (__GNUC__) && ((__GNUC__ > 4) || (__GNUC__ == 4 && __GNUC_MINOR__ 
>= 3))
+#  define SPICE_BYTESWAP16(val) __builtin_bswap16(val)
+#  define SPICE_BYTESWAP32(val) __builtin_bswap32(val)
+#  define SPICE_BYTESWAP64(val) __builtin_bswap64(val)
+#elif defined (__GNUC__) && (__GNUC__ >= 2) && defined (__OPTIMIZE__)
 #  if defined (__i386__)
 #define SPICE_BYTESWAP16_IA32(val) \
(__extension__  \

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-common v2 1/3] Make the compiler work out better way to write unaligned memory

2017-09-25 Thread Christophe Fergeau 
On Mon, Sep 25, 2017 at 10:22:57AM -0400, Frediano Ziglio wrote:
> > 
> > On Mon, Sep 25, 2017 at 09:04:16AM -0400, Frediano Ziglio wrote:
> > > Yes, intentional and consistent with previous code.
> > > The usage of unsigned for BYTESWAP avoid sign extension during arithmetic.
> > 
> > If BYTESWAP has such issues (and I don't think it does), then this should be
> > fixed.
> > ack on being consistent with what was done before.
> > 
> > Christophe
> > 
> 
> Looking at macro sources do not seem to have such issue.
> 
> OT: Looking at macro sources why don't we use asm/swab.h or byteswap.h on 
> Linux?
> They provide support for more architectures. Or something like

Even el6 has gcc 4.4, I'd just drop the whole #elif defined (__GNUC__) && 
(__GNUC__ >= 2) && defined (__OPTIMIZE__)
block and only keep __builtin_bswap*() and the generic fallback.

Christophe
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-common v2 1/3] Make the compiler work out better way to write unaligned memory

2017-09-25 Thread Frediano Ziglio 
> 
> On Fri, Sep 22, 2017 at 11:10:02AM +0100, Frediano Ziglio wrote:
> > Instead of assuming that the system can safely do unaligned access
> > to memory use packed structures to allow the compiler generate
> > best code possible.
> > A packed structure tells the compiler to not leave padding inside it
> > and that the structure can be unaligned so any field can be unaligned
> > having to generate proper access code based on architecture.
> > For instance ARM7 can use unaligned access but not for 64 bit
> > numbers (currently these accesses are emulated by Linux kernel
> > with obvious performance consequences).
> > 
> > This changes the current methods from:
> > 
> > #ifdef WORDS_BIGENDIAN
> > #define read_uint32(ptr) ((uint32_t)SPICE_BYTESWAP32(*((uint32_t *)(ptr
> > #define write_uint32(ptr, val) *(uint32_t *)(ptr) =
> > SPICE_BYTESWAP32((uint32_t)val)
> > #else
> > #define read_uint32(ptr) (*((uint32_t *)(ptr)))
> > #define write_uint32(ptr, val) (*((uint32_t *)(ptr))) = val
> > #endif
> > 
> > to:
> > 
> > #include 
> > typedef struct SPICE_ATTR_PACKED {
> > uint32_t v;
> > } uint32_unaligned_t;
> > #include 
> > 
> > #ifdef WORDS_BIGENDIAN
> > #define read_uint32(ptr) ((uint32_t)SPICE_BYTESWAP32(((uint32_unaligned_t
> > *)(ptr))->v))
> 
> For int32, this generates
> #define read_uint32(ptr) ((int32_t)SPICE_BYTESWAP32(((uint32_unaligned_t
> *)(ptr))->v))
> rather than
> #define read_uint32(ptr) ((int32_t)SPICE_BYTESWAP32(((int32_unaligned_t
> *)(ptr))->v))
> I don't know if this is intentional?

Yes, intentional and consistent with previous code.
The usage of unsigned for BYTESWAP avoid sign extension during arithmetic.

> (the change would be:
> 
> diff --git a/python_modules/demarshal.py b/python_modules/demarshal.py
> index da87d44..241e620 100644
> --- a/python_modules/demarshal.py
> +++ b/python_modules/demarshal.py
> @@ -63,7 +63,7 @@ def write_parser_helpers(writer):
>  utype = "uint%d" % (size)
>  type = "%sint%d" % (sign, size)
>  swap = "SPICE_BYTESWAP%d" % size
> -writer.macro("read_%s" % type, "ptr",
> "((%s_t)%s(((%s_unaligned_t *)(ptr))->v))" % (type, swap, utype))
> +writer.macro("read_%s" % type, "ptr",
> "((%s_t)%s(((%s_unaligned_t *)(ptr))->v))" % (type, swap, type))
>  writer.macro("write_%s" % type, "ptr, val", "((%s_unaligned_t
>  *)(ptr))->v = %s((%s_t)val)" % (utype, swap, utype))
>  writer.writeln("#else")
>  for size in [16, 32, 64]:
> 
> Apart from this, looks good.
> 
> Christophe
> 

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-common v2 1/3] Make the compiler work out better way to write unaligned memory

2017-09-25 Thread Christophe Fergeau 
On Fri, Sep 22, 2017 at 11:10:02AM +0100, Frediano Ziglio wrote:
> Instead of assuming that the system can safely do unaligned access
> to memory use packed structures to allow the compiler generate
> best code possible.
> A packed structure tells the compiler to not leave padding inside it
> and that the structure can be unaligned so any field can be unaligned
> having to generate proper access code based on architecture.
> For instance ARM7 can use unaligned access but not for 64 bit
> numbers (currently these accesses are emulated by Linux kernel
> with obvious performance consequences).
> 
> This changes the current methods from:
> 
> #ifdef WORDS_BIGENDIAN
> #define read_uint32(ptr) ((uint32_t)SPICE_BYTESWAP32(*((uint32_t *)(ptr
> #define write_uint32(ptr, val) *(uint32_t *)(ptr) = 
> SPICE_BYTESWAP32((uint32_t)val)
> #else
> #define read_uint32(ptr) (*((uint32_t *)(ptr)))
> #define write_uint32(ptr, val) (*((uint32_t *)(ptr))) = val
> #endif
> 
> to:
> 
> #include 
> typedef struct SPICE_ATTR_PACKED {
> uint32_t v;
> } uint32_unaligned_t;
> #include 
> 
> #ifdef WORDS_BIGENDIAN
> #define read_uint32(ptr) ((uint32_t)SPICE_BYTESWAP32(((uint32_unaligned_t 
> *)(ptr))->v))

For int32, this generates
#define read_uint32(ptr) ((int32_t)SPICE_BYTESWAP32(((uint32_unaligned_t 
*)(ptr))->v))
rather than
#define read_uint32(ptr) ((int32_t)SPICE_BYTESWAP32(((int32_unaligned_t 
*)(ptr))->v))
I don't know if this is intentional?
(the change would be:

diff --git a/python_modules/demarshal.py b/python_modules/demarshal.py
index da87d44..241e620 100644
--- a/python_modules/demarshal.py
+++ b/python_modules/demarshal.py
@@ -63,7 +63,7 @@ def write_parser_helpers(writer):
 utype = "uint%d" % (size)
 type = "%sint%d" % (sign, size)
 swap = "SPICE_BYTESWAP%d" % size
-writer.macro("read_%s" % type, "ptr", "((%s_t)%s(((%s_unaligned_t 
*)(ptr))->v))" % (type, swap, utype))
+writer.macro("read_%s" % type, "ptr", "((%s_t)%s(((%s_unaligned_t 
*)(ptr))->v))" % (type, swap, type))
 writer.macro("write_%s" % type, "ptr, val", "((%s_unaligned_t 
*)(ptr))->v = %s((%s_t)val)" % (utype, swap, utype))
 writer.writeln("#else")
 for size in [16, 32, 64]:

Apart from this, looks good.

Christophe
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server v2 02/23] mjpeg: Use GLib memory functions

2017-09-25 Thread Frediano Ziglio 
> 
> On Wed, 2017-09-20 at 08:50 +0100, Frediano Ziglio wrote:
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  server/mjpeg-encoder.c | 28 ++--
> >  1 file changed, 14 insertions(+), 14 deletions(-)
> > 
> > diff --git a/server/mjpeg-encoder.c b/server/mjpeg-encoder.c
> > index 07ca69c20..cb70ab1b5 100644
> > --- a/server/mjpeg-encoder.c
> > +++ b/server/mjpeg-encoder.c
> > @@ -191,18 +191,18 @@ static uint32_t
> > get_min_required_playback_delay(uint64_t frame_enc_size,
> >  static void mjpeg_video_buffer_free(VideoBuffer *video_buffer)
> >  {
> >  MJpegVideoBuffer *buffer = (MJpegVideoBuffer*)video_buffer;
> > -free(buffer->base.data);
> > -free(buffer);
> > +g_free(buffer->base.data);
> > +g_free(buffer);
> >  }
> >  
> >  static MJpegVideoBuffer* create_mjpeg_video_buffer(void)
> >  {
> > -MJpegVideoBuffer *buffer = spice_new0(MJpegVideoBuffer, 1);
> > +MJpegVideoBuffer *buffer = g_new0(MJpegVideoBuffer, 1);
> >  buffer->base.free = mjpeg_video_buffer_free;
> >  buffer->maxsize = MJPEG_INITIAL_BUFFER_SIZE;
> > -buffer->base.data = malloc(buffer->maxsize);
> > +buffer->base.data = g_try_malloc(buffer->maxsize);
> >  if (!buffer->base.data) {
> > -free(buffer);
> > +g_free(buffer);
> >  buffer = NULL;
> >  }
> >  return buffer;
> > @@ -211,10 +211,10 @@ static MJpegVideoBuffer*
> > create_mjpeg_video_buffer(void)
> >  static void mjpeg_encoder_destroy(VideoEncoder *video_encoder)
> >  {
> >  MJpegEncoder *encoder = (MJpegEncoder*)video_encoder;
> > -free(encoder->cinfo.dest);
> > +g_free(encoder->cinfo.dest);
> >  jpeg_destroy_compress(>cinfo);
> > -free(encoder->row);
> > -free(encoder);
> > +g_free(encoder->row);
> > +g_free(encoder);
> >  }
> >  
> >  static uint8_t mjpeg_encoder_get_bytes_per_pixel(MJpegEncoder
> > *encoder)
> > @@ -278,7 +278,7 @@ static boolean
> > empty_mem_output_buffer(j_compress_ptr cinfo)
> >  
> >/* Try to allocate new buffer with double size */
> >nextsize = dest->bufsize * 2;
> > -  nextbuffer = realloc(dest->buffer, nextsize);
> > +  nextbuffer = g_try_realloc(dest->buffer, nextsize);
> >  
> >if (nextbuffer == NULL)
> >  ERREXIT1(cinfo, JERR_OUT_OF_MEMORY, 10);
> > @@ -304,7 +304,7 @@ static void term_mem_destination(j_compress_ptr
> > cinfo)
> >   * Prepare for output to a memory buffer.
> >   * The caller must supply its own initial buffer and size.
> >   * When the actual data output exceeds the given size, the library
> > - * will adapt the buffer size as necessary using the malloc()/free()
> > + * will adapt the buffer size as necessary using the
> > g_malloc()/g_free()
> >   * functions. The buffer is available to the application after the
> >   * compression and the application is then responsible for freeing
> > it.
> >   */
> > @@ -323,7 +323,7 @@ spice_jpeg_mem_dest(j_compress_ptr cinfo,
> > * can be written to the same buffer without re-executing
> > jpeg_mem_dest.
> > */
> >if (cinfo->dest == NULL) { /* first time for this JPEG object? */
> > -cinfo->dest = spice_malloc(sizeof(mem_destination_mgr));
> > +cinfo->dest = g_malloc(sizeof(mem_destination_mgr));
> 
> I'd be tempted to change this to g_new() or g_new0(), but maybe that's
> just me.
> 

However dest is not mem_destination_mgr so would be something like

  cinfo->dest = &(g_new(mem_destination_mgr, 1))->pub;

or

  cinfo->dest = (jpeg_destination_msg *) g_new(mem_destination_mgr, 1);

> Acked-by: Jonathon Jongsma 
> 
> >}
> >  
> >dest = (mem_destination_mgr *) cinfo->dest;
> > @@ -700,7 +700,7 @@ static void mjpeg_encoder_adjust_fps(MJpegEncoder
> > *encoder, uint64_t now)
> >  }
> >  
> >  /*
> > - * dest must be either NULL or allocated by malloc, since it might
> > be freed
> > + * dest must be either NULL or allocated by g_malloc, since it might
> > be freed
> >   * during the encoding, if its size is too small.
> >   *
> >   * return:
> > @@ -790,7 +790,7 @@ static int mjpeg_encoder_start_frame(MJpegEncoder
> > *encoder,
> >  return VIDEO_ENCODER_FRAME_UNSUPPORTED;
> >  }
> >  if (encoder->row_size < stride) {
> > -encoder->row = spice_realloc(encoder->row, stride);
> > +encoder->row = g_realloc(encoder->row, stride);
> >  encoder->row_size = stride;
> >  }
> >  }
> > @@ -1357,7 +1357,7 @@ VideoEncoder
> > *mjpeg_encoder_new(SpiceVideoCodecType codec_type,
> >  
> >  spice_return_val_if_fail(codec_type ==
> > SPICE_VIDEO_CODEC_TYPE_MJPEG, NULL);
> >  
> > -encoder = spice_new0(MJpegEncoder, 1);
> > +encoder = g_new0(MJpegEncoder, 1);
> >  encoder->base.destroy = mjpeg_encoder_destroy;
> >  encoder->base.encode_frame = mjpeg_encoder_encode_frame;
> >  encoder->base.client_stream_report =
> > mjpeg_encoder_client_stream_report;
> 
___

[Spice-devel] [PATCH spice-server v3 06/23] spicevmc: Use GLib memory functions

2017-09-25 Thread Frediano Ziglio 
Signed-off-by: Frediano Ziglio 
---
 server/spicevmc.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

Changes since v2:
- move RedPipeItem stuff in proper patch.

diff --git a/server/spicevmc.c b/server/spicevmc.c
index 6b9b96fc8..f56f50eda 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -437,18 +437,18 @@ static void red_port_init_item_free(struct RedPipeItem 
*base)
 {
 RedPortInitPipeItem *item = SPICE_UPCAST(RedPortInitPipeItem, base);
 
-free(item->name);
-free(item);
+g_free(item->name);
+g_free(item);
 }
 
 static void spicevmc_port_send_init(RedChannelClient *rcc)
 {
 RedVmcChannel *channel = 
RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
 SpiceCharDeviceInstance *sin = channel->chardev_sin;
-RedPortInitPipeItem *item = spice_new(RedPortInitPipeItem, 1);
+RedPortInitPipeItem *item = g_new(RedPortInitPipeItem, 1);
 
 red_pipe_item_init_full(>base, RED_PIPE_ITEM_TYPE_PORT_INIT, 
red_port_init_item_free);
-item->name = strdup(sin->portname);
+item->name = g_strdup(sin->portname);
 item->opened = channel->port_opened;
 red_channel_client_pipe_add_push(rcc, >base);
 }
@@ -589,7 +589,7 @@ static bool 
spicevmc_red_channel_client_handle_message(RedChannelClient *rcc,
uint32_t size,
void *msg)
 {
-/* NOTE: *msg free by free() (when cb to 
spicevmc_red_channel_release_msg_rcv_buf
+/* NOTE: *msg free by g_free() (when cb to 
spicevmc_red_channel_release_msg_rcv_buf
  * with the compressed msg type) */
 RedVmcChannel *channel;
 SpiceCharDeviceInterface *sif;
@@ -646,7 +646,7 @@ static uint8_t 
*spicevmc_red_channel_alloc_msg_rcv_buf(RedChannelClient *rcc,
 }
 
 default:
-return spice_malloc(size);
+return g_malloc(size);
 }
 
 }
@@ -665,7 +665,7 @@ static void 
spicevmc_red_channel_release_msg_rcv_buf(RedChannelClient *rcc,
 break;
 }
 default:
-free(msg);
+g_free(msg);
 }
 }
 
-- 
2.13.5

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server v2 02/23] mjpeg: Use GLib memory functions

2017-09-25 Thread Jonathon Jongsma 
On Mon, 2017-09-25 at 03:56 -0400, Frediano Ziglio wrote:
> > 
> > On Wed, 2017-09-20 at 08:50 +0100, Frediano Ziglio wrote:
> > > Signed-off-by: Frediano Ziglio 
> > > ---
> > >  server/mjpeg-encoder.c | 28 ++--
> > >  1 file changed, 14 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/server/mjpeg-encoder.c b/server/mjpeg-encoder.c
> > > index 07ca69c20..cb70ab1b5 100644
> > > --- a/server/mjpeg-encoder.c
> > > +++ b/server/mjpeg-encoder.c
> > > @@ -191,18 +191,18 @@ static uint32_t
> > > get_min_required_playback_delay(uint64_t frame_enc_size,
> > >  static void mjpeg_video_buffer_free(VideoBuffer *video_buffer)
> > >  {
> > >  MJpegVideoBuffer *buffer = (MJpegVideoBuffer*)video_buffer;
> > > -free(buffer->base.data);
> > > -free(buffer);
> > > +g_free(buffer->base.data);
> > > +g_free(buffer);
> > >  }
> > >  
> > >  static MJpegVideoBuffer* create_mjpeg_video_buffer(void)
> > >  {
> > > -MJpegVideoBuffer *buffer = spice_new0(MJpegVideoBuffer, 1);
> > > +MJpegVideoBuffer *buffer = g_new0(MJpegVideoBuffer, 1);
> > >  buffer->base.free = mjpeg_video_buffer_free;
> > >  buffer->maxsize = MJPEG_INITIAL_BUFFER_SIZE;
> > > -buffer->base.data = malloc(buffer->maxsize);
> > > +buffer->base.data = g_try_malloc(buffer->maxsize);
> > >  if (!buffer->base.data) {
> > > -free(buffer);
> > > +g_free(buffer);
> > >  buffer = NULL;
> > >  }
> > >  return buffer;
> > > @@ -211,10 +211,10 @@ static MJpegVideoBuffer*
> > > create_mjpeg_video_buffer(void)
> > >  static void mjpeg_encoder_destroy(VideoEncoder *video_encoder)
> > >  {
> > >  MJpegEncoder *encoder = (MJpegEncoder*)video_encoder;
> > > -free(encoder->cinfo.dest);
> > > +g_free(encoder->cinfo.dest);
> > >  jpeg_destroy_compress(>cinfo);
> > > -free(encoder->row);
> > > -free(encoder);
> > > +g_free(encoder->row);
> > > +g_free(encoder);
> > >  }
> > >  
> > >  static uint8_t mjpeg_encoder_get_bytes_per_pixel(MJpegEncoder
> > > *encoder)
> > > @@ -278,7 +278,7 @@ static boolean
> > > empty_mem_output_buffer(j_compress_ptr cinfo)
> > >  
> > >/* Try to allocate new buffer with double size */
> > >nextsize = dest->bufsize * 2;
> > > -  nextbuffer = realloc(dest->buffer, nextsize);
> > > +  nextbuffer = g_try_realloc(dest->buffer, nextsize);
> > >  
> > >if (nextbuffer == NULL)
> > >  ERREXIT1(cinfo, JERR_OUT_OF_MEMORY, 10);
> > > @@ -304,7 +304,7 @@ static void
> > > term_mem_destination(j_compress_ptr
> > > cinfo)
> > >   * Prepare for output to a memory buffer.
> > >   * The caller must supply its own initial buffer and size.
> > >   * When the actual data output exceeds the given size, the
> > > library
> > > - * will adapt the buffer size as necessary using the
> > > malloc()/free()
> > > + * will adapt the buffer size as necessary using the
> > > g_malloc()/g_free()
> > >   * functions. The buffer is available to the application after
> > > the
> > >   * compression and the application is then responsible for
> > > freeing
> > > it.
> > >   */
> > > @@ -323,7 +323,7 @@ spice_jpeg_mem_dest(j_compress_ptr cinfo,
> > > * can be written to the same buffer without re-executing
> > > jpeg_mem_dest.
> > > */
> > >if (cinfo->dest == NULL) { /* first time for this JPEG object?
> > > */
> > > -cinfo->dest = spice_malloc(sizeof(mem_destination_mgr));
> > > +cinfo->dest = g_malloc(sizeof(mem_destination_mgr));
> > 
> > I'd be tempted to change this to g_new() or g_new0(), but maybe
> > that's
> > just me.
> > 
> 
> However dest is not mem_destination_mgr so would be something like
> 
>   cinfo->dest = &(g_new(mem_destination_mgr, 1))->pub;
> 
> or
> 
>   cinfo->dest = (jpeg_destination_msg *) g_new(mem_destination_mgr,
> 1);

OK, not worth it then.

> 
> > Acked-by: Jonathon Jongsma 
> > 
> > >}
> > >  
> > >dest = (mem_destination_mgr *) cinfo->dest;
> > > @@ -700,7 +700,7 @@ static void
> > > mjpeg_encoder_adjust_fps(MJpegEncoder
> > > *encoder, uint64_t now)
> > >  }
> > >  
> > >  /*
> > > - * dest must be either NULL or allocated by malloc, since it
> > > might
> > > be freed
> > > + * dest must be either NULL or allocated by g_malloc, since it
> > > might
> > > be freed
> > >   * during the encoding, if its size is too small.
> > >   *
> > >   * return:
> > > @@ -790,7 +790,7 @@ static int
> > > mjpeg_encoder_start_frame(MJpegEncoder
> > > *encoder,
> > >  return VIDEO_ENCODER_FRAME_UNSUPPORTED;
> > >  }
> > >  if (encoder->row_size < stride) {
> > > -encoder->row = spice_realloc(encoder->row, stride);
> > > +encoder->row = g_realloc(encoder->row, stride);
> > >  encoder->row_size = stride;
> > >  }
> > >  }
> > > @@ -1357,7 +1357,7 @@ VideoEncoder
> > > *mjpeg_encoder_new(SpiceVideoCodecType codec_type,
> > >  
> > >  spice_return_val_if_fail(codec_type ==
>

Re: [Spice-devel] [PATCH spice-common v2 1/3] Make the compiler work out better way to write unaligned memory

2017-09-25 Thread Christophe Fergeau 
On Mon, Sep 25, 2017 at 04:37:43PM +0200, Christophe Fergeau wrote:
> On Mon, Sep 25, 2017 at 10:22:57AM -0400, Frediano Ziglio wrote:
> > > 
> > > On Mon, Sep 25, 2017 at 09:04:16AM -0400, Frediano Ziglio wrote:
> > > > Yes, intentional and consistent with previous code.
> > > > The usage of unsigned for BYTESWAP avoid sign extension during 
> > > > arithmetic.
> > > 
> > > If BYTESWAP has such issues (and I don't think it does), then this should 
> > > be
> > > fixed.
> > > ack on being consistent with what was done before.
> > > 
> > > Christophe
> > > 
> > 
> > Looking at macro sources do not seem to have such issue.
> > 
> > OT: Looking at macro sources why don't we use asm/swab.h or byteswap.h on 
> > Linux?
> > They provide support for more architectures. Or something like
> 
> Even el6 has gcc 4.4, I'd just drop the whole #elif defined (__GNUC__) && 
> (__GNUC__ >= 2) && defined (__OPTIMIZE__)
> block and only keep __builtin_bswap*() and the generic fallback.


by the way, for the series,
Acked-by: Christophe Fergeau 

> 
> Christophe
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server v3 23/23] red-pipe-item: Use GLib memory functions

2017-09-25 Thread Jonathon Jongsma 
Acked-by: Jonathon Jongsma 


On Mon, 2017-09-25 at 13:20 +0100, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> Acked-by: Jonathon Jongsma 
> ---
>  server/cache-item.tmpl.c  |  6 +++---
>  server/dcc.c  | 10 +-
>  server/inputs-channel.c   |  4 ++--
>  server/main-channel-client.c  | 14 +++---
>  server/red-channel-client.c   |  8 
>  server/red-channel.c  |  2 +-
>  server/red-pipe-item.c|  2 +-
>  server/smartcard-channel-client.c |  2 +-
>  server/spicevmc.c | 10 +-
>  server/stream.c   |  2 +-
>  10 files changed, 30 insertions(+), 30 deletions(-)
> 
> Changes since v2:
> - moved some changes to spicevmc.c from another patch to this patch.
> 
> diff --git a/server/cache-item.tmpl.c b/server/cache-item.tmpl.c
> index 19e6b95f1..44b45f812 100644
> --- a/server/cache-item.tmpl.c
> +++ b/server/cache-item.tmpl.c
> @@ -86,7 +86,7 @@ static int FUNC_NAME(add)(CHANNELCLIENT
> *channel_client, uint64_t id, size_t siz
>  RedCacheItem *item;
>  int key;
>  
> -item = spice_new(RedCacheItem, 1);
> +item = g_new(RedCacheItem, 1);
>  
>  channel_client->priv->VAR_NAME(available) -= size;
>  SPICE_VERIFY(SPICE_OFFSETOF(RedCacheItem, u.cache_data.lru_link)
> == 0);
> @@ -94,7 +94,7 @@ static int FUNC_NAME(add)(CHANNELCLIENT
> *channel_client, uint64_t id, size_t siz
>  RedCacheItem *tail = (RedCacheItem
> *)ring_get_tail(_client->priv->VAR_NAME(lru));
>  if (!tail) {
>  channel_client->priv->VAR_NAME(available) += size;
> -free(item);
> +g_free(item);
>  return FALSE;
>  }
>  FUNC_NAME(remove)(channel_client, tail);
> @@ -117,7 +117,7 @@ static void FUNC_NAME(reset)(CHANNELCLIENT
> *channel_client, long size)
>  while (channel_client->priv->CACHE_NAME[i]) {
>  RedCacheItem *item = channel_client->priv-
> >CACHE_NAME[i];
>  channel_client->priv->CACHE_NAME[i] = item-
> >u.cache_data.next;
> -free(item);
> +g_free(item);
>  }
>  }
>  ring_init(_client->priv->VAR_NAME(lru));
> diff --git a/server/dcc.c b/server/dcc.c
> index f3d53051c..90684e17c 100644
> --- a/server/dcc.c
> +++ b/server/dcc.c
> @@ -194,7 +194,7 @@ static RedSurfaceCreateItem
> *red_surface_create_item_new(RedChannel* channel,
>  {
>  RedSurfaceCreateItem *create;
>  
> -create = spice_new(RedSurfaceCreateItem, 1);
> +create = g_new(RedSurfaceCreateItem, 1);
>  
>  create->surface_create.surface_id = surface_id;
>  create->surface_create.width = width;
> @@ -340,7 +340,7 @@ RedImageItem
> *dcc_add_surface_area_image(DisplayChannelClient *dcc,
>  bpp = SPICE_SURFACE_FMT_DEPTH(surface->context.format) / 8;
>  stride = width * bpp;
>  
> -item = (RedImageItem *)spice_malloc_n_m(height, stride,
> sizeof(RedImageItem));
> +item = (RedImageItem *)g_malloc(height * stride +
> sizeof(RedImageItem));
>  
>  red_pipe_item_init(>base, RED_PIPE_ITEM_TYPE_IMAGE);
>  
> @@ -689,7 +689,7 @@ static RedSurfaceDestroyItem
> *red_surface_destroy_item_new(RedChannel *channel,
>  {
>  RedSurfaceDestroyItem *destroy;
>  
> -destroy = spice_new(RedSurfaceDestroyItem, 1);
> +destroy = g_new(RedSurfaceDestroyItem, 1);
>  destroy->surface_destroy.surface_id = surface_id;
>  red_pipe_item_init(>pipe_item,
> RED_PIPE_ITEM_TYPE_DESTROY_SURFACE);
>  
> @@ -708,7 +708,7 @@ RedPipeItem
> *dcc_gl_scanout_item_new(RedChannelClient *rcc, void *data, int num)
>  return NULL;
>  }
>  
> -item = spice_new(RedGlScanoutUnixItem, 1);
> +item = g_new(RedGlScanoutUnixItem, 1);
>  red_pipe_item_init(>base, RED_PIPE_ITEM_TYPE_GL_SCANOUT);
>  
>  return >base;
> @@ -728,7 +728,7 @@ RedPipeItem
> *dcc_gl_draw_item_new(RedChannelClient *rcc, void *data, int num)
>  }
>  
>  dcc->priv->gl_draw_ongoing = TRUE;
> -item = spice_new(RedGlDrawItem, 1);
> +item = g_new(RedGlDrawItem, 1);
>  item->draw = *draw;
>  red_pipe_item_init(>base, RED_PIPE_ITEM_TYPE_GL_DRAW);
>  
> diff --git a/server/inputs-channel.c b/server/inputs-channel.c
> index b8944cb0f..96fd87399 100644
> --- a/server/inputs-channel.c
> +++ b/server/inputs-channel.c
> @@ -196,7 +196,7 @@ static uint8_t kbd_get_leds(SpiceKbdInstance
> *sin)
>  
>  static RedPipeItem *red_inputs_key_modifiers_item_new(uint8_t
> modifiers)
>  {
> -RedKeyModifiersPipeItem *item =
> spice_malloc(sizeof(RedKeyModifiersPipeItem));
> +RedKeyModifiersPipeItem *item = g_new(RedKeyModifiersPipeItem,
> 1);
>  
>  red_pipe_item_init(>base, RED_PIPE_ITEM_KEY_MODIFIERS);
>  item->modifiers = modifiers;
> @@ -435,7 +435,7 @@ void inputs_release_keys(InputsChannel *inputs)
>  
>  static void inputs_pipe_add_init(RedChannelClient *rcc)
>  {
> -RedInputsInitPipeItem

[Spice-devel] [PATCH spice-server v3 23/23] red-pipe-item: Use GLib memory functions

2017-09-25 Thread Frediano Ziglio 
Signed-off-by: Frediano Ziglio 
Acked-by: Jonathon Jongsma 
---
 server/cache-item.tmpl.c  |  6 +++---
 server/dcc.c  | 10 +-
 server/inputs-channel.c   |  4 ++--
 server/main-channel-client.c  | 14 +++---
 server/red-channel-client.c   |  8 
 server/red-channel.c  |  2 +-
 server/red-pipe-item.c|  2 +-
 server/smartcard-channel-client.c |  2 +-
 server/spicevmc.c | 10 +-
 server/stream.c   |  2 +-
 10 files changed, 30 insertions(+), 30 deletions(-)

Changes since v2:
- moved some changes to spicevmc.c from another patch to this patch.

diff --git a/server/cache-item.tmpl.c b/server/cache-item.tmpl.c
index 19e6b95f1..44b45f812 100644
--- a/server/cache-item.tmpl.c
+++ b/server/cache-item.tmpl.c
@@ -86,7 +86,7 @@ static int FUNC_NAME(add)(CHANNELCLIENT *channel_client, 
uint64_t id, size_t siz
 RedCacheItem *item;
 int key;
 
-item = spice_new(RedCacheItem, 1);
+item = g_new(RedCacheItem, 1);
 
 channel_client->priv->VAR_NAME(available) -= size;
 SPICE_VERIFY(SPICE_OFFSETOF(RedCacheItem, u.cache_data.lru_link) == 0);
@@ -94,7 +94,7 @@ static int FUNC_NAME(add)(CHANNELCLIENT *channel_client, 
uint64_t id, size_t siz
 RedCacheItem *tail = (RedCacheItem 
*)ring_get_tail(_client->priv->VAR_NAME(lru));
 if (!tail) {
 channel_client->priv->VAR_NAME(available) += size;
-free(item);
+g_free(item);
 return FALSE;
 }
 FUNC_NAME(remove)(channel_client, tail);
@@ -117,7 +117,7 @@ static void FUNC_NAME(reset)(CHANNELCLIENT *channel_client, 
long size)
 while (channel_client->priv->CACHE_NAME[i]) {
 RedCacheItem *item = channel_client->priv->CACHE_NAME[i];
 channel_client->priv->CACHE_NAME[i] = item->u.cache_data.next;
-free(item);
+g_free(item);
 }
 }
 ring_init(_client->priv->VAR_NAME(lru));
diff --git a/server/dcc.c b/server/dcc.c
index f3d53051c..90684e17c 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -194,7 +194,7 @@ static RedSurfaceCreateItem 
*red_surface_create_item_new(RedChannel* channel,
 {
 RedSurfaceCreateItem *create;
 
-create = spice_new(RedSurfaceCreateItem, 1);
+create = g_new(RedSurfaceCreateItem, 1);
 
 create->surface_create.surface_id = surface_id;
 create->surface_create.width = width;
@@ -340,7 +340,7 @@ RedImageItem 
*dcc_add_surface_area_image(DisplayChannelClient *dcc,
 bpp = SPICE_SURFACE_FMT_DEPTH(surface->context.format) / 8;
 stride = width * bpp;
 
-item = (RedImageItem *)spice_malloc_n_m(height, stride, 
sizeof(RedImageItem));
+item = (RedImageItem *)g_malloc(height * stride + sizeof(RedImageItem));
 
 red_pipe_item_init(>base, RED_PIPE_ITEM_TYPE_IMAGE);
 
@@ -689,7 +689,7 @@ static RedSurfaceDestroyItem 
*red_surface_destroy_item_new(RedChannel *channel,
 {
 RedSurfaceDestroyItem *destroy;
 
-destroy = spice_new(RedSurfaceDestroyItem, 1);
+destroy = g_new(RedSurfaceDestroyItem, 1);
 destroy->surface_destroy.surface_id = surface_id;
 red_pipe_item_init(>pipe_item, 
RED_PIPE_ITEM_TYPE_DESTROY_SURFACE);
 
@@ -708,7 +708,7 @@ RedPipeItem *dcc_gl_scanout_item_new(RedChannelClient *rcc, 
void *data, int num)
 return NULL;
 }
 
-item = spice_new(RedGlScanoutUnixItem, 1);
+item = g_new(RedGlScanoutUnixItem, 1);
 red_pipe_item_init(>base, RED_PIPE_ITEM_TYPE_GL_SCANOUT);
 
 return >base;
@@ -728,7 +728,7 @@ RedPipeItem *dcc_gl_draw_item_new(RedChannelClient *rcc, 
void *data, int num)
 }
 
 dcc->priv->gl_draw_ongoing = TRUE;
-item = spice_new(RedGlDrawItem, 1);
+item = g_new(RedGlDrawItem, 1);
 item->draw = *draw;
 red_pipe_item_init(>base, RED_PIPE_ITEM_TYPE_GL_DRAW);
 
diff --git a/server/inputs-channel.c b/server/inputs-channel.c
index b8944cb0f..96fd87399 100644
--- a/server/inputs-channel.c
+++ b/server/inputs-channel.c
@@ -196,7 +196,7 @@ static uint8_t kbd_get_leds(SpiceKbdInstance *sin)
 
 static RedPipeItem *red_inputs_key_modifiers_item_new(uint8_t modifiers)
 {
-RedKeyModifiersPipeItem *item = 
spice_malloc(sizeof(RedKeyModifiersPipeItem));
+RedKeyModifiersPipeItem *item = g_new(RedKeyModifiersPipeItem, 1);
 
 red_pipe_item_init(>base, RED_PIPE_ITEM_KEY_MODIFIERS);
 item->modifiers = modifiers;
@@ -435,7 +435,7 @@ void inputs_release_keys(InputsChannel *inputs)
 
 static void inputs_pipe_add_init(RedChannelClient *rcc)
 {
-RedInputsInitPipeItem *item = spice_malloc(sizeof(RedInputsInitPipeItem));
+RedInputsInitPipeItem *item = g_new(RedInputsInitPipeItem, 1);
 InputsChannel *inputs = 
INPUTS_CHANNEL(red_channel_client_get_channel(rcc));
 
 red_pipe_item_init(>base, RED_PIPE_ITEM_INPUTS_INIT);
diff --git a/server/main-channel-client.c b/server/main-channel-client.c
index b7b60eddb..bdcc1e825 100644