[Spice-devel] [PATCH spice-server] gstreamer: Include only needed fields in SpiceFormatForGStreamer structure

[Frediano Ziglio]

This structure is used to store format information for
both Gstreamer 0.10 and 1.0 however the two format uses
different fields from it.
Use a macro to filter only needed fields.
This currently also fixes a compile error using Gstreamer 0.10.

Signed-off-by: Frediano Ziglio 
---
 server/gstreamer-encoder.c | 23 +--
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index 35573bd..cd9e627 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -42,16 +42,27 @@
 
 typedef struct {
 SpiceBitmapFmt spice_format;
+uint32_t bpp;
+#ifndef HAVE_GSTREAMER_0_10
 char format[8];
 GstVideoFormat gst_format;
-uint32_t bpp;
+#else
 uint32_t depth;
 uint32_t endianness;
 uint32_t blue_mask;
 uint32_t green_mask;
 uint32_t red_mask;
+#endif
 } SpiceFormatForGStreamer;
 
+#ifndef HAVE_GSTREAMER_0_10
+#define FMT_DESC(sf, b, f, gf, d, end, bm, gm, rm) \
+{ sf, b, f, gf }
+#else
+#define FMT_DESC(sf, b, f, gf, d, end, bm, gm, rm) \
+{ sf, b, d, end, bm, gm, rm }
+#endif
+
 typedef struct SpiceGstVideoBuffer {
 VideoBuffer base;
 GstBuffer *gst_buffer;
@@ -758,12 +769,12 @@ static const SpiceFormatForGStreamer format_map[] =  {
 /* First item is invalid.
  * It's located first so the loop catch invalid values.
  */
-{SPICE_BITMAP_FMT_INVALID, "", GST_VIDEO_FORMAT_UNKNOWN, 0, 0, 0, 0, 0, 0},
-{SPICE_BITMAP_FMT_RGBA, "BGRA", GST_VIDEO_FORMAT_BGRA, 32, 24, 4321, 
0xff00, 0xff, 0xff00},
-{SPICE_BITMAP_FMT_16BIT, "RGB15", GST_VIDEO_FORMAT_RGB15, 16, 15, 4321, 
0x001f, 0x03E0, 0x7C00},
+FMT_DESC(SPICE_BITMAP_FMT_INVALID, 0, "", GST_VIDEO_FORMAT_UNKNOWN, 0, 0, 
0, 0, 0),
+FMT_DESC(SPICE_BITMAP_FMT_RGBA, 32, "BGRA", GST_VIDEO_FORMAT_BGRA, 24, 
4321, 0xff00, 0xff, 0xff00),
+FMT_DESC(SPICE_BITMAP_FMT_16BIT, 16, "RGB15", GST_VIDEO_FORMAT_RGB15, 15, 
4321, 0x001f, 0x03E0, 0x7C00),
 /* TODO: Test the other formats under GStreamer 0.10*/
-{SPICE_BITMAP_FMT_32BIT, "BGRx", GST_VIDEO_FORMAT_BGRx, 32, 24, 4321, 
0xff00, 0xff, 0xff00},
-{SPICE_BITMAP_FMT_24BIT, "BGR", GST_VIDEO_FORMAT_BGR, 24, 24, 4321, 
0xff, 0xff00, 0xff},
+FMT_DESC(SPICE_BITMAP_FMT_32BIT, 32, "BGRx", GST_VIDEO_FORMAT_BGRx, 24, 
4321, 0xff00, 0xff, 0xff00),
+FMT_DESC(SPICE_BITMAP_FMT_24BIT, 24, "BGR", GST_VIDEO_FORMAT_BGR, 24, 
4321, 0xff, 0xff00, 0xff),
 };
 #define GSTREAMER_FORMAT_INVALID (&format_map[0])
 
-- 
2.9.3

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

Re: [Spice-devel] [PATCH] RFC: Document display channel drawable trees, etc

[Jonathon Jongsma]

On Thu, 2017-01-26 at 04:18 -0500, Frediano Ziglio wrote:
> > 
> > ---
> > I started looking into a bug related to surfaces and drawing, but
> > after a
> > little initial investigation, I realized that I didn't really
> > understand any
> > of
> > the code in this part of spice server. I discussed it a little bit
> > with
> > several
> > others who also didn't have a good understanding of this part of
> > the code. So
> > I
> > decided it needed to be documented so that we can understand it and
> > work on
> > it
> > effectively. I've spent several long days trying to understand the
> > details
> > and
> > trying to document them as well as I can. There is a lot of
> > confusing
> > terminology, and there are things I still don't fully understand,
> > but I
> > wanted
> > to send this out to get feedback from others. Is it helpful? Do you
> > see any
> > cases where I misinterpreted things? Can anybody solve any pieces
> > of the
> > puzzle
> > that I didn't quite figure out? etc. etc.
> > 
> > I know it's rather dense and will probably require others to spend
> > some
> > serious
> > time understanding the code, but I'd love to get some feedback on
> > it.
> > 
> 
> Take some time to find this post, so before get really buried on the
> ML...

Thanks for taking the time to review. Would love to hear from others as
well.

> 
> Some part are quite good and should be split and merged, other,
> particularly exclude_region have too much questions to solve.

Yeah, not a bad idea. Some discussion below.

> 
> >  server/display-channel-private.h |   5 +-
> >  server/display-channel.c | 256
> >  +++
> >  server/display-channel.h |  10 +-
> >  server/tree.c|  12 +-
> >  server/tree.h|   6 +
> >  5 files changed, 285 insertions(+), 4 deletions(-)
> > 
> > diff --git a/server/display-channel-private.h
> > b/server/display-channel-private.h
> > index 6e299b9..dc4d605 100644
> > --- a/server/display-channel-private.h
> > +++ b/server/display-channel-private.h
> > @@ -32,7 +32,10 @@ struct DisplayChannelPrivate
> >  int enable_jpeg;
> >  int enable_zlib_glz_wrap;
> >  
> > -Ring current_list; /* Drawable */
> > +/* A ring of pending drawables for this DisplayChannel,
> > regardless of which
> > + * surface they're associated with. This list is mainly used
> > to flush older
> > + * drawables when we need to make room for new drawables. */
> > +Ring current_list;
> 
> is this list containing ALL drawables allocated ?
> Could be renamed to drawable_list ?

Not exactly, the list of all allocated drawables is
DisplayChannelPrivate::drawables. This is a list of all drawables that
are pending for any surface associated with this DisplayChannel. In
essence, it's roughly equivalent to the union of all
DisplayChannelPrivate::surfaces[i].current_list rings. But the
drawables are ordered by age so that if we want to find the oldest
drawable in the queue (regardless of which surface it's associated
with), we can just get the tail of this ring (see free_one_drawable()).
So as far as I can tell, it's just a convenience ring to avoid
iterating through all of the surfaces to find the oldest drawable.

> 
> Wondering what these "current" are...

As far as I understand, it's basically all of the QXL commands that
have been received from the device, but have not actually been drawn
yet. It's not a great name though...

> 
> >  uint32_t current_size;
> >  
> >  uint32_t drawable_count;
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index 6069883..83e76c1 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -393,6 +393,8 @@ static void current_add_drawable(DisplayChannel
> > *display,
> >  drawable->refs++;
> >  }
> >  
> > +/* Unrefs the drawable and removes it from any rings that it's in,
> > as well as
> > + * removing any associated shadow item */
> >  static void current_remove_drawable(DisplayChannel *display,
> > Drawable *item)
> 
> display_channel_remove_drawable ?
> display_channel_unlink_drawable ?

Definitely could use a better name. Not sure what the best would be. I
would personally like to refactor a lot of this code eventually, but
wanted to document it without any code changes first.

> 
> >  {
> >  /* todo: move all to unref? */
> > @@ -422,6 +424,7 @@ static void drawable_remove_from_pipes(Drawable
> > *drawable)
> >  }
> >  }
> >  
> > +/* This function should never be called for Shadow TreeItems */
> >  static void current_remove(DisplayChannel *display, TreeItem
> > *item)
> >  {
> >  TreeItem *now = item;
> > @@ -439,22 +442,38 @@ static void current_remove(DisplayChannel
> > *display,
> > TreeItem *item)
> >  } else {
> >  Container *now_as_container = CONTAINER(now);
> >  
> > +/* This is a questionable assert. It relies several
> > assumptions
> > + * to ensure that this fun

[Spice-devel] [PATCH spice-jhbuild v2] Update repositories for virt-viewer and virt-manager

[Eduardo Lima (Etrunko)]

fedorahosted.org is shutting down soon, and projects have migrated to
different locations. Virt-viewer moved to pagure.io, while virt-manger
is now hosted on github.

https://www.redhat.com/archives/virt-tools-list/2017-January/msg00070.html

Signed-off-by: Eduardo Lima (Etrunko) 
---
 modulesets/spice.xml | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/modulesets/spice.xml b/modulesets/spice.xml
index 48d3795..f7c25a4 100644
--- a/modulesets/spice.xml
+++ b/modulesets/spice.xml
@@ -16,8 +16,11 @@
   
 
-  
+   https://github.com/virt-manager/"/>
+
+   https://pagure.io/"/>
 
   http://downloads.sourceforge.net/project/"/>
@@ -157,7 +160,7 @@
   
 
   
-
+
 
   
   
@@ -170,7 +173,7 @@
  autogenargs=
 "--enable-werror
 --with-spice-gtk">
-
+
 
   
   
-- 
2.9.3

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

[Spice-devel] [PATCH spice-server] display-channel: Remove current_size field

[Frediano Ziglio]

This field is used only for debugging.
Remove it reducing a bit all these "current" fields around.

Signed-off-by: Frediano Ziglio 
---
 server/display-channel-private.h | 1 -
 server/display-channel.c | 4 +---
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/server/display-channel-private.h b/server/display-channel-private.h
index 38330da..a930cce 100644
--- a/server/display-channel-private.h
+++ b/server/display-channel-private.h
@@ -33,7 +33,6 @@ struct DisplayChannelPrivate
 int enable_zlib_glz_wrap;
 
 Ring current_list; // of TreeItem
-uint32_t current_size;
 
 uint32_t drawable_count;
 _Drawable drawables[NUM_DRAWABLES];
diff --git a/server/display-channel.c b/server/display-channel.c
index 6069883..7d3c6e4 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -389,7 +389,6 @@ static void current_add_drawable(DisplayChannel *display,
 ring_add_after(&drawable->tree_item.base.siblings_link, pos);
 ring_add(&display->priv->current_list, &drawable->list_link);
 ring_add(&surface->current_list, &drawable->surface_list_link);
-display->priv->current_size++;
 drawable->refs++;
 }
 
@@ -402,7 +401,6 @@ static void current_remove_drawable(DisplayChannel 
*display, Drawable *item)
 ring_remove(&item->list_link);
 ring_remove(&item->surface_list_link);
 drawable_unref(item);
-display->priv->current_size--;
 }
 
 static void drawable_remove_from_pipes(Drawable *drawable)
@@ -2269,6 +2267,6 @@ void display_channel_debug_oom(DisplayChannel *display, 
const char *msg)
 msg,
 display->priv->drawable_count,
 display->priv->encoder_shared_data.glz_drawable_count,
-display->priv->current_size,
+ring_get_length(&display->priv->current_list),
 red_channel_sum_pipes_size(channel));
 }
-- 
2.9.3

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

Re: [Spice-devel] [PATCH] Update repository for virt-viewer

[Eduardo Lima (Etrunko)]

On 01/25/2017 01:10 PM, Christophe Fergeau wrote:
> On Wed, Jan 25, 2017 at 12:34:10PM -0200, Eduardo Lima (Etrunko) wrote:
>> Virt-viewer recently moved from fedorahosted.org to pagure.io
>> https://www.redhat.com/archives/virt-tools-list/2017-January/msg00070.html
>>
>> Signed-off-by: Eduardo Lima (Etrunko) 
>> ---
>>  modulesets/spice.xml | 5 -
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/modulesets/spice.xml b/modulesets/spice.xml
>> index 48d3795..9e101c1 100644
>> --- a/modulesets/spice.xml
>> +++ b/modulesets/spice.xml
>> @@ -19,6 +19,9 @@
>>>href="git://git.fedorahosted.org/git/"/>
>>  
>> +   > +  href="https://pagure.io/"/>
>> +
>>>href="http://downloads.sourceforge.net/project/"/>
>>  
>> @@ -170,7 +173,7 @@
>>   autogenargs=
>>  "--enable-werror
>>  --with-spice-gtk">
>> -
>> +
>>  
>>
>>
> 
> 
> Acked-by: Christophe Fergeau 
> 
> From the look of it, virt-manager moved to github, so the 'fedorahosted'
> repository could be removed.

Ah ok, I will fix the virt-manager repository as well.


-- 
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etru...@redhat.com



signature.asc
Description: OpenPGP digital signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH spice-server] Avoid to use global variable for channel IDs

[Frediano Ziglio]

This patch allocate VMC IDs finding the first ID not used
instead of using a global variable and incrementing the value
for each channel created.
This solve some potential issues:
- remove the global state potentially making possible
  to use multiple SpiceServer on the same process;
- avoid to potentially overflow the variable. This can happen
  if channels are allocated/deallocated multiple times
  (currently not done by Qemu).

Signed-off-by: Frediano Ziglio 
---
 server/reds.c | 35 +++
 server/reds.h |  1 +
 server/spicevmc.c | 10 --
 3 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/server/reds.c b/server/reds.c
index 29485a8..44c8f4a 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -403,6 +403,41 @@ RedChannel *reds_find_channel(RedsState *reds, uint32_t 
type, uint32_t id)
 return NULL;
 }
 
+/* Search for first free channel id for a specific channel type.
+ * Return first id free or <0 if not found. */
+int reds_get_free_channel_id(RedsState *reds, uint32_t type)
+{
+GListIter it;
+RedChannel *channel;
+
+// this mark if some IDs are used.
+// Actually used to store boolean values but char take less memory
+// then gboolean/bool.
+// The size of the array limit the possible id returned but
+// usually the IDs used for a channel type are not much.
+char used_ids[256];
+
+unsigned n;
+
+// mark id used for the specific channel type
+memset(used_ids, 0, sizeof(used_ids));
+GLIST_FOREACH(reds->channels, it, RedChannel, channel) {
+uint32_t this_type, this_id;
+g_object_get(channel, "channel-type", &this_type, "id", &this_id, 
NULL);
+if (this_type == type && this_id < SPICE_N_ELEMENTS(used_ids)) {
+used_ids[this_id] = TRUE;
+}
+}
+
+// find first ID not marked as used
+for (n = 0; n < SPICE_N_ELEMENTS(used_ids); ++n) {
+if (!used_ids[n]) {
+return n;
+}
+}
+return -1;
+}
+
 static void reds_mig_cleanup(RedsState *reds)
 {
 if (reds->mig_inprogress) {
diff --git a/server/reds.h b/server/reds.h
index 28e3444..14d9071 100644
--- a/server/reds.h
+++ b/server/reds.h
@@ -48,6 +48,7 @@ uint32_t reds_get_mm_time(void);
 void reds_register_channel(RedsState *reds, RedChannel *channel);
 void reds_unregister_channel(RedsState *reds, RedChannel *channel);
 RedChannel *reds_find_channel(RedsState *reds, uint32_t type, uint32_t id);
+int reds_get_free_channel_id(RedsState *reds, uint32_t type);
 int reds_get_mouse_mode(RedsState *reds); // used by inputs_channel
 gboolean reds_config_get_agent_mouse(const RedsState *reds); // used by 
inputs_channel
 int reds_has_vdagent(RedsState *reds); // used by inputs channel
diff --git a/server/spicevmc.c b/server/spicevmc.c
index 89249b2..c184bca 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -224,7 +224,6 @@ red_vmc_channel_finalize(GObject *object)
 static RedVmcChannel *red_vmc_channel_new(RedsState *reds, uint8_t 
channel_type)
 {
 GType gtype = G_TYPE_NONE;
-static uint8_t id[SPICE_END_CHANNEL] = { 0, };
 
 switch (channel_type) {
 case SPICE_CHANNEL_USBREDIR:
@@ -240,11 +239,18 @@ static RedVmcChannel *red_vmc_channel_new(RedsState 
*reds, uint8_t channel_type)
 g_error("Unsupported channel_type for red_vmc_channel_new(): %u", 
channel_type);
 return NULL;
 }
+
+int id = reds_get_free_channel_id(reds, channel_type);
+if (id < 0) {
+g_warning("Free ID not found creating new VMC channel");
+return NULL;
+}
+
 return g_object_new(gtype,
 "spice-server", reds,
 "core-interface", reds_get_core_interface(reds),
 "channel-type", channel_type,
-"id", id[channel_type]++,
+"id", id,
 "handle-acks", FALSE,
 "migration-flags",
 (SPICE_MIGRATE_NEED_FLUSH | 
SPICE_MIGRATE_NEED_DATA_TRANSFER),
-- 
2.9.3

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

Re: [Spice-devel] [spice-server v3 10/10] sound: Convert SndChannelClient to RedChannelClient

[Jonathon Jongsma]

On Thu, 2017-01-26 at 07:39 -0500, Frediano Ziglio wrote:
> > 
> > SndChannelClient is now inheriting from GObject, and has switched
> > from using its own code for sending data to using RedChannelClient.
> > This commit makes it directly inherit from RedChannelClient rather
> > than
> > having a channel_client field. This allows to get rid of the whole
> > DummyChannel/DummyChannelClient code.
> > 
> > Based on a patch from Frediano Ziglio 
> > 
> > Signed-off-by: Christophe Fergeau 
> > ---

[snip]

> 
> Still think this should be merged to 9/10.
> 
> Frediano
> 

Personally I kind of like them as separate commits. But I don't feel
too strongly either way.

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

Re: [Spice-devel] [PATCH spice-server] Support VP9 encoder using GStreamer

[Frediano Ziglio]

ping

> 
> Signed-off-by: Frediano Ziglio 
> ---
>  configure.ac   | 2 +-
>  server/gstreamer-encoder.c | 4 
>  server/reds.c  | 4 +++-
>  3 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index a78b4ec..c155c35 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -142,7 +142,7 @@ AS_IF([test x"$have_smartcard" = "xyes"], [
>  AS_VAR_APPEND([SPICE_REQUIRES], [" libcacard >= 0.1.2"])
>  ])
>  
> -SPICE_PROTOCOL_MIN_VER=0.12.12
> +SPICE_PROTOCOL_MIN_VER=0.12.13
>  PKG_CHECK_MODULES([SPICE_PROTOCOL], [spice-protocol >=
>  $SPICE_PROTOCOL_MIN_VER])
>  AC_SUBST([SPICE_PROTOCOL_MIN_VER])
>  
> diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> index 988d193..cb0af49 100644
> --- a/server/gstreamer-encoder.c
> +++ b/server/gstreamer-encoder.c
> @@ -878,6 +878,8 @@ static const gchar* get_gst_codec_name(SpiceGstEncoder
> *encoder)
>  return "vp8enc";
>  case SPICE_VIDEO_CODEC_TYPE_H264:
>  return "x264enc";
> +case SPICE_VIDEO_CODEC_TYPE_VP9:
> +return "vp9enc";
>  default:
>  /* gstreamer_encoder_new() should have rejected this codec type */
>  spice_warning("unsupported codec type %d",
>  encoder->base.codec_type);
> @@ -907,6 +909,7 @@ static gboolean create_pipeline(SpiceGstEncoder *encoder)
>  gstenc_opts = g_strdup("max-threads=1");
>  #endif
>  break;
> +case SPICE_VIDEO_CODEC_TYPE_VP9:
>  case SPICE_VIDEO_CODEC_TYPE_VP8: {
>  /* See http://www.webmproject.org/docs/encoder-parameters/
>   * - Set mode/end-usage to get a constant bitrate to help with
> @@ -1694,6 +1697,7 @@ VideoEncoder *gstreamer_encoder_new(SpiceVideoCodecType
> codec_type,
>  SPICE_VERIFY(SPICE_GST_FRAME_STATISTICS_COUNT <=
>  SPICE_GST_HISTORY_SIZE);
>  spice_return_val_if_fail(codec_type == SPICE_VIDEO_CODEC_TYPE_MJPEG ||
>   codec_type == SPICE_VIDEO_CODEC_TYPE_VP8 ||
> + codec_type == SPICE_VIDEO_CODEC_TYPE_VP9 ||
>   codec_type == SPICE_VIDEO_CODEC_TYPE_H264,
>   NULL);
>  
>  GError *err = NULL;
> diff --git a/server/reds.c b/server/reds.c
> index 3b30928..e831ab7 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -3405,7 +3405,7 @@ err:
>  
>  static const char default_renderer[] = "sw";
>  #if defined(HAVE_GSTREAMER_1_0) || defined(HAVE_GSTREAMER_0_10)
> -#define GSTREAMER_CODECS "gstreamer:mjpeg;gstreamer:h264;gstreamer:vp8;"
> +#define GSTREAMER_CODECS
> "gstreamer:mjpeg;gstreamer:h264;gstreamer:vp8;gstreamer:vp9;"
>  #else
>  #define GSTREAMER_CODECS ""
>  #endif
> @@ -3501,6 +3501,7 @@ static const EnumNames video_codec_names[] = {
>  {SPICE_VIDEO_CODEC_TYPE_MJPEG, "mjpeg"},
>  {SPICE_VIDEO_CODEC_TYPE_VP8, "vp8"},
>  {SPICE_VIDEO_CODEC_TYPE_H264, "h264"},
> +{SPICE_VIDEO_CODEC_TYPE_VP9, "vp9"},
>  {0, NULL},
>  };
>  
> @@ -3508,6 +3509,7 @@ static const int video_codec_caps[] = {
>  SPICE_DISPLAY_CAP_CODEC_MJPEG,
>  SPICE_DISPLAY_CAP_CODEC_VP8,
>  SPICE_DISPLAY_CAP_CODEC_H264,
> +SPICE_DISPLAY_CAP_CODEC_VP9,
>  };
>  
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-protocol] Add support for VP9 video codec

[Frediano Ziglio]

ping

> 
> On Tue, Jan 03, 2017 at 06:13:55PM +0100, Victor Toso wrote:
> > Hi,
> > 
> > On Tue, Jan 03, 2017 at 01:42:33PM +, Frediano Ziglio wrote:
> > > Signed-off-by: Frediano Ziglio 
> > > ---
> > >  spice/enums.h| 1 +
> > >  spice/protocol.h | 1 +
> > >  2 files changed, 2 insertions(+)
> > >
> > > diff --git a/spice/enums.h b/spice/enums.h
> > > index 743a4a8..86d02d7 100644
> > > --- a/spice/enums.h
> > > +++ b/spice/enums.h
> > > @@ -148,6 +148,7 @@ typedef enum SpiceVideoCodecType {
> > >  SPICE_VIDEO_CODEC_TYPE_MJPEG = 1,
> > >  SPICE_VIDEO_CODEC_TYPE_VP8,
> > >  SPICE_VIDEO_CODEC_TYPE_H264,
> > > +SPICE_VIDEO_CODEC_TYPE_VP9,
> > >
> > 
> > I don't see a reason to not have it.
> > 
> > In spice-gtk we will need to update gst_opts[] array to include the
> > protocol + vp9dec bits. (channel-display-gst.c)
> > 
> > In server, although not mandatory, I would include the gstreamer:vp9 in
> > default_renderer string (reds.c).
> 
> Yes, I would wait until we have server/client patches using this before
> getting it in.
> 
> Christophe
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk] Add support for VP9

[Frediano Ziglio]

ping

> 
> Signed-off-by: Frediano Ziglio 
> ---
>  configure.ac  | 2 +-
>  src/channel-display-gst.c | 2 ++
>  src/channel-display.c | 6 ++
>  3 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/configure.ac b/configure.ac
> index f3e7f8d..aa60e91 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -69,7 +69,7 @@ AC_CHECK_LIBM
>  AC_SUBST(LIBM)
>  
>  AC_CONFIG_SUBDIRS([spice-common])
> -PKG_CHECK_MODULES([SPICE_PROTOCOL], [spice-protocol >= 0.12.12])
> +PKG_CHECK_MODULES([SPICE_PROTOCOL], [spice-protocol >= 0.12.13])
>  
>  COMMON_CFLAGS='-I${top_builddir}/spice-common/ -I${top_srcdir}/spice-common/
>  ${SPICE_PROTOCOL_CFLAGS}'
>  AC_SUBST(COMMON_CFLAGS)
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index f52299f..6ace86f 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -77,6 +77,8 @@ static struct {
>   */
>  { "h264parse ! avdec_h264", "" },
>  
> +/* SPICE_VIDEO_CODEC_TYPE_VP9 */
> +{ "vp9dec", "caps=video/x-vp9" },
>  };
>  
>  G_STATIC_ASSERT(G_N_ELEMENTS(gst_opts) == SPICE_VIDEO_CODEC_TYPE_ENUM_END);
> diff --git a/src/channel-display.c b/src/channel-display.c
> index 709b3d2..7506863 100644
> --- a/src/channel-display.c
> +++ b/src/channel-display.c
> @@ -736,6 +736,12 @@ static void
> spice_display_channel_reset_capabilities(SpiceChannel *channel)
>  } else {
>  SPICE_DEBUG("GStreamer does not support the h264 codec");
>  }
> +if (gstvideo_has_codec(SPICE_VIDEO_CODEC_TYPE_VP9)) {
> +spice_channel_set_capability(SPICE_CHANNEL(channel),
> + SPICE_DISPLAY_CAP_CODEC_VP9);
> +} else {
> +SPICE_DEBUG("GStreamer does not support the vp9 codec");
> +}
>  }
>  
>  static void destroy_surface(gpointer data)
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server] reds: Check link header magic without waiting all header

[Frediano Ziglio]

> 
> On 01/26/2017 06:20 PM, Uri Lublin wrote:
> > Hi Frediano,
> >
> > I'd replace "all" in subject with "for the whole"
> >
> > On 01/26/2017 05:54 PM, Frediano Ziglio wrote:
> >> This allows the connection to early fail in case initial bytes
> >> are not correct.
> >> This allows for instance VNC client to graceful fail connecting
> >> to a spice-server. This happens easily as the two protocols
> >> share the same range of ports.
> >>
> >> Signed-off-by: Frediano Ziglio 
> >> Tested-by: Daniel P. Berrange 
> >
> 
> I wrote the following python script to test it, till vnc
> client is ready:
> (And added some debug messages in spice-server)
> 
> ---
> import socket
> s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
> s.connect(('localhost', 5924))
> s.send("RFB ")
> magic = s.recv(4)
> print 'magic is ', magic
> ---
> 
> Uri.

Seems to work. I mean, the program get not stuck which
was the intention of the patch.

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

Re: [Spice-devel] [PATCH spice-server] reds: Check link header magic without waiting all header

[Uri Lublin]

On 01/26/2017 06:20 PM, Uri Lublin wrote:

Hi Frediano,

I'd replace "all" in subject with "for the whole"

On 01/26/2017 05:54 PM, Frediano Ziglio wrote:

This allows the connection to early fail in case initial bytes
are not correct.
This allows for instance VNC client to graceful fail connecting
to a spice-server. This happens easily as the two protocols
share the same range of ports.

Signed-off-by: Frediano Ziglio 
Tested-by: Daniel P. Berrange 




I wrote the following python script to test it, till vnc
client is ready:
(And added some debug messages in spice-server)

---
import socket
s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
s.connect(('localhost', 5924))
s.send("RFB ")
magic = s.recv(4)
print 'magic is ', magic
---

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

[Spice-devel] [PATCH spice-server v2] reds: Check link header magic without waiting for the whole header

[Frediano Ziglio]

This allows the connection to early fail in case initial bytes
are not correct.
This allows for instance VNC client to graceful fail connecting
to a spice-server. This happens easily as the two protocols
share the same range of ports.

This resolves rhbz#1416692.

Signed-off-by: Frediano Ziglio 
Tested-by: Daniel P. Berrange 
Acked-by: Uri Lublin 
---
 server/reds.c | 28 
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/server/reds.c b/server/reds.c
index 29485a8..40c9485 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -2260,12 +2260,6 @@ static void reds_handle_read_header_done(void *opaque)
 header->minor_version = GUINT32_FROM_LE(header->minor_version);
 header->size = GUINT32_FROM_LE(header->size);
 
-if (header->magic != SPICE_MAGIC) {
-reds_send_link_error(link, SPICE_LINK_ERR_INVALID_MAGIC);
-reds_link_free(link);
-return;
-}
-
 if (header->major_version != SPICE_VERSION_MAJOR) {
 if (header->major_version > 0) {
 reds_send_link_error(link, SPICE_LINK_ERR_VERSION_MISMATCH);
@@ -2292,13 +2286,31 @@ static void reds_handle_read_header_done(void *opaque)
link);
 }
 
+static void reds_handle_read_magic_done(void *opaque)
+{
+RedLinkInfo *link = (RedLinkInfo *)opaque;
+const SpiceLinkHeader *header = &link->link_header;
+
+if (header->magic != SPICE_MAGIC) {
+reds_send_link_error(link, SPICE_LINK_ERR_INVALID_MAGIC);
+reds_link_free(link);
+return;
+}
+
+reds_stream_async_read(link->stream,
+   ((uint8_t *)&link->link_header) + 
sizeof(header->magic),
+   sizeof(SpiceLinkHeader) - sizeof(header->magic),
+   reds_handle_read_header_done,
+   link);
+}
+
 static void reds_handle_new_link(RedLinkInfo *link)
 {
 reds_stream_set_async_error_handler(link->stream, reds_handle_link_error);
 reds_stream_async_read(link->stream,
(uint8_t *)&link->link_header,
-   sizeof(SpiceLinkHeader),
-   reds_handle_read_header_done,
+   sizeof(link->link_header.magic),
+   reds_handle_read_magic_done,
link);
 }
 
-- 
2.9.3

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

Re: [Spice-devel] [PATCH spice-server] reds: Check link header magic without waiting all header

[Uri Lublin]

Hi Frediano,

I'd replace "all" in subject with "for the whole"

On 01/26/2017 05:54 PM, Frediano Ziglio wrote:

This allows the connection to early fail in case initial bytes
are not correct.
This allows for instance VNC client to graceful fail connecting
to a spice-server. This happens easily as the two protocols
share the same range of ports.

Signed-off-by: Frediano Ziglio 
Tested-by: Daniel P. Berrange 


Ack.



---

This is well described in
https://lists.freedesktop.org/archives/spice-devel/2017-January/035201.html
I'm not sure the comment I wrote is enough or I should copy some
explanation from the mail.


I think it's enough to add a reference to rhbz#1416692

Uri.



It add an extra read handling but I don't think it's really
a performance issue as happening only on initial connection.
---
 server/reds.c | 28 
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/server/reds.c b/server/reds.c
index 29485a8..40c9485 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -2260,12 +2260,6 @@ static void reds_handle_read_header_done(void *opaque)
 header->minor_version = GUINT32_FROM_LE(header->minor_version);
 header->size = GUINT32_FROM_LE(header->size);

-if (header->magic != SPICE_MAGIC) {
-reds_send_link_error(link, SPICE_LINK_ERR_INVALID_MAGIC);
-reds_link_free(link);
-return;
-}
-
 if (header->major_version != SPICE_VERSION_MAJOR) {
 if (header->major_version > 0) {
 reds_send_link_error(link, SPICE_LINK_ERR_VERSION_MISMATCH);
@@ -2292,13 +2286,31 @@ static void reds_handle_read_header_done(void *opaque)
link);
 }

+static void reds_handle_read_magic_done(void *opaque)
+{
+RedLinkInfo *link = (RedLinkInfo *)opaque;
+const SpiceLinkHeader *header = &link->link_header;
+
+if (header->magic != SPICE_MAGIC) {
+reds_send_link_error(link, SPICE_LINK_ERR_INVALID_MAGIC);
+reds_link_free(link);
+return;
+}
+
+reds_stream_async_read(link->stream,
+   ((uint8_t *)&link->link_header) + 
sizeof(header->magic),
+   sizeof(SpiceLinkHeader) - sizeof(header->magic),
+   reds_handle_read_header_done,
+   link);
+}
+
 static void reds_handle_new_link(RedLinkInfo *link)
 {
 reds_stream_set_async_error_handler(link->stream, reds_handle_link_error);
 reds_stream_async_read(link->stream,
(uint8_t *)&link->link_header,
-   sizeof(SpiceLinkHeader),
-   reds_handle_read_header_done,
+   sizeof(link->link_header.magic),
+   reds_handle_read_magic_done,
link);
 }




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

[Spice-devel] [PATCH spice-server] reds-stream: Simplify error logic

[Frediano Ziglio]

Handling read returning 0 (usually end of connection/pipe)
is the same of handling an error (read result -1) with errno == 0
so merge the two paths to reuse code and simplify.

Signed-off-by: Frediano Ziglio 
---

Patch is much smaller without space changes.
---
 server/reds-stream.c | 31 ---
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/server/reds-stream.c b/server/reds-stream.c
index 2730549..d0dadb9 100644
--- a/server/reds-stream.c
+++ b/server/reds-stream.c
@@ -499,28 +499,21 @@ static void async_read_handler(G_GNUC_UNUSED int fd,
 spice_assert(n > 0);
 n = reds_stream_read(stream, async->now, n);
 if (n <= 0) {
-if (n < 0) {
-switch (errno) {
-case EAGAIN:
-if (!stream->watch) {
-stream->watch = reds_core_watch_add(reds, 
stream->socket,
-
SPICE_WATCH_EVENT_READ,
-
async_read_handler, async);
-}
-return;
-case EINTR:
-break;
-default:
-async_read_clear_handlers(async);
-if (async->error) {
-async->error(async->opaque, errno);
-}
-return;
+int err = n < 0 ? errno: 0;
+switch (err) {
+case EAGAIN:
+if (!stream->watch) {
+stream->watch = reds_core_watch_add(reds, stream->socket,
+SPICE_WATCH_EVENT_READ,
+async_read_handler, 
async);
 }
-} else {
+return;
+case EINTR:
+break;
+default:
 async_read_clear_handlers(async);
 if (async->error) {
-async->error(async->opaque, 0);
+async->error(async->opaque, err);
 }
 return;
 }
-- 
2.9.3

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

[Spice-devel] [PATCH spice-server] reds: Check link header magic without waiting all header

[Frediano Ziglio]

This allows the connection to early fail in case initial bytes
are not correct.
This allows for instance VNC client to graceful fail connecting
to a spice-server. This happens easily as the two protocols
share the same range of ports.

Signed-off-by: Frediano Ziglio 
Tested-by: Daniel P. Berrange 

---

This is well described in
https://lists.freedesktop.org/archives/spice-devel/2017-January/035201.html
I'm not sure the comment I wrote is enough or I should copy some
explanation from the mail.

It add an extra read handling but I don't think it's really
a performance issue as happening only on initial connection.
---
 server/reds.c | 28 
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/server/reds.c b/server/reds.c
index 29485a8..40c9485 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -2260,12 +2260,6 @@ static void reds_handle_read_header_done(void *opaque)
 header->minor_version = GUINT32_FROM_LE(header->minor_version);
 header->size = GUINT32_FROM_LE(header->size);
 
-if (header->magic != SPICE_MAGIC) {
-reds_send_link_error(link, SPICE_LINK_ERR_INVALID_MAGIC);
-reds_link_free(link);
-return;
-}
-
 if (header->major_version != SPICE_VERSION_MAJOR) {
 if (header->major_version > 0) {
 reds_send_link_error(link, SPICE_LINK_ERR_VERSION_MISMATCH);
@@ -2292,13 +2286,31 @@ static void reds_handle_read_header_done(void *opaque)
link);
 }
 
+static void reds_handle_read_magic_done(void *opaque)
+{
+RedLinkInfo *link = (RedLinkInfo *)opaque;
+const SpiceLinkHeader *header = &link->link_header;
+
+if (header->magic != SPICE_MAGIC) {
+reds_send_link_error(link, SPICE_LINK_ERR_INVALID_MAGIC);
+reds_link_free(link);
+return;
+}
+
+reds_stream_async_read(link->stream,
+   ((uint8_t *)&link->link_header) + 
sizeof(header->magic),
+   sizeof(SpiceLinkHeader) - sizeof(header->magic),
+   reds_handle_read_header_done,
+   link);
+}
+
 static void reds_handle_new_link(RedLinkInfo *link)
 {
 reds_stream_set_async_error_handler(link->stream, reds_handle_link_error);
 reds_stream_async_read(link->stream,
(uint8_t *)&link->link_header,
-   sizeof(SpiceLinkHeader),
-   reds_handle_read_header_done,
+   sizeof(link->link_header.magic),
+   reds_handle_read_magic_done,
link);
 }
 
-- 
2.9.3

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

Re: [Spice-devel] [PATCH spice-server] spicevmc: Check correctly rcc argument

[Frediano Ziglio]

> 
> Do not check rcc after being use but before.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/spicevmc.c | 7 +++
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/server/spicevmc.c b/server/spicevmc.c
> index 064910e..df776f4 100644
> --- a/server/spicevmc.c
> +++ b/server/spicevmc.c
> @@ -450,12 +450,11 @@ static void
> spicevmc_red_channel_client_on_disconnect(RedChannelClient *rcc)
>  {
>  RedVmcChannel *channel;
>  SpiceCharDeviceInterface *sif;
> -RedClient *client = red_channel_client_get_client(rcc);
> +RedClient *client;
>  
> -if (!rcc) {
> -return;
> -}
> +g_return_if_fail(rcc != NULL);
>  
> +client = red_channel_client_get_client(rcc);
>  channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
>  
>  /* partial message which wasn't pushed to device */

I prefer the version removing the NULL check.

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

[Spice-devel] [PATCH spice-server] spicevmc: Check correctly rcc argument

[Frediano Ziglio]

Do not check rcc after being use but before.

Signed-off-by: Frediano Ziglio 
---
 server/spicevmc.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/server/spicevmc.c b/server/spicevmc.c
index 064910e..df776f4 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -450,12 +450,11 @@ static void 
spicevmc_red_channel_client_on_disconnect(RedChannelClient *rcc)
 {
 RedVmcChannel *channel;
 SpiceCharDeviceInterface *sif;
-RedClient *client = red_channel_client_get_client(rcc);
+RedClient *client;
 
-if (!rcc) {
-return;
-}
+g_return_if_fail(rcc != NULL);
 
+client = red_channel_client_get_client(rcc);
 channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
 
 /* partial message which wasn't pushed to device */
-- 
2.9.3

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

[Spice-devel] [PATCH spice-server] spicevmc: Avoid computing some variable value if not necessary

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/spicevmc.c | 17 -
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/server/spicevmc.c b/server/spicevmc.c
index 89249b2..9bcbada 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -592,13 +592,12 @@ static uint8_t 
*spicevmc_red_channel_alloc_msg_rcv_buf(RedChannelClient *rcc,
uint16_t type,
uint32_t size)
 {
-RedVmcChannel *channel;
-RedClient *client = red_channel_client_get_client(rcc);
-
-channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
 
 switch (type) {
-case SPICE_MSGC_SPICEVMC_DATA:
+case SPICE_MSGC_SPICEVMC_DATA: {
+RedClient *client = red_channel_client_get_client(rcc);
+RedVmcChannel *channel = 
RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
+
 assert(!channel->recv_from_client_buf);
 
 channel->recv_from_client_buf = 
red_char_device_write_buffer_get(channel->chardev,
@@ -609,6 +608,7 @@ static uint8_t 
*spicevmc_red_channel_alloc_msg_rcv_buf(RedChannelClient *rcc,
 return NULL;
 }
 return channel->recv_from_client_buf->buf;
+}
 
 default:
 return spice_malloc(size);
@@ -621,15 +621,14 @@ static void 
spicevmc_red_channel_release_msg_rcv_buf(RedChannelClient *rcc,
  uint32_t size,
  uint8_t *msg)
 {
-RedVmcChannel *channel;
-
-channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
 
 switch (type) {
-case SPICE_MSGC_SPICEVMC_DATA:
+case SPICE_MSGC_SPICEVMC_DATA: {
+RedVmcChannel *channel = 
RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
 /* buffer wasn't pushed to device */
 red_char_device_write_buffer_release(channel->chardev, 
&channel->recv_from_client_buf);
 break;
+}
 default:
 free(msg);
 }
-- 
2.9.3

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

Re: [Spice-devel] [PATCH spice-server 3/6] spicevmc: Avoid computing some variable value if not necessary

[Frediano Ziglio]

> 
> On Thu, Jan 26, 2017 at 09:37:56AM -0500, Frediano Ziglio wrote:
> > > 
> > > On Thu, Jan 26, 2017 at 10:56:45AM +, Frediano Ziglio wrote:
> > > > Signed-off-by: Frediano Ziglio 
> > > > ---
> > > >  server/spicevmc.c | 10 +-
> > > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/server/spicevmc.c b/server/spicevmc.c
> > > > index a92ec7f..23e60b3 100644
> > > > --- a/server/spicevmc.c
> > > > +++ b/server/spicevmc.c
> > > > @@ -585,12 +585,13 @@ static uint8_t
> > > > *spicevmc_red_channel_alloc_msg_rcv_buf(RedChannelClient *rcc,
> > > > uint32_t size)
> > > >  {
> > > >  RedVmcChannel *channel;
> > > > -RedClient *client = red_channel_client_get_client(rcc);
> > > > -
> > > > -channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
> > > > +RedClient *client;
> > > >  
> > > >  switch (type) {
> > > >  case SPICE_MSGC_SPICEVMC_DATA:
> > > > +client = red_channel_client_get_client(rcc);
> > > > +channel =
> > > > RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
> > > > +
> > > >  assert(!channel->recv_from_client_buf);
> > > >  
> > > >  channel->recv_from_client_buf =
> > > >  red_char_device_write_buffer_get(channel->chardev,
> > > > @@ -615,10 +616,9 @@ static void
> > > > spicevmc_red_channel_release_msg_rcv_buf(RedChannelClient *rcc,
> > > >  {
> > > >  RedVmcChannel *channel;
> > > >  
> > > > -channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
> > > > -
> > > >  switch (type) {
> > > >  case SPICE_MSGC_SPICEVMC_DATA:
> > > > +channel =
> > > > RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
> > > >  /* buffer wasn't pushed to device */
> > > >  red_char_device_write_buffer_release(channel->chardev,
> > > >  &channel->recv_from_client_buf);
> > > >  break;
> > > 
> > > This is something that I'd just let the compiler handle if needed. Note
> > > that this also change some variables from being always initialized to
> > > sometimes being initialized, which is in my opinion is not necessarily a
> > > good move.
> > > 
> > > Christophe
> > > 
> > 
> > In this case the compiler cannot do it. How can it know that not calling
> > a function does not make any difference?
> 
> So red_channel_client_get_channel() should be annotated with
> G_GNUC_PURE so that the compiler knows about this and can optimize
> everywhere by itself?

Not enough and potentially false.

> Thinking more about this patch, even though I hate the {} you have to
> add, I'd be ok with
> 
> spicevmc_red_channel_release_msg_rcv_buf(RedChannelClient *rcc,
> {
> switch (type) {
> case SPICE_MSGC_SPICEVMC_DATA: {
>   RedVmcChannel *channel;
> 
>   channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
>   /* buffer wasn't pushed to device */
>   red_char_device_write_buffer_release(channel->chardev,
>   &channel->recv_from_client_buf);
>   break;
> }
> 
> which improves the reading of the code in my opinion (you know 'channel'
> is not going to be used anywhere else).
> 
> Christophe
> 

I was thinking the same, even better using some more C99 style like

switch (type) {
case SPICE_MSGC_SPICEVMC_DATA: {
  RedVmcChannel *channel = 
RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
  /* buffer wasn't pushed to device */
  red_char_device_write_buffer_release(channel->chardev,
  &channel->recv_from_client_buf);
  break;
}

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

Re: [Spice-devel] [PATCH spice-server 3/6] spicevmc: Avoid computing some variable value if not necessary

[Christophe Fergeau]

On Thu, Jan 26, 2017 at 09:37:56AM -0500, Frediano Ziglio wrote:
> > 
> > On Thu, Jan 26, 2017 at 10:56:45AM +, Frediano Ziglio wrote:
> > > Signed-off-by: Frediano Ziglio 
> > > ---
> > >  server/spicevmc.c | 10 +-
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/server/spicevmc.c b/server/spicevmc.c
> > > index a92ec7f..23e60b3 100644
> > > --- a/server/spicevmc.c
> > > +++ b/server/spicevmc.c
> > > @@ -585,12 +585,13 @@ static uint8_t
> > > *spicevmc_red_channel_alloc_msg_rcv_buf(RedChannelClient *rcc,
> > > uint32_t size)
> > >  {
> > >  RedVmcChannel *channel;
> > > -RedClient *client = red_channel_client_get_client(rcc);
> > > -
> > > -channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
> > > +RedClient *client;
> > >  
> > >  switch (type) {
> > >  case SPICE_MSGC_SPICEVMC_DATA:
> > > +client = red_channel_client_get_client(rcc);
> > > +channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
> > > +
> > >  assert(!channel->recv_from_client_buf);
> > >  
> > >  channel->recv_from_client_buf =
> > >  red_char_device_write_buffer_get(channel->chardev,
> > > @@ -615,10 +616,9 @@ static void
> > > spicevmc_red_channel_release_msg_rcv_buf(RedChannelClient *rcc,
> > >  {
> > >  RedVmcChannel *channel;
> > >  
> > > -channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
> > > -
> > >  switch (type) {
> > >  case SPICE_MSGC_SPICEVMC_DATA:
> > > +channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
> > >  /* buffer wasn't pushed to device */
> > >  red_char_device_write_buffer_release(channel->chardev,
> > >  &channel->recv_from_client_buf);
> > >  break;
> > 
> > This is something that I'd just let the compiler handle if needed. Note
> > that this also change some variables from being always initialized to
> > sometimes being initialized, which is in my opinion is not necessarily a
> > good move.
> > 
> > Christophe
> > 
> 
> In this case the compiler cannot do it. How can it know that not calling
> a function does not make any difference?

So red_channel_client_get_channel() should be annotated with
G_GNUC_PURE so that the compiler knows about this and can optimize
everywhere by itself?
Thinking more about this patch, even though I hate the {} you have to
add, I'd be ok with

spicevmc_red_channel_release_msg_rcv_buf(RedChannelClient *rcc,
{
switch (type) {
case SPICE_MSGC_SPICEVMC_DATA: {
  RedVmcChannel *channel;

  channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
  /* buffer wasn't pushed to device */
  red_char_device_write_buffer_release(channel->chardev,
  &channel->recv_from_client_buf);
  break;
}

which improves the reading of the code in my opinion (you know 'channel'
is not going to be used anywhere else).

Christophe


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

Re: [Spice-devel] [PATCH spice-server 1/6] spicevmc: Use spice_new instead of spice_malloc

[Frediano Ziglio]

> 
> On 01/26/2017 12:56 PM, Frediano Ziglio wrote:
> > spice_new is a bit more safe as return a properly typed pointer.
> 
> Hi Frediano,
> 
> I do not think it's much safer, as spice_new is just

As I said it's a bit safer, not much :-)

> a #define macro that (with second param 1)
> calls spice_malloc and casts from void* ; but

Yes, this is why is safer, returning a typeX * make sure
you can't do something like

typeY *p = spice_new0(typeX, 1);

which give conversion error.

> it does not hurt either, so fine by me.
> 
> Uri.
> 
> >
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  server/spicevmc.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/server/spicevmc.c b/server/spicevmc.c
> > index bbe72b5..0dc2b19 100644
> > --- a/server/spicevmc.c
> > +++ b/server/spicevmc.c
> > @@ -380,7 +380,7 @@ 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_malloc(sizeof(RedPortInitPipeItem));
> > +RedPortInitPipeItem *item = spice_new(RedPortInitPipeItem, 1);
> >
> >  red_pipe_item_init(&item->base, RED_PIPE_ITEM_TYPE_PORT_INIT);
> >  item->name = strdup(sin->portname);
> > @@ -390,7 +390,7 @@ static void spicevmc_port_send_init(RedChannelClient
> > *rcc)
> >
> >  static void spicevmc_port_send_event(RedChannelClient *rcc, uint8_t event)
> >  {
> > -RedPortEventPipeItem *item =
> > spice_malloc(sizeof(RedPortEventPipeItem));
> > +RedPortEventPipeItem *item = spice_new(RedPortEventPipeItem, 1);
> >
> >  red_pipe_item_init(&item->base, RED_PIPE_ITEM_TYPE_PORT_EVENT);
> >  item->event = event;
> >
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [nsis] Fix driver path for win2k*

[Yedidyah Bar David]

On Thu, Jan 26, 2017 at 9:47 AM, Christophe Fergeau  wrote:
> On the virtio-win ISO, the win2k* drivers are in a path of the form
> '2k*', while the installer is looking for them in 'w2k*', and thus
> failing.
> This commit fixes the various paths
>
> This should fix https://bugzilla.redhat.com/show_bug.cgi?id=1416533 and
> https://bugzilla.redhat.com/show_bug.cgi?id=1416579
> ---
>  win-guest-tools.nsis | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/win-guest-tools.nsis b/win-guest-tools.nsis
> index 5f212f7..288e8a3 100644
> --- a/win-guest-tools.nsis
> +++ b/win-guest-tools.nsis
> @@ -288,17 +288,17 @@ Function GetDriverSubdir
> ${ElseIf} ${IsWin7}
>StrCpy $0 "$0\w7"
> ${ElseIf} ${IsWin2008}
> -  StrCpy $0 "$0\w2k8"
> +  StrCpy $0 "$0\2k8"
> ${ElseIf} ${IsWin2008R2}
> -  StrCpy $0 "$0\w2k8R2"
> +  StrCpy $0 "$0\2k8R2"
> ${ElseIf} ${IsWin8}
>StrCpy $0 "$0\w8"
> ${ElseIf} ${IsWin2012}
> -  StrCpy $0 "$0\w2k12"
> +  StrCpy $0 "$0\2k12"
> ${ElseIf} ${IsWin8.1}
>StrCpy $0 "$0\w8.1"
> ${ElseIf} ${IsWin2012R2}
> -  StrCpy $0 "$0\w2k12r2"
> +  StrCpy $0 "$0\2k12r2"
> ${ElseIf} ${IsWin10}
>StrCpy $0 "$0\w10"
> ${Else}
> --
> 2.9.3
>

Verified on win2k12r2-64 from oVirt jenkins build, looks good to me.
Merged to gitlab and oVirt.

Thanks a lot for the patch!
-- 
Didi
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server 3/6] spicevmc: Avoid computing some variable value if not necessary

[Frediano Ziglio]

> 
> On Thu, Jan 26, 2017 at 10:56:45AM +, Frediano Ziglio wrote:
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  server/spicevmc.c | 10 +-
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/server/spicevmc.c b/server/spicevmc.c
> > index a92ec7f..23e60b3 100644
> > --- a/server/spicevmc.c
> > +++ b/server/spicevmc.c
> > @@ -585,12 +585,13 @@ static uint8_t
> > *spicevmc_red_channel_alloc_msg_rcv_buf(RedChannelClient *rcc,
> > uint32_t size)
> >  {
> >  RedVmcChannel *channel;
> > -RedClient *client = red_channel_client_get_client(rcc);
> > -
> > -channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
> > +RedClient *client;
> >  
> >  switch (type) {
> >  case SPICE_MSGC_SPICEVMC_DATA:
> > +client = red_channel_client_get_client(rcc);
> > +channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
> > +
> >  assert(!channel->recv_from_client_buf);
> >  
> >  channel->recv_from_client_buf =
> >  red_char_device_write_buffer_get(channel->chardev,
> > @@ -615,10 +616,9 @@ static void
> > spicevmc_red_channel_release_msg_rcv_buf(RedChannelClient *rcc,
> >  {
> >  RedVmcChannel *channel;
> >  
> > -channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
> > -
> >  switch (type) {
> >  case SPICE_MSGC_SPICEVMC_DATA:
> > +channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
> >  /* buffer wasn't pushed to device */
> >  red_char_device_write_buffer_release(channel->chardev,
> >  &channel->recv_from_client_buf);
> >  break;
> 
> This is something that I'd just let the compiler handle if needed. Note
> that this also change some variables from being always initialized to
> sometimes being initialized, which is in my opinion is not necessarily a
> good move.
> 
> Christophe
> 

In this case the compiler cannot do it. How can it know that not calling
a function does not make any difference?
If you are going to use channel not initialized the compiler will
warn you so it's not a big deal.

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

Re: [Spice-devel] [PATCH spice-server 6/6] spicevmc: Reduce number of last saved IDs

[Uri Lublin]

On 01/26/2017 12:56 PM, Frediano Ziglio wrote:

Signed-off-by: Frediano Ziglio 


Ack.


---
 server/spicevmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server/spicevmc.c b/server/spicevmc.c
index 2039a80..db2dd99 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -224,7 +224,7 @@ red_vmc_channel_finalize(GObject *object)
 static RedVmcChannel *red_vmc_channel_new(RedsState *reds, uint8_t 
channel_type)
 {
 GType gtype = G_TYPE_NONE;
-static uint8_t id[256] = { 0, };
+static uint8_t id[SPICE_END_CHANNEL] = { 0, };

 switch (channel_type) {
 case SPICE_CHANNEL_USBREDIR:



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

Re: [Spice-devel] [PATCH spice-server 3/6] spicevmc: Avoid computing some variable value if not necessary

[Christophe Fergeau]

On Thu, Jan 26, 2017 at 10:56:45AM +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/spicevmc.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/server/spicevmc.c b/server/spicevmc.c
> index a92ec7f..23e60b3 100644
> --- a/server/spicevmc.c
> +++ b/server/spicevmc.c
> @@ -585,12 +585,13 @@ static uint8_t 
> *spicevmc_red_channel_alloc_msg_rcv_buf(RedChannelClient *rcc,
> uint32_t size)
>  {
>  RedVmcChannel *channel;
> -RedClient *client = red_channel_client_get_client(rcc);
> -
> -channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
> +RedClient *client;
>  
>  switch (type) {
>  case SPICE_MSGC_SPICEVMC_DATA:
> +client = red_channel_client_get_client(rcc);
> +channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
> +
>  assert(!channel->recv_from_client_buf);
>  
>  channel->recv_from_client_buf = 
> red_char_device_write_buffer_get(channel->chardev,
> @@ -615,10 +616,9 @@ static void 
> spicevmc_red_channel_release_msg_rcv_buf(RedChannelClient *rcc,
>  {
>  RedVmcChannel *channel;
>  
> -channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
> -
>  switch (type) {
>  case SPICE_MSGC_SPICEVMC_DATA:
> +channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
>  /* buffer wasn't pushed to device */
>  red_char_device_write_buffer_release(channel->chardev, 
> &channel->recv_from_client_buf);
>  break;

This is something that I'd just let the compiler handle if needed. Note
that this also change some variables from being always initialized to
sometimes being initialized, which is in my opinion is not necessarily a
good move.

Christophe


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

Re: [Spice-devel] [PATCH spice-server 5/6] spicevmc: Remove useless check

[Uri Lublin]

On 01/26/2017 12:56 PM, Frediano Ziglio wrote:

rcc is already deferenced in red_channel_client_get_client so
checking for NULL after that is uselss.


Hi Frediano,

On one hand you're right, it is useless to check if a pointer
is NULL after it was dereferenced.
On the other hand maybe that's wrong and we
should move the call to red_channel_client_get_client
after checking rcc.

It seems to me it's safe to assume rcc is not NULL
here.
I looked at other on_disconnect callbacks, some do
check rcc some don't.

Uri.



Signed-off-by: Frediano Ziglio 
---
 server/spicevmc.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/server/spicevmc.c b/server/spicevmc.c
index f441822..2039a80 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -452,10 +452,6 @@ static void 
spicevmc_red_channel_client_on_disconnect(RedChannelClient *rcc)
 SpiceCharDeviceInterface *sif;
 RedClient *client = red_channel_client_get_client(rcc);

-if (!rcc) {
-return;
-}
-
 channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));

 /* partial message which wasn't pushed to device */



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

Re: [Spice-devel] [PATCH spice-server 4/6] spicevmc: Remove leak of RedPortInitPipeItem::name

[Uri Lublin]

On 01/26/2017 12:56 PM, Frediano Ziglio wrote:

Signed-off-by: Frediano Ziglio 


Ack.

Uri.


---
 server/spicevmc.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/server/spicevmc.c b/server/spicevmc.c
index 23e60b3..f441822 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -376,13 +376,21 @@ static void 
spicevmc_chardev_send_msg_to_client(RedCharDevice *self,
 red_channel_client_pipe_add_push(channel->rcc, msg);
 }

+static void red_port_init_item_free(struct RedPipeItem *base)
+{
+RedPortInitPipeItem *item = SPICE_UPCAST(RedPortInitPipeItem, base);
+
+free(item->name);
+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);

-red_pipe_item_init(&item->base, RED_PIPE_ITEM_TYPE_PORT_INIT);
+red_pipe_item_init_full(&item->base, RED_PIPE_ITEM_TYPE_PORT_INIT, 
red_port_init_item_free);
 item->name = strdup(sin->portname);
 item->opened = channel->port_opened;
 red_channel_client_pipe_add_push(rcc, &item->base);



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

Re: [Spice-devel] [PATCH spice-server 1/6] spicevmc: Use spice_new instead of spice_malloc

[Uri Lublin]

On 01/26/2017 12:56 PM, Frediano Ziglio wrote:

spice_new is a bit more safe as return a properly typed pointer.


Hi Frediano,

I do not think it's much safer, as spice_new is just
a #define macro that (with second param 1)
calls spice_malloc and casts from void* ; but
it does not hurt either, so fine by me.

Uri.



Signed-off-by: Frediano Ziglio 
---
 server/spicevmc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/server/spicevmc.c b/server/spicevmc.c
index bbe72b5..0dc2b19 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -380,7 +380,7 @@ 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_malloc(sizeof(RedPortInitPipeItem));
+RedPortInitPipeItem *item = spice_new(RedPortInitPipeItem, 1);

 red_pipe_item_init(&item->base, RED_PIPE_ITEM_TYPE_PORT_INIT);
 item->name = strdup(sin->portname);
@@ -390,7 +390,7 @@ static void spicevmc_port_send_init(RedChannelClient *rcc)

 static void spicevmc_port_send_event(RedChannelClient *rcc, uint8_t event)
 {
-RedPortEventPipeItem *item = spice_malloc(sizeof(RedPortEventPipeItem));
+RedPortEventPipeItem *item = spice_new(RedPortEventPipeItem, 1);

 red_pipe_item_init(&item->base, RED_PIPE_ITEM_TYPE_PORT_EVENT);
 item->event = event;



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

Re: [Spice-devel] [spice-server v3 07/10] sound: Use RedChannelClient to receive/send data

[Christophe Fergeau]

On Thu, Jan 26, 2017 at 07:34:05AM -0500, Frediano Ziglio wrote:
> > +static void snd_persistent_pipe_item_free(struct RedPipeItem *item)
> > +{
> > +SndChannelClient *client = SPICE_CONTAINEROF(item, SndChannelClient,
> > persistent_pipe_item);
> > +
> > +red_pipe_item_init_full(item, RED_PIPE_ITEM_PERSISTENT,
> > +snd_persistent_pipe_item_free);
> > +
> > +if (client->on_message_done) {
> > +client->on_message_done(client);
> > +}
> > +}
> > +
> > +static void snd_send(SndChannelClient * client)
> > +{
> > +RedChannelClient *rcc = client->channel_client;
> > +
> > +if (!client || !red_channel_client_pipe_is_empty(rcc) ||
> > !client->command) {
> 
> the !client check here is useless after its use

I've changed this part to

@@ -715,10 +715,12 @@ static void snd_send(SndChannelClient * client)
 {
 RedChannelClient *rcc;

-if (!client || !red_channel_client_pipe_is_empty(rcc) || !client->command) 
{
-return;
-}
+g_return_if_fail(client != NULL);
+
 rcc = client->channel_client;
+if (!red_channel_client_pipe_is_empty(rcc) || !client->command) {
+return;
+}
 // just append a dummy item and push!
 red_pipe_item_init_full(&client->persistent_pipe_item, 
RED_PIPE_ITEM_PERSISTENT,
 snd_persistent_pipe_item_free);

(not 100% sure 'client' cannot be NULL here, so we might have to silence this if
this triggers during testing).

Christophe


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

Re: [Spice-devel] [PATCH spice-server v2] gitignore: Reduce html files exclusion

[Uri Lublin]

On 01/26/2017 12:52 PM, Frediano Ziglio wrote:

ping



Limit the html files ignored.
Can happen that you are working on some html files on your main
spice-server directory and it's not desirable to ignore them.


Hi Frediano,

How can that happen ?  What html files one may work on ?

I'd add in the commit log that html files under docs/ are still
ignored (be more specific with above "Limit").

Uri.




Signed-off-by: Frediano Ziglio 
---
 .gitignore | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Changes since v1:
- remove redundant ignore;
- move exclusion to get better order.

diff --git a/.gitignore b/.gitignore
index cac10f9..85f922b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -4,7 +4,6 @@
 *.tar.bz2
 *.tar.gz
 *.pyc
-*.html
 aclocal.m4
 autom4te.cache
 compile
@@ -34,7 +33,7 @@ INSTALL
 .version
 .tarball-version
 docs/manual/manual.chunked/
-docs/manual/manual.html
+docs/**/*.html
 .dirstamp
 .deps
 .libs


___
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] [spice-server v3 10/10] sound: Convert SndChannelClient to RedChannelClient

[Frediano Ziglio]

> 
> SndChannelClient is now inheriting from GObject, and has switched
> from using its own code for sending data to using RedChannelClient.
> This commit makes it directly inherit from RedChannelClient rather than
> having a channel_client field. This allows to get rid of the whole
> DummyChannel/DummyChannelClient code.
> 
> Based on a patch from Frediano Ziglio 
> 
> Signed-off-by: Christophe Fergeau 
> ---
>  server/Makefile.am|   2 -
>  server/dummy-channel-client.c | 140 --
>  server/dummy-channel-client.h |  65 ---
>  server/sound.c| 262
>  +-
>  4 files changed, 103 insertions(+), 366 deletions(-)
>  delete mode 100644 server/dummy-channel-client.c
>  delete mode 100644 server/dummy-channel-client.h
> 

...

> diff --git a/server/sound.c b/server/sound.c
> index 7738de4..899eecf 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -31,13 +31,10 @@
>  
>  #include "spice.h"
>  #include "red-common.h"
> -#include "dummy-channel-client.h"
>  #include "main-channel.h"
>  #include "reds.h"
>  #include "red-qxl.h"
>  #include "red-channel-client.h"
> -/* FIXME: for now, allow sound channel access to private RedChannelClient
> data */
> -#include "red-channel-client-private.h"
>  #include "red-client.h"
>  #include "sound.h"
>  #include "main-channel-client.h"
> @@ -84,17 +81,15 @@ typedef struct SpiceRecordState RecordChannel;
>  
>  typedef void (*snd_channel_on_message_done_proc)(SndChannelClient *client);
>  
> +
>  #define TYPE_SND_CHANNEL_CLIENT snd_channel_client_get_type()
>  #define SND_CHANNEL_CLIENT(obj) \
>  (G_TYPE_CHECK_INSTANCE_CAST((obj), TYPE_SND_CHANNEL_CLIENT,
>  SndChannelClient))
> -#define IS_SND_CHANNEL_CLIENT(obj) \
> -(G_TYPE_CHECK_INSTANCE_TYPE ((obj), TYPE_SND_CHANNEL_CLIENT))
>  GType snd_channel_client_get_type(void) G_GNUC_CONST;
>  
>  /* Connects an audio client to a Spice client */
>  struct SndChannelClient {
> -GObject parent;
> -RedChannelClient *channel_client;
> +RedChannelClient parent;
>  
>  int active;
>  int client_active;
> @@ -110,10 +105,10 @@ struct SndChannelClient {
>  };
>  
>  typedef struct SndChannelClientClass {
> -GObjectClass parent_class;
> +RedChannelClientClass parent_class;
>  } SndChannelClientClass;
>  
> -G_DEFINE_TYPE(SndChannelClient, snd_channel_client, G_TYPE_OBJECT)
> +G_DEFINE_TYPE(SndChannelClient, snd_channel_client, RED_TYPE_CHANNEL_CLIENT)
>  
>  
>  enum {
> @@ -253,65 +248,10 @@ static void snd_playback_start(SndChannel *channel);
>  static void snd_record_start(SndChannel *channel);
>  static void snd_send(SndChannelClient * client);
>  
> -enum {
> -PROP0,
> -PROP_CHANNEL_CLIENT
> -};
> -
> -static void
> -snd_channel_client_set_property(GObject *object,
> -guint property_id,
> -const GValue *value,
> -GParamSpec *pspec)
> -{
> -SndChannelClient *self = SND_CHANNEL_CLIENT(object);
> -
> -switch (property_id)
> -{
> -case PROP_CHANNEL_CLIENT:
> -self->channel_client = g_value_dup_object(value);
> -break;
> -default:
> -G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec);
> -}
> -}
> -
> -static SndChannelClient *snd_channel_client_from_dummy(RedChannelClient
> *dummy)
> -{
> -SndChannelClient *sound_client;
> -
> -g_assert(IS_DUMMY_CHANNEL_CLIENT(dummy));
> -sound_client =  g_object_get_data(G_OBJECT(dummy),
> "sound-channel-client");
> -g_assert(IS_SND_CHANNEL_CLIENT(sound_client));
> -
> -return sound_client;
> -}
> -
>  static RedsState* snd_channel_get_server(SndChannelClient *client)
>  {
>  g_return_val_if_fail(client != NULL, NULL);
> -return
> red_channel_get_server(red_channel_client_get_channel(client->channel_client));
> -}
> -
> -static void snd_disconnect_channel(SndChannelClient *client)
> -{
> -SndChannel *channel;
> -RedChannel *red_channel;
> -uint32_t type;
> -
> -if (!client || !red_channel_client_is_connected(client->channel_client))
> {
> -spice_debug("not connected");
> -return;
> -}
> -red_channel = red_channel_client_get_channel(client->channel_client);
> -g_object_get(red_channel, "channel-type", &type, NULL);
> -spice_debug("SndChannelClient=%p rcc=%p type=%d",
> - client, client->channel_client, type);
> -channel = SND_CHANNEL(red_channel);
> -red_channel_client_disconnect(channel->connection->channel_client);
> -channel->connection->channel_client = NULL;
> -g_object_unref(client);
> -channel->connection = NULL;
> +return
> red_channel_get_server(red_channel_client_get_channel(RED_CHANNEL_CLIENT(client)));
>  }
>  
>  static void snd_playback_free_frame(PlaybackChannelClient *playback_client,
>  AudioFrame *frame)
> @@ -394,8 +334,7 @@ playback_channel_handle_parsed(RedCha

Re: [Spice-devel] [spice-server v3 06/10] sound: Remove code from spice_server_record_get_samples()

[Frediano Ziglio]

> 
> The removed code was trying to read data when
> spice_server_record_get_samples() is called. Since reading of data is
> event-driven anyway (see snd_event), it's redundant to try
> again to read more data.
> This commit removes this code as this will some refactoring easier in
> the next commits.
> 
> Based on a patch from Frediano Ziglio 
> 
> Signed-off-by: Christophe Fergeau 
> ---
>  server/sound.c | 9 -
>  1 file changed, 9 deletions(-)
> 
> diff --git a/server/sound.c b/server/sound.c
> index ac69bfd..f27a53d 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -1474,15 +1474,6 @@ SPICE_GNUC_VISIBLE uint32_t
> spice_server_record_get_samples(SpiceRecordInstance
>  
>  len = MIN(record_client->write_pos - record_client->read_pos, bufsize);
>  
> -if (len < bufsize) {
> -SndChannel *channel =
> SND_CHANNEL(red_channel_client_get_channel(client->channel_client));
> -snd_receive(client);
> -if (!channel->connection) {
> -return 0;
> -}
> -len = MIN(record_client->write_pos - record_client->read_pos,
> bufsize);
> -}
> -
>  read_pos = record_client->read_pos % RECORD_SAMPLES_SIZE;
>  record_client->read_pos += len;
>  now = MIN(len, RECORD_SAMPLES_SIZE - read_pos);

Acked-by: Frediano Ziglio 

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

Re: [Spice-devel] [spice-server v3 03/10] sound: Add sanity checks in snd_{playback, record}_send

[Frediano Ziglio]

> 
> Filter out commands which should not happen. Should it be a
> g_warn_if_fail() or such instead?
> 
> Based on a patch from Frediano Ziglio 
> 
> Signed-off-by: Christophe Fergeau 
> ---
>  server/sound.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/server/sound.c b/server/sound.c
> index e04f1e7..b24d89e 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -851,6 +851,9 @@ static void snd_playback_send(void* data)
>  return;
>  }
>  
> +client->command &= SND_PLAYBACK_MODE_MASK|SND_PLAYBACK_PCM_MASK|
> +   SND_CTRL_MASK|SND_VOLUME_MUTE_MASK|
> +   SND_MIGRATE_MASK|SND_PLAYBACK_LATENCY_MASK;
>  while (client->command) {
>  if (client->command & SND_PLAYBACK_MODE_MASK) {
>  if (!playback_send_mode(playback_client)) {
> @@ -910,6 +913,7 @@ static void snd_record_send(void* data)
>  return;
>  }
>  
> +client->command &= SND_CTRL_MASK|SND_VOLUME_MUTE_MASK|SND_MIGRATE_MASK;
>  while (client->command) {
>  if (client->command & SND_CTRL_MASK) {
>  if (!snd_record_send_ctl(record_client)) {

Acked-by: Frediano Ziglio 

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

Re: [Spice-devel] [spice-server v3 02/10] sound: Implement snd_channel_config_socket

[Frediano Ziglio]

> 
> This is in preparation for switching SndChannelClient into a proper
> RedChannelClient. The prototype of the new helper matches what is
> expected from the RedChannel::config_socket vfunc.
> 
> To be able to achieve that, this commit associates the sound channel
> RedsStream instance with the DummyChannelClient instance we have, and
> then call snd_channel_config_socket() on that instance.
> 
> Based on a patch from Frediano Ziglio 
> 
> Signed-off-by: Christophe Fergeau 
> ---
>  server/dummy-channel-client.c |  2 +
>  server/dummy-channel-client.h |  1 +
>  server/sound.c| 99
>  +++
>  3 files changed, 55 insertions(+), 47 deletions(-)
> 
> diff --git a/server/dummy-channel-client.c b/server/dummy-channel-client.c
> index 61d5683..4efaa31 100644
> --- a/server/dummy-channel-client.c
> +++ b/server/dummy-channel-client.c
> @@ -104,6 +104,7 @@ dummy_channel_client_init(DummyChannelClient *self)
>  
>  RedChannelClient* dummy_channel_client_create(RedChannel *channel,
>RedClient  *client,
> +  RedsStream *stream,
>int num_common_caps,
>uint32_t *common_caps,
>int num_caps, uint32_t *caps)
> @@ -125,6 +126,7 @@ RedChannelClient* dummy_channel_client_create(RedChannel
> *channel,
>   NULL, NULL,
>   "channel", channel,
>   "client", client,
> + "stream", stream,
>   "caps", caps_array,
>   "common-caps", common_caps_array,
>   NULL);
> diff --git a/server/dummy-channel-client.h b/server/dummy-channel-client.h
> index 8013aa2..54e0e6c 100644
> --- a/server/dummy-channel-client.h
> +++ b/server/dummy-channel-client.h
> @@ -56,6 +56,7 @@ GType dummy_channel_client_get_type(void) G_GNUC_CONST;
>  
>  RedChannelClient *dummy_channel_client_create(RedChannel *channel,
>RedClient  *client,
> +  RedsStream *stream,
>int num_common_caps, uint32_t
>*common_caps,
>int num_caps, uint32_t *caps);
>  
> diff --git a/server/sound.c b/server/sound.c
> index 2165644..e04f1e7 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -938,6 +938,8 @@ static void snd_record_send(void* data)
>  }
>  }
>  
> +static int snd_channel_config_socket(RedChannelClient *rcc);
> +
>  static SndChannelClient *__new_channel(SndChannel *channel, int size,
>  uint32_t channel_id,
> RedClient *red_client,
> RedsStream *stream,
> @@ -949,50 +951,8 @@ static SndChannelClient *__new_channel(SndChannel
> *channel, int size, uint32_t c
> uint32_t *caps, int num_caps)
>  {
>  SndChannelClient *client;
> -int delay_val;
> -int flags;
> -#ifdef SO_PRIORITY
> -int priority;
> -#endif
> -int tos;
> -MainChannelClient *mcc = red_client_get_main(red_client);
>  RedsState *reds = red_channel_get_server(RED_CHANNEL(channel));
>  
> -spice_assert(stream);
> -if ((flags = fcntl(stream->socket, F_GETFL)) == -1) {
> -spice_printerr("accept failed, %s", strerror(errno));
> -goto error1;
> -}
> -
> -#ifdef SO_PRIORITY
> -priority = 6;
> -if (setsockopt(stream->socket, SOL_SOCKET, SO_PRIORITY,
> (void*)&priority,
> -   sizeof(priority)) == -1) {
> -if (errno != ENOTSUP) {
> -spice_printerr("setsockopt failed, %s", strerror(errno));
> -}
> -}
> -#endif
> -
> -tos = IPTOS_LOWDELAY;
> -if (setsockopt(stream->socket, IPPROTO_IP, IP_TOS, (void*)&tos,
> sizeof(tos)) == -1) {
> -if (errno != ENOTSUP) {
> -spice_printerr("setsockopt failed, %s", strerror(errno));
> -}
> -}
> -
> -delay_val = main_channel_client_is_low_bandwidth(mcc) ? 0 : 1;
> -if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY, &delay_val,
> sizeof(delay_val)) == -1) {
> -if (errno != ENOTSUP) {
> -spice_printerr("setsockopt failed, %s", strerror(errno));
> -}
> -}
> -
> -if (fcntl(stream->socket, F_SETFL, flags | O_NONBLOCK) == -1) {
> -spice_printerr("accept failed, %s", strerror(errno));
> -goto error1;
> -}
> -
>  spice_assert(size >= sizeof(*client));
>  client = spice_malloc0(size);
>  client->refs = 1;
> @@ -1017,24 +977,69 @@ static SndChannelClient *__new_channel(SndChannel
> *channel, int size, uint32_t c
>  client->cleanup = cleanup;
>  
>  client-

Re: [Spice-devel] [spice-server v3 01/10] sound: Rework spice_server_playback_get_buffer() error handling

[Frediano Ziglio]

> 
> The main goal of this commit is to avoid to dereference 'client' before
> it's checked for NULL. This meant splitting one error condition in 2
> separate ones.
> 
> This also sets default values for the 'frame' and 'num-samples'
> out parameters so that we just have to return on error conditions.
> 
> Based on a patch from Frediano Ziglio 
> 
> Signed-off-by: Christophe Fergeau 
> ---
>  server/sound.c | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/server/sound.c b/server/sound.c
> index bacd340..2165644 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -1166,11 +1166,14 @@ SPICE_GNUC_VISIBLE void
> spice_server_playback_get_buffer(SpicePlaybackInstance *
>   uint32_t **frame,
>   uint32_t
>   *num_samples)
>  {
>  SndChannelClient *client = sin->st->channel.connection;
> -PlaybackChannelClient *playback_client = SPICE_CONTAINEROF(client,
> PlaybackChannelClient, base);
>  
> -if (!client || !playback_client->free_frames) {
> -*frame = NULL;
> -*num_samples = 0;
> +*frame = NULL;
> +*num_samples = 0;
> +if (!client) {
> +return;
> +}
> +PlaybackChannelClient *playback_client = SPICE_CONTAINEROF(client,
> PlaybackChannelClient, base);
> +if (!playback_client->free_frames) {
>  return;
>  }
>  spice_assert(client->active);

Acked-by: Frediano Ziglio 

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

Re: [Spice-devel] [spice-server v3 07/10] sound: Use RedChannelClient to receive/send data

[Frediano Ziglio]

> 
> You can see that SndChannelClient has much less field
> as the code to read/write from/to client is reused from
> RedChannelClient instead of creating a fake RedChannelClient
> just to make the system happy.
> 
> One of the different between the old sound code and all other
> RedChannelClient objects was that the sound channel don't use
> a queue while RedChannelClient use RedPipeItem object. This was
> the main reason why RedChannelClient was not used. To implement
> the old behaviour a "persistent_pipe_item" is used. This RedPipeItem
> will be queued to RedChannelClient (only one!) so signal code we
> have data to send. The {playback,record}_channel_send_item will
> then send the messages to the client using RedChannelClient functions.
> For this reason snd_reset_send_data is replaced by a call to
> red_channel_client_init_send_data and snd_begin_send_message is
> replaced by red_channel_client_begin_send_message.
> 
> Signed-off-by: Frediano Ziglio 
> Signed-off-by: Christophe Fergeau 
> ---
>  server/sound.c | 659
>  +
>  1 file changed, 245 insertions(+), 414 deletions(-)
> 
> diff --git a/server/sound.c b/server/sound.c
> index f27a53d..2bb5688 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -82,8 +82,6 @@ typedef struct AudioFrameContainer AudioFrameContainer;
>  typedef struct SpicePlaybackState PlaybackChannel;
>  typedef struct SpiceRecordState RecordChannel;
>  
> -typedef void (*snd_channel_send_messages_proc)(void *in_channel);
> -typedef int (*snd_channel_handle_message_proc)(SndChannelClient *client,
> size_t size, uint32_t type, void *message);
>  typedef void (*snd_channel_on_message_done_proc)(SndChannelClient *client);
>  typedef void (*snd_channel_cleanup_channel_proc)(SndChannelClient *client);
>  



> @@ -994,6 +672,13 @@ static SndChannelClient *__new_channel(SndChannel
> *channel, int size, uint32_t c
>  if (!client->channel_client) {
>  goto error2;
>  }
> +
> +/* SndChannelClient is not yet a RedChannelClient, but we still need to
> go from our
> + * RedChannelClient implementation (DummyChannelClient) to the
> SndChannelClient instance
> + * in various vfuncs
> + */
> +g_object_set_data(G_OBJECT(client->channel_client),
> "sound-channel-client", client);
> +
>  if
>  (!snd_channel_config_socket(RED_CHANNEL_CLIENT(client->channel_client)))
>  {
>  goto error2;
>  }
> @@ -1005,6 +690,135 @@ error2:
>  return NULL;
>  }
>  
> +/* This function is called when the "persistent" item is removed from the
> + * queue. Note that there is not free call as the item is allocated into
> + * SndChannelClient.
> + * This is used to have a simple item in RedChannelClient queue but to send
> + * multiple messages in a row if possible.
> + * During realtime sound transmission you usually don't want to queue too
> + * much data or having retransmission preferring instead loosing some
> + * samples.
> + */
> +

I would remove this extra empty line 

> +static void snd_persistent_pipe_item_free(struct RedPipeItem *item)
> +{
> +SndChannelClient *client = SPICE_CONTAINEROF(item, SndChannelClient,
> persistent_pipe_item);
> +
> +red_pipe_item_init_full(item, RED_PIPE_ITEM_PERSISTENT,
> +snd_persistent_pipe_item_free);
> +
> +if (client->on_message_done) {
> +client->on_message_done(client);
> +}
> +}
> +
> +static void snd_send(SndChannelClient * client)
> +{
> +RedChannelClient *rcc = client->channel_client;
> +
> +if (!client || !red_channel_client_pipe_is_empty(rcc) ||
> !client->command) {

the !client check here is useless after its use

> +return;
> +}
> +// just append a dummy item and push!
> +red_pipe_item_init_full(&client->persistent_pipe_item,
> RED_PIPE_ITEM_PERSISTENT,
> +snd_persistent_pipe_item_free);
> +red_channel_client_pipe_add_push(rcc, &client->persistent_pipe_item);
> +}
> +
> +static void playback_channel_send_item(RedChannelClient *rcc, G_GNUC_UNUSED
> RedPipeItem *item)
> +{
> +SndChannelClient *client = snd_channel_client_from_dummy(rcc);
> +PlaybackChannelClient *playback_client = SPICE_CONTAINEROF(client,
> PlaybackChannelClient, base);
> +
> +client->command &= SND_PLAYBACK_MODE_MASK|SND_PLAYBACK_PCM_MASK|
> +   SND_CTRL_MASK|SND_VOLUME_MUTE_MASK|
> +   SND_MIGRATE_MASK|SND_PLAYBACK_LATENCY_MASK;
> +while (client->command) {
> +if (client->command & SND_PLAYBACK_MODE_MASK) {
> +client->command &= ~SND_PLAYBACK_MODE_MASK;
> +if (playback_send_mode(playback_client)) {
> +break;
> +}
> +}
> +if (client->command & SND_PLAYBACK_PCM_MASK) {
> +spice_assert(!playback_client->in_progress &&
> playback_client->pending_frame);
> +playback_client->in_progress = playback_client->pendin

[Spice-devel] [PATCH] Allow websockify path to be controlled

[Daniel P. Berrange]

The spice_auto.html file allows websockify path to be set via a
query parameter, but the standard spice.html does not provide
any mechanism for this. Add another form input field to allow
it to be set, defaulting to /websockify

Signed-off-by: Daniel P. Berrange 
---
 spice.html | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/spice.html b/spice.html
index d4c9962..e341f13 100644
--- a/spice.html
+++ b/spice.html
@@ -74,8 +74,13 @@
 
 host = document.getElementById("host").value;
 port = document.getElementById("port").value;
+token = document.getElementById("token").value;
 password = document.getElementById("password").value;
+path = document.getElementById("path").value;
 
+if (window.location.protocol == 'https:') {
+scheme = "wss://";
+}
 
 if ((!host) || (!port)) {
 console.log("must set host and port");
@@ -86,7 +91,7 @@
 sc.stop();
 }
 
-uri = scheme + host + ":" + port;
+uri = scheme + host + ":" + port + "/" + path + "?token=" + 
token;
 
 document.getElementById('connectButton').innerHTML = "Stop";
 document.getElementById('connectButton').onclick = disconnect;
@@ -165,7 +170,9 @@
 SPICE
 Host:  
 Port: 
+Token: 
 Password: 
+Path: 
 Start
 
 
-- 
2.9.3

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

[Spice-devel] [spice-server v3 08/10] sound: Prefer snd_set_command() over snd_*_send_*()

[Christophe Fergeau]

snd_set_command()/snd_send() are higher level methods which take care of
scheduling calls to the corresponding snd_*_send_*() methods when
appropriate. This commit switches a few direct snd_*_send_*() calls to
snd_set_command()/snd_send().

Based on a patch from Frediano Ziglio 

Signed-off-by: Christophe Fergeau 
Acked-by: Frediano Ziglio 
---
 server/sound.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index 2bb5688..b629377 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -928,7 +928,6 @@ SPICE_GNUC_VISIBLE void 
spice_server_playback_set_volume(SpicePlaybackInstance *
 {
 SpiceVolumeState *st = &sin->st->channel.volume;
 SndChannelClient *client = sin->st->channel.connection;
-PlaybackChannelClient *playback_client = SPICE_CONTAINEROF(client, 
PlaybackChannelClient, base);
 
 st->volume_nchannels = nchannels;
 free(st->volume);
@@ -937,21 +936,22 @@ SPICE_GNUC_VISIBLE void 
spice_server_playback_set_volume(SpicePlaybackInstance *
 if (!client || nchannels == 0)
 return;
 
-snd_playback_send_volume(playback_client);
+snd_set_command(client, SND_VOLUME_MASK);
+snd_send(client);
 }
 
 SPICE_GNUC_VISIBLE void spice_server_playback_set_mute(SpicePlaybackInstance 
*sin, uint8_t mute)
 {
 SpiceVolumeState *st = &sin->st->channel.volume;
 SndChannelClient *client = sin->st->channel.connection;
-PlaybackChannelClient *playback_client = SPICE_CONTAINEROF(client, 
PlaybackChannelClient, base);
 
 st->mute = mute;
 
 if (!client)
 return;
 
-snd_playback_send_mute(playback_client);
+snd_set_command(client, SND_MUTE_MASK);
+snd_send(client);
 }
 
 static void snd_playback_start(SndChannel *channel)
@@ -1210,7 +1210,6 @@ SPICE_GNUC_VISIBLE void 
spice_server_record_set_volume(SpiceRecordInstance *sin,
 {
 SpiceVolumeState *st = &sin->st->channel.volume;
 SndChannelClient *client = sin->st->channel.connection;
-RecordChannelClient *record_client = SPICE_CONTAINEROF(client, 
RecordChannelClient, base);
 
 st->volume_nchannels = nchannels;
 free(st->volume);
@@ -1219,21 +1218,22 @@ SPICE_GNUC_VISIBLE void 
spice_server_record_set_volume(SpiceRecordInstance *sin,
 if (!client || nchannels == 0)
 return;
 
-snd_record_send_volume(record_client);
+snd_set_command(client, SND_VOLUME_MASK);
+snd_send(client);
 }
 
 SPICE_GNUC_VISIBLE void spice_server_record_set_mute(SpiceRecordInstance *sin, 
uint8_t mute)
 {
 SpiceVolumeState *st = &sin->st->channel.volume;
 SndChannelClient *client = sin->st->channel.connection;
-RecordChannelClient *record_client = SPICE_CONTAINEROF(client, 
RecordChannelClient, base);
 
 st->mute = mute;
 
 if (!client)
 return;
 
-snd_record_send_mute(record_client);
+snd_set_command(client, SND_MUTE_MASK);
+snd_send(client);
 }
 
 static void snd_record_start(SndChannel *channel)
-- 
2.9.3

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

Re: [Spice-devel] [PATCH] qxl-wddm-dod: Fix for screen not shown after driver disabled

[Frediano Ziglio]

Acked-by: Frediano Ziglio  

Frediano 

- Original Message -

> From: "Yuri Benditovich" 
> To: "Frediano Ziglio" 
> Cc: spice-devel@lists.freedesktop.org, "Yan Vugenfirer" 
> Sent: Wednesday, January 25, 2017 5:45:23 PM
> Subject: Re: [Spice-devel] [PATCH] qxl-wddm-dod: Fix for screen not shown
> after driver disabled

> We use this name as video mode that the driver uses and passes to the
> BasicVideo is expected to be supported by BIOS or UEFI. "VGA Compatible"
> sounds like conformance to some specification, which is not a case. Name of
> the field " SupportNonVGA" related to this feature is also inclear and seems
> at least inverted. The difference between VGA mode and QXL mode is that
> first uses video modes declared by the BIOS and changes them via BIOS calls,
> when the seconds does not.

> On Wed, Jan 25, 2017 at 6:26 PM, Frediano Ziglio < fzig...@redhat.com >
> wrote:

> > >
> 
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1411340
> 
> > > Do not set SupportNonVGA field for device rev.4 and higher.
> 
> > > Then the class driver will not call miniport's procedure
> 
> > > StopDeviceAndReleasePostDisplayOwnership upon PnP stop.
> 
> > > QXL device modes are not compatible with VGA ones; the
> 
> > > driver in this procedure returns display information related
> 
> > > to QXL mode which later used by BasicDisplay driver and causes
> 
> > > it to show the screen incorrectly or fail to show it at all.
> 
> > >
> 
> > > Signed-off-by: Yuri Benditovich < yuri.benditov...@daynix.com >
> 
> > > ---
> 
> > > qxldod/QxlDod.cpp | 2 +-
> 
> > > qxldod/QxlDod.h | 2 ++
> 
> > > 2 files changed, 3 insertions(+), 1 deletion(-)
> 
> > >
> 
> > > diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
> 
> > > index 7adbec4..001cd67 100755
> 
> > > --- a/qxldod/QxlDod.cpp
> 
> > > +++ b/qxldod/QxlDod.cpp
> 
> > > @@ -411,7 +411,7 @@ NTSTATUS QxlDod::QueryAdapterInfo(_In_ CONST
> 
> > > DXGKARG_QUERYADAPTERINFO* pQueryAda
> 
> > > pDriverCaps->PointerCaps.Monochrome = 1;
> 
> > > pDriverCaps->PointerCaps.Color = 1;
> 
> > >
> 
> > > - pDriverCaps->SupportNonVGA = TRUE;
> 
> > > + pDriverCaps->SupportNonVGA = m_pHWDevice->IsBIOSCompatible();
> 
> > >
> 
> > > DbgPrint(TRACE_LEVEL_VERBOSE, ("<--- %s 1\n", __FUNCTION__));
> 
> > > return STATUS_SUCCESS;
> 
> > > diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h
> 
> > > index 4df4c61..8df1fcf 100755
> 
> > > --- a/qxldod/QxlDod.h
> 
> > > +++ b/qxldod/QxlDod.h
> 
> > > @@ -263,6 +263,7 @@ public:
> 
> > > virtual NTSTATUS Escape(_In_ CONST DXGKARG_ESCAPE* pEscap) = 0;
> 
> > > NTSTATUS AcquireDisplayInfo(DXGK_DISPLAY_INFORMATION& DispInfo);
> 
> > > ULONG GetId(void) { return m_Id; }
> 
> > > + virtual BOOLEAN IsBIOSCompatible() { return TRUE; }
> 
> > > protected:
> 
> > > virtual NTSTATUS GetModeList(DXGK_DISPLAY_INFORMATION* pDispInfo) = 0;
> 
> > > protected:
> 
> > > @@ -480,6 +481,7 @@ public:
> 
> > > NTSTATUS SetPointerShape(_In_ CONST DXGKARG_SETPOINTERSHAPE*
> 
> > > pSetPointerShape);
> 
> > > NTSTATUS SetPointerPosition(_In_ CONST DXGKARG_SETPOINTERPOSITION*
> 
> > > pSetPointerPosition);
> 
> > > NTSTATUS Escape(_In_ CONST DXGKARG_ESCAPE* pEscap);
> 
> > > + BOOLEAN IsBIOSCompatible() { return FALSE; }
> 
> > > protected:
> 
> > > NTSTATUS GetModeList(DXGK_DISPLAY_INFORMATION* pDispInfo);
> 
> > > VOID BltBits (BLT_INFO* pDst,
> 

> > Why you used IsBIOSCompatible name instead of IsVGACompatible ?
> 

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

Re: [Spice-devel] [spice-server v2 09/10] sound: Turn {Playback, Record}ChannelClient into GObjects

[Christophe Fergeau]

Hey,

On Thu, Jan 26, 2017 at 02:55:03AM -0500, Frediano Ziglio wrote:
> > 
> > This is in preparation for making them inherit from RedChannelClient.
> > Doing it in one go would result in a very huge commit, so this commit
> > starts by turning these into GObjects, while still using a
> > DummyChannelClient instance for sending the data.
> > 
> > Based on a patch from Frediano Ziglio 
> > 
> > Signed-off-by: Christophe Fergeau 
> 
> IMO this patch is too artificial, I would merge to 10/10

To be honest, I'm a bit torn on this one. I totally agree that it's very
artificial. On the other hand, it also reduces the per-commit changes in sound.c
significantly:


server/sound.c | 325 
+-
and then
server/sound.c | 262 
++-

VS

server/sound.c | 411 
+--
For the combined patch

and we've got a more or less mechanical patch turning SndChannelClient
into a GObject, and then another one removing
SndChannelClient::channel_client. So not fully sure what is best between
merging the two patches or not :-/

Christophe


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

[Spice-devel] [spice-server v3 07/10] sound: Use RedChannelClient to receive/send data

[Christophe Fergeau]

You can see that SndChannelClient has much less field
as the code to read/write from/to client is reused from
RedChannelClient instead of creating a fake RedChannelClient
just to make the system happy.

One of the different between the old sound code and all other
RedChannelClient objects was that the sound channel don't use
a queue while RedChannelClient use RedPipeItem object. This was
the main reason why RedChannelClient was not used. To implement
the old behaviour a "persistent_pipe_item" is used. This RedPipeItem
will be queued to RedChannelClient (only one!) so signal code we
have data to send. The {playback,record}_channel_send_item will
then send the messages to the client using RedChannelClient functions.
For this reason snd_reset_send_data is replaced by a call to
red_channel_client_init_send_data and snd_begin_send_message is
replaced by red_channel_client_begin_send_message.

Signed-off-by: Frediano Ziglio 
Signed-off-by: Christophe Fergeau 
---
 server/sound.c | 659 +
 1 file changed, 245 insertions(+), 414 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index f27a53d..2bb5688 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -82,8 +82,6 @@ typedef struct AudioFrameContainer AudioFrameContainer;
 typedef struct SpicePlaybackState PlaybackChannel;
 typedef struct SpiceRecordState RecordChannel;
 
-typedef void (*snd_channel_send_messages_proc)(void *in_channel);
-typedef int (*snd_channel_handle_message_proc)(SndChannelClient *client, 
size_t size, uint32_t type, void *message);
 typedef void (*snd_channel_on_message_done_proc)(SndChannelClient *client);
 typedef void (*snd_channel_cleanup_channel_proc)(SndChannelClient *client);
 
@@ -91,37 +89,30 @@ typedef void 
(*snd_channel_cleanup_channel_proc)(SndChannelClient *client);
 
 /* Connects an audio client to a Spice client */
 struct SndChannelClient {
-RedsStream *stream;
-spice_parse_channel_func_t parser;
 int refs;
 
 RedChannelClient *channel_client;
 
 int active;
 int client_active;
-int blocked;
 
 uint32_t command;
 
-struct {
-uint64_t serial;
-uint32_t size;
-uint32_t pos;
-} send_data;
+/* we don't expect very big messages so don't allocate too much
+ * bytes, data will be cached in RecordChannelClient::samples */
+uint8_t receive_buf[SND_CODEC_MAX_FRAME_BYTES + 64];
+RedPipeItem persistent_pipe_item;
 
-struct {
-uint8_t buf[SND_RECEIVE_BUF_SIZE];
-uint8_t *message_start;
-uint8_t *now;
-uint8_t *end;
-} receive_data;
-
-snd_channel_send_messages_proc send_messages;
-snd_channel_handle_message_proc handle_message;
 snd_channel_on_message_done_proc on_message_done;
 snd_channel_cleanup_channel_proc cleanup;
 };
 
+
+enum {
+RED_PIPE_ITEM_PERSISTENT = RED_PIPE_ITEM_TYPE_CHANNEL_BASE,
+};
+
+
 struct AudioFrame {
 uint32_t time;
 uint32_t samples[SND_CODEC_MAX_FRAME_SIZE];
@@ -226,10 +217,10 @@ struct RecordChannelClient {
 /* A list of all Spice{Playback,Record}State objects */
 static SndChannel *snd_channels;
 
-static void snd_receive(SndChannelClient *client);
 static void snd_playback_start(SndChannel *channel);
 static void snd_record_start(SndChannel *channel);
 static void snd_playback_alloc_frames(PlaybackChannelClient *playback);
+static void snd_send(SndChannelClient * client);
 
 static SndChannelClient *snd_channel_unref(SndChannelClient *client)
 {
@@ -241,6 +232,16 @@ static SndChannelClient 
*snd_channel_unref(SndChannelClient *client)
 return client;
 }
 
+static SndChannelClient *snd_channel_client_from_dummy(RedChannelClient *dummy)
+{
+SndChannelClient *sound_client;
+
+g_assert(IS_DUMMY_CHANNEL_CLIENT(dummy));
+sound_client =  g_object_get_data(G_OBJECT(dummy), "sound-channel-client");
+
+return sound_client;
+}
+
 static RedsState* snd_channel_get_server(SndChannelClient *client)
 {
 g_return_val_if_fail(client != NULL, NULL);
@@ -250,16 +251,14 @@ static RedsState* snd_channel_get_server(SndChannelClient 
*client)
 static void snd_disconnect_channel(SndChannelClient *client)
 {
 SndChannel *channel;
-RedsState *reds;
 RedChannel *red_channel;
 uint32_t type;
 
-if (!client || !client->stream) {
+if (!client || !red_channel_client_is_connected(client->channel_client)) {
 spice_debug("not connected");
 return;
 }
 red_channel = red_channel_client_get_channel(client->channel_client);
-reds = snd_channel_get_server(client);
 g_object_get(red_channel, "channel-type", &type, NULL);
 spice_debug("SndChannelClient=%p rcc=%p type=%d",
  client, client->channel_client, type);
@@ -267,10 +266,6 @@ static void snd_disconnect_channel(SndChannelClient 
*client)
 client->cleanup(client);
 red_channel_client_disconnect(channel->connection->channel_client);
 channel->connection->channel_client = NULL;
-

[Spice-devel] [spice-server v3 01/10] sound: Rework spice_server_playback_get_buffer() error handling

[Christophe Fergeau]

The main goal of this commit is to avoid to dereference 'client' before
it's checked for NULL. This meant splitting one error condition in 2
separate ones.

This also sets default values for the 'frame' and 'num-samples'
out parameters so that we just have to return on error conditions.

Based on a patch from Frediano Ziglio 

Signed-off-by: Christophe Fergeau 
---
 server/sound.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index bacd340..2165644 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -1166,11 +1166,14 @@ SPICE_GNUC_VISIBLE void 
spice_server_playback_get_buffer(SpicePlaybackInstance *
  uint32_t **frame, 
uint32_t *num_samples)
 {
 SndChannelClient *client = sin->st->channel.connection;
-PlaybackChannelClient *playback_client = SPICE_CONTAINEROF(client, 
PlaybackChannelClient, base);
 
-if (!client || !playback_client->free_frames) {
-*frame = NULL;
-*num_samples = 0;
+*frame = NULL;
+*num_samples = 0;
+if (!client) {
+return;
+}
+PlaybackChannelClient *playback_client = SPICE_CONTAINEROF(client, 
PlaybackChannelClient, base);
+if (!playback_client->free_frames) {
 return;
 }
 spice_assert(client->active);
-- 
2.9.3

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

[Spice-devel] [spice-server v3 09/10] sound: Turn {Playback, Record}ChannelClient into GObjects

[Christophe Fergeau]

This is in preparation for making them inherit from RedChannelClient.
Doing it in one go would result in a very huge commit, so this commit
starts by turning these into GObjects, while still using a
DummyChannelClient instance for sending the data.

Based on a patch from Frediano Ziglio 

Signed-off-by: Christophe Fergeau 
---
 server/sound.c | 325 +
 1 file changed, 210 insertions(+), 115 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index b629377..7738de4 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -83,14 +83,17 @@ typedef struct SpicePlaybackState PlaybackChannel;
 typedef struct SpiceRecordState RecordChannel;
 
 typedef void (*snd_channel_on_message_done_proc)(SndChannelClient *client);
-typedef void (*snd_channel_cleanup_channel_proc)(SndChannelClient *client);
 
-#define SND_CHANNEL_CLIENT(obj) (&(obj)->base)
+#define TYPE_SND_CHANNEL_CLIENT snd_channel_client_get_type()
+#define SND_CHANNEL_CLIENT(obj) \
+(G_TYPE_CHECK_INSTANCE_CAST((obj), TYPE_SND_CHANNEL_CLIENT, 
SndChannelClient))
+#define IS_SND_CHANNEL_CLIENT(obj) \
+(G_TYPE_CHECK_INSTANCE_TYPE ((obj), TYPE_SND_CHANNEL_CLIENT))
+GType snd_channel_client_get_type(void) G_GNUC_CONST;
 
 /* Connects an audio client to a Spice client */
 struct SndChannelClient {
-int refs;
-
+GObject parent;
 RedChannelClient *channel_client;
 
 int active;
@@ -104,9 +107,14 @@ struct SndChannelClient {
 RedPipeItem persistent_pipe_item;
 
 snd_channel_on_message_done_proc on_message_done;
-snd_channel_cleanup_channel_proc cleanup;
 };
 
+typedef struct SndChannelClientClass {
+GObjectClass parent_class;
+} SndChannelClientClass;
+
+G_DEFINE_TYPE(SndChannelClient, snd_channel_client, G_TYPE_OBJECT)
+
 
 enum {
 RED_PIPE_ITEM_PERSISTENT = RED_PIPE_ITEM_TYPE_CHANNEL_BASE,
@@ -129,8 +137,13 @@ struct AudioFrameContainer
 AudioFrame items[NUM_AUDIO_FRAMES];
 };
 
+#define TYPE_PLAYBACK_CHANNEL_CLIENT playback_channel_client_get_type()
+#define PLAYBACK_CHANNEL_CLIENT(obj) \
+(G_TYPE_CHECK_INSTANCE_CAST((obj), TYPE_PLAYBACK_CHANNEL_CLIENT, 
PlaybackChannelClient))
+GType playback_channel_client_get_type(void) G_GNUC_CONST;
+
 struct PlaybackChannelClient {
-SndChannelClient base;
+SndChannelClient parent;
 
 AudioFrameContainer *frames;
 AudioFrame *free_frames;
@@ -142,6 +155,13 @@ struct PlaybackChannelClient {
 uint8_t  encode_buf[SND_CODEC_MAX_COMPRESSED_BYTES];
 };
 
+typedef struct PlaybackChannelClientClass {
+SndChannelClientClass parent_class;
+} PlaybackChannelClientClass;
+
+G_DEFINE_TYPE(PlaybackChannelClient, playback_channel_client, 
TYPE_SND_CHANNEL_CLIENT)
+
+
 typedef struct SpiceVolumeState {
 uint16_t *volume;
 uint8_t volume_nchannels;
@@ -202,8 +222,13 @@ typedef struct RecordChannelClass {
 G_DEFINE_TYPE(RecordChannel, record_channel, TYPE_SND_CHANNEL)
 
 
+#define TYPE_RECORD_CHANNEL_CLIENT record_channel_client_get_type()
+#define RECORD_CHANNEL_CLIENT(obj) \
+(G_TYPE_CHECK_INSTANCE_CAST((obj), TYPE_RECORD_CHANNEL_CLIENT, 
RecordChannelClient))
+GType record_channel_client_get_type(void) G_GNUC_CONST;
+
 struct RecordChannelClient {
-SndChannelClient base;
+SndChannelClient parent;
 uint32_t samples[RECORD_SAMPLES_SIZE];
 uint32_t write_pos;
 uint32_t read_pos;
@@ -214,22 +239,41 @@ struct RecordChannelClient {
 uint8_t  decode_buf[SND_CODEC_MAX_FRAME_BYTES];
 };
 
+typedef struct RecordChannelClientClass {
+SndChannelClientClass parent_class;
+} RecordChannelClientClass;
+
+G_DEFINE_TYPE(RecordChannelClient, record_channel_client, 
TYPE_SND_CHANNEL_CLIENT)
+
+
 /* A list of all Spice{Playback,Record}State objects */
 static SndChannel *snd_channels;
 
 static void snd_playback_start(SndChannel *channel);
 static void snd_record_start(SndChannel *channel);
-static void snd_playback_alloc_frames(PlaybackChannelClient *playback);
 static void snd_send(SndChannelClient * client);
 
-static SndChannelClient *snd_channel_unref(SndChannelClient *client)
+enum {
+PROP0,
+PROP_CHANNEL_CLIENT
+};
+
+static void
+snd_channel_client_set_property(GObject *object,
+guint property_id,
+const GValue *value,
+GParamSpec *pspec)
 {
-if (!--client->refs) {
-spice_printerr("SndChannelClient=%p freed", client);
-free(client);
-return NULL;
+SndChannelClient *self = SND_CHANNEL_CLIENT(object);
+
+switch (property_id)
+{
+case PROP_CHANNEL_CLIENT:
+self->channel_client = g_value_dup_object(value);
+break;
+default:
+G_OBJECT_WARN_INVALID_PROPERTY_ID(object, property_id, pspec);
 }
-return client;
 }
 
 static SndChannelClient *snd_channel_client_from_dummy(RedChannelClient *dummy)
@@ -238,6 +282,7 @@ static SndChannelClient 
*snd_channel_client_from_dummy(RedChannelClient *dum

[Spice-devel] [spice-server v3 03/10] sound: Add sanity checks in snd_{playback, record}_send

[Christophe Fergeau]

Filter out commands which should not happen. Should it be a
g_warn_if_fail() or such instead?

Based on a patch from Frediano Ziglio 

Signed-off-by: Christophe Fergeau 
---
 server/sound.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/server/sound.c b/server/sound.c
index e04f1e7..b24d89e 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -851,6 +851,9 @@ static void snd_playback_send(void* data)
 return;
 }
 
+client->command &= SND_PLAYBACK_MODE_MASK|SND_PLAYBACK_PCM_MASK|
+   SND_CTRL_MASK|SND_VOLUME_MUTE_MASK|
+   SND_MIGRATE_MASK|SND_PLAYBACK_LATENCY_MASK;
 while (client->command) {
 if (client->command & SND_PLAYBACK_MODE_MASK) {
 if (!playback_send_mode(playback_client)) {
@@ -910,6 +913,7 @@ static void snd_record_send(void* data)
 return;
 }
 
+client->command &= SND_CTRL_MASK|SND_VOLUME_MUTE_MASK|SND_MIGRATE_MASK;
 while (client->command) {
 if (client->command & SND_CTRL_MASK) {
 if (!snd_record_send_ctl(record_client)) {
-- 
2.9.3

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

[Spice-devel] [spice-server v3 10/10] sound: Convert SndChannelClient to RedChannelClient

[Christophe Fergeau]

SndChannelClient is now inheriting from GObject, and has switched
from using its own code for sending data to using RedChannelClient.
This commit makes it directly inherit from RedChannelClient rather than
having a channel_client field. This allows to get rid of the whole
DummyChannel/DummyChannelClient code.

Based on a patch from Frediano Ziglio 

Signed-off-by: Christophe Fergeau 
---
 server/Makefile.am|   2 -
 server/dummy-channel-client.c | 140 --
 server/dummy-channel-client.h |  65 ---
 server/sound.c| 262 +-
 4 files changed, 103 insertions(+), 366 deletions(-)
 delete mode 100644 server/dummy-channel-client.c
 delete mode 100644 server/dummy-channel-client.h

diff --git a/server/Makefile.am b/server/Makefile.am
index 90ff779..1442bf1 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -104,8 +104,6 @@ libserver_la_SOURCES =  \
red-channel-client-private.h\
red-client.c\
red-client.h\
-   dummy-channel-client.c  \
-   dummy-channel-client.h  \
red-common.h\
dispatcher.c\
dispatcher.h\
diff --git a/server/dummy-channel-client.c b/server/dummy-channel-client.c
deleted file mode 100644
index 4efaa31..000
--- a/server/dummy-channel-client.c
+++ /dev/null
@@ -1,140 +0,0 @@
-/*
-   Copyright (C) 2009-2015 Red Hat, Inc.
-
-   This library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   This library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with this library; if not, see .
-*/
-#ifdef HAVE_CONFIG_H
-#include 
-#endif
-
-#include "dummy-channel-client.h"
-#include "red-channel.h"
-#include "red-client.h"
-
-static void dummy_channel_client_initable_interface_init(GInitableIface 
*iface);
-
-G_DEFINE_TYPE_WITH_CODE(DummyChannelClient, dummy_channel_client, 
RED_TYPE_CHANNEL_CLIENT,
-G_IMPLEMENT_INTERFACE(G_TYPE_INITABLE,
-  
dummy_channel_client_initable_interface_init))
-
-#define DUMMY_CHANNEL_CLIENT_PRIVATE(o) \
-(G_TYPE_INSTANCE_GET_PRIVATE((o), TYPE_DUMMY_CHANNEL_CLIENT, 
DummyChannelClientPrivate))
-
-struct DummyChannelClientPrivate
-{
-gboolean connected;
-};
-
-static gboolean dummy_channel_client_initable_init(GInitable *initable,
-   GCancellable *cancellable,
-   GError **error)
-{
-GError *local_error = NULL;
-RedChannelClient *rcc = RED_CHANNEL_CLIENT(initable);
-RedClient *client = red_channel_client_get_client(rcc);
-RedChannel *channel = red_channel_client_get_channel(rcc);
-
-red_channel_add_client(channel, rcc);
-if (!red_client_add_channel(client, rcc, &local_error)) {
-red_channel_remove_client(channel, rcc);
-}
-
-if (local_error) {
-g_warning("Failed to create channel client: %s", local_error->message);
-g_propagate_error(error, local_error);
-}
-return local_error == NULL;
-}
-
-static void dummy_channel_client_initable_interface_init(GInitableIface *iface)
-{
-iface->init = dummy_channel_client_initable_init;
-}
-
-static gboolean dummy_channel_client_is_connected(RedChannelClient *rcc)
-{
-return DUMMY_CHANNEL_CLIENT(rcc)->priv->connected;
-}
-
-static void dummy_channel_client_disconnect(RedChannelClient *rcc)
-{
-DummyChannelClient *self = DUMMY_CHANNEL_CLIENT(rcc);
-RedChannel *channel = red_channel_client_get_channel(rcc);
-GList *link;
-uint32_t type, id;
-
-if (channel && (link = g_list_find(red_channel_get_clients(channel), 
rcc))) {
-g_object_get(channel, "channel-type", &type, "id", &id, NULL);
-spice_printerr("rcc=%p (channel=%p type=%d id=%d)", rcc, channel,
-   type, id);
-red_channel_remove_client(channel, link->data);
-}
-self->priv->connected = FALSE;
-}
-
-static void
-dummy_channel_client_class_init(DummyChannelClientClass *klass)
-{
-RedChannelClientClass *cc_class = RED_CHANNEL_CLIENT_CLASS(klass);
-
-g_type_class_add_private(klass, sizeof(DummyChannelClientPrivate));
-
-cc_class->is_connected = dummy_channel_client_is_connected;
-cc_class->disconnect = du

[Spice-devel] [spice-server v3 04/10] sound: Remove SndChannelClient::send_data::marshaller

[Christophe Fergeau]

We can use the marshaller provided by the dummy RedChannelClient
associated with SndChannelClient.

Based on a patch from Frediano Ziglio 

Signed-off-by: Christophe Fergeau 
Acked-by: Frediano Ziglio 
---
 server/sound.c | 49 -
 1 file changed, 28 insertions(+), 21 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index b24d89e..a0b6675 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -26,7 +26,6 @@
 #include 
 #include 
 
-#include 
 #include 
 #include 
 
@@ -41,7 +40,6 @@
 #include "red-channel-client-private.h"
 #include "red-client.h"
 #include "sound.h"
-#include "demarshallers.h"
 #include "main-channel-client.h"
 
 #ifndef IOV_MAX
@@ -108,7 +106,6 @@ struct SndChannelClient {
 
 struct {
 uint64_t serial;
-SpiceMarshaller *marshaller;
 uint32_t size;
 uint32_t pos;
 } send_data;
@@ -275,7 +272,6 @@ static void snd_disconnect_channel(SndChannelClient *client)
 client->stream->watch = NULL;
 reds_stream_free(client->stream);
 client->stream = NULL;
-spice_marshaller_destroy(client->send_data.marshaller);
 snd_channel_unref(client);
 channel->connection = NULL;
 }
@@ -306,6 +302,8 @@ static void snd_record_on_message_done(SndChannelClient 
*client)
 static int snd_send_data(SndChannelClient *client)
 {
 uint32_t n;
+RedChannelClient *rcc = client->channel_client;
+SpiceMarshaller *m = red_channel_client_get_marshaller(rcc);
 
 if (!client) {
 return FALSE;
@@ -330,7 +328,7 @@ static int snd_send_data(SndChannelClient *client)
 break;
 }
 
-vec_size = spice_marshaller_fill_iovec(client->send_data.marshaller,
+vec_size = spice_marshaller_fill_iovec(m,
vec, IOV_MAX, 
client->send_data.pos);
 n = reds_stream_writev(client->stream, vec, vec_size);
 if (n == -1) {
@@ -562,17 +560,17 @@ static void snd_event(int fd, int event, void *data)
 static inline int snd_reset_send_data(SndChannelClient *client, uint16_t verb)
 {
 SpiceDataHeaderOpaque *header;
+RedChannelClient *rcc = client->channel_client;
+SpiceMarshaller *m = red_channel_client_get_marshaller(rcc);
 
 if (!client) {
 return FALSE;
 }
 
 header = &client->channel_client->priv->send_data.header;
-spice_marshaller_reset(client->send_data.marshaller);
-header->data = spice_marshaller_reserve_space(client->send_data.marshaller,
-  header->header_size);
-spice_marshaller_set_base(client->send_data.marshaller,
-  header->header_size);
+spice_marshaller_reset(m);
+header->data = spice_marshaller_reserve_space(m, header->header_size);
+spice_marshaller_set_base(m, header->header_size);
 client->send_data.pos = 0;
 header->set_msg_size(header, 0);
 header->set_msg_type(header, verb);
@@ -588,16 +586,19 @@ static inline int snd_reset_send_data(SndChannelClient 
*client, uint16_t verb)
 static int snd_begin_send_message(SndChannelClient *client)
 {
 SpiceDataHeaderOpaque *header = 
&client->channel_client->priv->send_data.header;
+RedChannelClient *rcc = client->channel_client;
+SpiceMarshaller *m = red_channel_client_get_marshaller(rcc);
 
-spice_marshaller_flush(client->send_data.marshaller);
-client->send_data.size = 
spice_marshaller_get_total_size(client->send_data.marshaller);
+spice_marshaller_flush(m);
+client->send_data.size = spice_marshaller_get_total_size(m);
 header->set_msg_size(header, client->send_data.size - header->header_size);
 return snd_send_data(client);
 }
 
 static int snd_channel_send_migrate(SndChannelClient *client)
 {
-SpiceMarshaller *m = client->send_data.marshaller;
+RedChannelClient *rcc = client->channel_client;
+SpiceMarshaller *m = red_channel_client_get_marshaller(rcc);
 SpiceMsgMigrate migrate;
 
 if (!snd_reset_send_data(client, SPICE_MSG_MIGRATE)) {
@@ -619,8 +620,9 @@ static int snd_send_volume(SndChannelClient *client, 
uint32_t cap, int msg)
 {
 SpiceMsgAudioVolume *vol;
 uint8_t c;
+RedChannelClient *rcc = client->channel_client;
+SpiceMarshaller *m = red_channel_client_get_marshaller(rcc);
 SpiceVolumeState *st = &client->channel->volume;
-SpiceMarshaller *m = client->send_data.marshaller;
 
 if (!red_channel_client_test_remote_cap(client->channel_client, cap)) {
 return TRUE;
@@ -649,8 +651,9 @@ static int snd_playback_send_volume(PlaybackChannelClient 
*playback_client)
 static int snd_send_mute(SndChannelClient *client, uint32_t cap, int msg)
 {
 SpiceMsgAudioMute mute;
+RedChannelClient *rcc = client->channel_client;
+SpiceMarshaller *m = red_channel_client_get_marshaller(rcc);
 SpiceVolumeState *st = &client->channel->volume;
-SpiceMarshaller *m = client->send_data.marshaller;
 
 if (!red_channel_client_test_r

[Spice-devel] [spice-server v3 06/10] sound: Remove code from spice_server_record_get_samples()

[Christophe Fergeau]

The removed code was trying to read data when
spice_server_record_get_samples() is called. Since reading of data is
event-driven anyway (see snd_event), it's redundant to try
again to read more data.
This commit removes this code as this will some refactoring easier in
the next commits.

Based on a patch from Frediano Ziglio 

Signed-off-by: Christophe Fergeau 
---
 server/sound.c | 9 -
 1 file changed, 9 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index ac69bfd..f27a53d 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -1474,15 +1474,6 @@ SPICE_GNUC_VISIBLE uint32_t 
spice_server_record_get_samples(SpiceRecordInstance
 
 len = MIN(record_client->write_pos - record_client->read_pos, bufsize);
 
-if (len < bufsize) {
-SndChannel *channel = 
SND_CHANNEL(red_channel_client_get_channel(client->channel_client));
-snd_receive(client);
-if (!channel->connection) {
-return 0;
-}
-len = MIN(record_client->write_pos - record_client->read_pos, bufsize);
-}
-
 read_pos = record_client->read_pos % RECORD_SAMPLES_SIZE;
 record_client->read_pos += len;
 now = MIN(len, RECORD_SAMPLES_SIZE - read_pos);
-- 
2.9.3

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

[Spice-devel] [spice-server v3 05/10] sound: Remove SndChannelClient::channel

[Christophe Fergeau]

We can get it from our DummyChannelClient rather than storing it in
SndChannelClient.

Based on a patch from Frediano Ziglio 

Signed-off-by: Christophe Fergeau 
Acked-by: Frediano Ziglio 
---
 server/sound.c | 21 +++--
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index a0b6675..ac69bfd 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -92,7 +92,6 @@ typedef void 
(*snd_channel_cleanup_channel_proc)(SndChannelClient *client);
 /* Connects an audio client to a Spice client */
 struct SndChannelClient {
 RedsStream *stream;
-SndChannel *channel;
 spice_parse_channel_func_t parser;
 int refs;
 
@@ -245,7 +244,7 @@ static SndChannelClient *snd_channel_unref(SndChannelClient 
*client)
 static RedsState* snd_channel_get_server(SndChannelClient *client)
 {
 g_return_val_if_fail(client != NULL, NULL);
-return red_channel_get_server(RED_CHANNEL(client->channel));
+return 
red_channel_get_server(red_channel_client_get_channel(client->channel_client));
 }
 
 static void snd_disconnect_channel(SndChannelClient *client)
@@ -264,7 +263,7 @@ static void snd_disconnect_channel(SndChannelClient *client)
 g_object_get(red_channel, "channel-type", &type, NULL);
 spice_debug("SndChannelClient=%p rcc=%p type=%d",
  client, client->channel_client, type);
-channel = client->channel;
+channel = SND_CHANNEL(red_channel);
 client->cleanup(client);
 red_channel_client_disconnect(channel->connection->channel_client);
 channel->connection->channel_client = NULL;
@@ -420,6 +419,7 @@ static int snd_playback_handle_message(SndChannelClient 
*client, size_t size, ui
 static int snd_record_handle_message(SndChannelClient *client, size_t size, 
uint32_t type, void *message)
 {
 RecordChannelClient *record_client = (RecordChannelClient *)client;
+RedChannelClient *rcc = client->channel_client;
 
 if (!client) {
 return FALSE;
@@ -429,7 +429,7 @@ static int snd_record_handle_message(SndChannelClient 
*client, size_t size, uint
 return snd_record_handle_write(record_client, size, message);
 case SPICE_MSGC_RECORD_MODE: {
 SpiceMsgcRecordMode *mode = (SpiceMsgcRecordMode *)message;
-SndChannel *channel = client->channel;
+SndChannel *channel = SND_CHANNEL(red_channel_client_get_channel(rcc));
 record_client->mode_time = mode->time;
 if (mode->mode != SPICE_AUDIO_DATA_MODE_RAW) {
 if (snd_codec_is_capable(mode->mode, channel->frequency)) {
@@ -622,7 +622,8 @@ static int snd_send_volume(SndChannelClient *client, 
uint32_t cap, int msg)
 uint8_t c;
 RedChannelClient *rcc = client->channel_client;
 SpiceMarshaller *m = red_channel_client_get_marshaller(rcc);
-SpiceVolumeState *st = &client->channel->volume;
+SndChannel *channel = SND_CHANNEL(red_channel_client_get_channel(rcc));
+SpiceVolumeState *st = &channel->volume;
 
 if (!red_channel_client_test_remote_cap(client->channel_client, cap)) {
 return TRUE;
@@ -653,7 +654,8 @@ static int snd_send_mute(SndChannelClient *client, uint32_t 
cap, int msg)
 SpiceMsgAudioMute mute;
 RedChannelClient *rcc = client->channel_client;
 SpiceMarshaller *m = red_channel_client_get_marshaller(rcc);
-SpiceVolumeState *st = &client->channel->volume;
+SndChannel *channel = SND_CHANNEL(red_channel_client_get_channel(rcc));
+SpiceVolumeState *st = &channel->volume;
 
 if (!red_channel_client_test_remote_cap(client->channel_client, cap)) {
 return TRUE;
@@ -702,7 +704,7 @@ static int snd_playback_send_start(PlaybackChannelClient 
*playback_client)
 }
 
 start.channels = SPICE_INTERFACE_PLAYBACK_CHAN;
-start.frequency = client->channel->frequency;
+start.frequency = 
SND_CHANNEL(red_channel_client_get_channel(rcc))->frequency;
 spice_assert(SPICE_INTERFACE_PLAYBACK_FMT == 
SPICE_INTERFACE_AUDIO_FMT_S16);
 start.format = SPICE_AUDIO_FMT_S16;
 start.time = reds_get_mm_time();
@@ -745,7 +747,7 @@ static int snd_record_send_start(RecordChannelClient 
*record_client)
 }
 
 start.channels = SPICE_INTERFACE_RECORD_CHAN;
-start.frequency = client->channel->frequency;
+start.frequency = 
SND_CHANNEL(red_channel_client_get_channel(rcc))->frequency;
 spice_assert(SPICE_INTERFACE_RECORD_FMT == SPICE_INTERFACE_AUDIO_FMT_S16);
 start.format = SPICE_AUDIO_FMT_S16;
 spice_marshall_msg_record_start(m, &start);
@@ -970,7 +972,6 @@ static SndChannelClient *__new_channel(SndChannel *channel, 
int size, uint32_t c
 client->refs = 1;
 client->parser = spice_get_client_channel_parser(channel_id, NULL);
 client->stream = stream;
-client->channel = channel;
 client->receive_data.message_start = client->receive_data.buf;
 client->receive_data.now = client->receive_data.buf;
 client->receive_data.end = client->receive_data.buf + 
sizeof(client->receive_data.buf);
@@ -1474

[Spice-devel] [spice-server v3 00/10] Convert SndChannelClient to GObject

[Christophe Fergeau]

Hey,

This series is an attempt at splitting "sound: Convert SndChannelClient to 
GObject"
https://lists.freedesktop.org/archives/spice-devel/2017-January/034773.html
in much smaller chunks to ease reviews, and make git history more usable.
This might be a bit artificial at times (especially the conversion to GObject
before converting to RedChannelClient), but in my opinion most of the patches
make sense on their own.
The series should build and audio should still be functional with each of these
patches, though I've only tested playback, not recording.

In v2, I've moved "sound: Prefer snd_set_command()  over
snd_*_send_*() " after the conversion to sending data using RedChannelClient,
I've added error handling to the calls to snd_channel_config_socket().

In v3, I've added logs to the commit that were missing them based on Frediano's
input. I've removed an invalid reds_stream_free() call in
"sound: Implement snd_channel_config_socket". I haven't merged (yet?) the last
2 patches, but this is easy enough to do at any time.

Christophe

Christophe Fergeau (10):
  sound: Rework spice_server_playback_get_buffer() error handling
  sound: Implement snd_channel_config_socket
  sound: Add sanity checks in snd_{playback,record}_send
  sound: Remove SndChannelClient::send_data::marshaller
  sound: Remove SndChannelClient::channel
  sound: Remove code from spice_server_record_get_samples()
  sound: Use RedChannelClient to receive/send data
  sound: Prefer snd_set_command() over snd_*_send_*()
  sound: Turn {Playback,Record}ChannelClient into GObjects
  sound: Convert SndChannelClient to RedChannelClient

 server/Makefile.am|   2 -
 server/dummy-channel-client.c | 138 --
 server/dummy-channel-client.h |  64 ---
 server/sound.c| 963 ++
 4 files changed, 422 insertions(+), 745 deletions(-)
 delete mode 100644 server/dummy-channel-client.c
 delete mode 100644 server/dummy-channel-client.h

-- 
2.9.3

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

[Spice-devel] [spice-server v3 02/10] sound: Implement snd_channel_config_socket

[Christophe Fergeau]

This is in preparation for switching SndChannelClient into a proper
RedChannelClient. The prototype of the new helper matches what is
expected from the RedChannel::config_socket vfunc.

To be able to achieve that, this commit associates the sound channel
RedsStream instance with the DummyChannelClient instance we have, and
then call snd_channel_config_socket() on that instance.

Based on a patch from Frediano Ziglio 

Signed-off-by: Christophe Fergeau 
---
 server/dummy-channel-client.c |  2 +
 server/dummy-channel-client.h |  1 +
 server/sound.c| 99 +++
 3 files changed, 55 insertions(+), 47 deletions(-)

diff --git a/server/dummy-channel-client.c b/server/dummy-channel-client.c
index 61d5683..4efaa31 100644
--- a/server/dummy-channel-client.c
+++ b/server/dummy-channel-client.c
@@ -104,6 +104,7 @@ dummy_channel_client_init(DummyChannelClient *self)
 
 RedChannelClient* dummy_channel_client_create(RedChannel *channel,
   RedClient  *client,
+  RedsStream *stream,
   int num_common_caps,
   uint32_t *common_caps,
   int num_caps, uint32_t *caps)
@@ -125,6 +126,7 @@ RedChannelClient* dummy_channel_client_create(RedChannel 
*channel,
  NULL, NULL,
  "channel", channel,
  "client", client,
+ "stream", stream,
  "caps", caps_array,
  "common-caps", common_caps_array,
  NULL);
diff --git a/server/dummy-channel-client.h b/server/dummy-channel-client.h
index 8013aa2..54e0e6c 100644
--- a/server/dummy-channel-client.h
+++ b/server/dummy-channel-client.h
@@ -56,6 +56,7 @@ GType dummy_channel_client_get_type(void) G_GNUC_CONST;
 
 RedChannelClient *dummy_channel_client_create(RedChannel *channel,
   RedClient  *client,
+  RedsStream *stream,
   int num_common_caps, uint32_t 
*common_caps,
   int num_caps, uint32_t *caps);
 
diff --git a/server/sound.c b/server/sound.c
index 2165644..e04f1e7 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -938,6 +938,8 @@ static void snd_record_send(void* data)
 }
 }
 
+static int snd_channel_config_socket(RedChannelClient *rcc);
+
 static SndChannelClient *__new_channel(SndChannel *channel, int size, uint32_t 
channel_id,
RedClient *red_client,
RedsStream *stream,
@@ -949,50 +951,8 @@ static SndChannelClient *__new_channel(SndChannel 
*channel, int size, uint32_t c
uint32_t *caps, int num_caps)
 {
 SndChannelClient *client;
-int delay_val;
-int flags;
-#ifdef SO_PRIORITY
-int priority;
-#endif
-int tos;
-MainChannelClient *mcc = red_client_get_main(red_client);
 RedsState *reds = red_channel_get_server(RED_CHANNEL(channel));
 
-spice_assert(stream);
-if ((flags = fcntl(stream->socket, F_GETFL)) == -1) {
-spice_printerr("accept failed, %s", strerror(errno));
-goto error1;
-}
-
-#ifdef SO_PRIORITY
-priority = 6;
-if (setsockopt(stream->socket, SOL_SOCKET, SO_PRIORITY, (void*)&priority,
-   sizeof(priority)) == -1) {
-if (errno != ENOTSUP) {
-spice_printerr("setsockopt failed, %s", strerror(errno));
-}
-}
-#endif
-
-tos = IPTOS_LOWDELAY;
-if (setsockopt(stream->socket, IPPROTO_IP, IP_TOS, (void*)&tos, 
sizeof(tos)) == -1) {
-if (errno != ENOTSUP) {
-spice_printerr("setsockopt failed, %s", strerror(errno));
-}
-}
-
-delay_val = main_channel_client_is_low_bandwidth(mcc) ? 0 : 1;
-if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY, &delay_val, 
sizeof(delay_val)) == -1) {
-if (errno != ENOTSUP) {
-spice_printerr("setsockopt failed, %s", strerror(errno));
-}
-}
-
-if (fcntl(stream->socket, F_SETFL, flags | O_NONBLOCK) == -1) {
-spice_printerr("accept failed, %s", strerror(errno));
-goto error1;
-}
-
 spice_assert(size >= sizeof(*client));
 client = spice_malloc0(size);
 client->refs = 1;
@@ -1017,24 +977,69 @@ static SndChannelClient *__new_channel(SndChannel 
*channel, int size, uint32_t c
 client->cleanup = cleanup;
 
 client->channel_client =
-dummy_channel_client_create(RED_CHANNEL(channel), red_client,
+dummy_channel_client_create(RED_CHANNEL(channel), red_client, stream,
 num_common_caps, common_caps, num_caps, 
caps);
 if (!client->channel_client) {

[Spice-devel] [PATCH spice-server 4/6] spicevmc: Remove leak of RedPortInitPipeItem::name

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/spicevmc.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/server/spicevmc.c b/server/spicevmc.c
index 23e60b3..f441822 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -376,13 +376,21 @@ static void 
spicevmc_chardev_send_msg_to_client(RedCharDevice *self,
 red_channel_client_pipe_add_push(channel->rcc, msg);
 }
 
+static void red_port_init_item_free(struct RedPipeItem *base)
+{
+RedPortInitPipeItem *item = SPICE_UPCAST(RedPortInitPipeItem, base);
+
+free(item->name);
+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);
 
-red_pipe_item_init(&item->base, RED_PIPE_ITEM_TYPE_PORT_INIT);
+red_pipe_item_init_full(&item->base, RED_PIPE_ITEM_TYPE_PORT_INIT, 
red_port_init_item_free);
 item->name = strdup(sin->portname);
 item->opened = channel->port_opened;
 red_channel_client_pipe_add_push(rcc, &item->base);
-- 
2.9.3

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

[Spice-devel] [PATCH spice-server 5/6] spicevmc: Remove useless check

[Frediano Ziglio]

rcc is already deferenced in red_channel_client_get_client so
checking for NULL after that is uselss.

Signed-off-by: Frediano Ziglio 
---
 server/spicevmc.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/server/spicevmc.c b/server/spicevmc.c
index f441822..2039a80 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -452,10 +452,6 @@ static void 
spicevmc_red_channel_client_on_disconnect(RedChannelClient *rcc)
 SpiceCharDeviceInterface *sif;
 RedClient *client = red_channel_client_get_client(rcc);
 
-if (!rcc) {
-return;
-}
-
 channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
 
 /* partial message which wasn't pushed to device */
-- 
2.9.3

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

[Spice-devel] [PATCH spice-server 6/6] spicevmc: Reduce number of last saved IDs

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/spicevmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server/spicevmc.c b/server/spicevmc.c
index 2039a80..db2dd99 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -224,7 +224,7 @@ red_vmc_channel_finalize(GObject *object)
 static RedVmcChannel *red_vmc_channel_new(RedsState *reds, uint8_t 
channel_type)
 {
 GType gtype = G_TYPE_NONE;
-static uint8_t id[256] = { 0, };
+static uint8_t id[SPICE_END_CHANNEL] = { 0, };
 
 switch (channel_type) {
 case SPICE_CHANNEL_USBREDIR:
-- 
2.9.3

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

[Spice-devel] [PATCH spice-server 2/6] spicevmc: Avoid useless pointer cast

[Frediano Ziglio]

red_channel_client_handle_message already accepts a void* pointer.

Signed-off-by: Frediano Ziglio 
---
 server/spicevmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server/spicevmc.c b/server/spicevmc.c
index 0dc2b19..a92ec7f 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -574,7 +574,7 @@ static int 
spicevmc_red_channel_client_handle_message_parsed(RedChannelClient *r
 sif->event(channel->chardev_sin, *(uint8_t*)msg);
 break;
 default:
-return red_channel_client_handle_message(rcc, size, type, 
(uint8_t*)msg);
+return red_channel_client_handle_message(rcc, size, type, msg);
 }
 
 return TRUE;
-- 
2.9.3

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

[Spice-devel] [PATCH spice-server 1/6] spicevmc: Use spice_new instead of spice_malloc

[Frediano Ziglio]

spice_new is a bit more safe as return a properly typed pointer.

Signed-off-by: Frediano Ziglio 
---
 server/spicevmc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/server/spicevmc.c b/server/spicevmc.c
index bbe72b5..0dc2b19 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -380,7 +380,7 @@ 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_malloc(sizeof(RedPortInitPipeItem));
+RedPortInitPipeItem *item = spice_new(RedPortInitPipeItem, 1);
 
 red_pipe_item_init(&item->base, RED_PIPE_ITEM_TYPE_PORT_INIT);
 item->name = strdup(sin->portname);
@@ -390,7 +390,7 @@ static void spicevmc_port_send_init(RedChannelClient *rcc)
 
 static void spicevmc_port_send_event(RedChannelClient *rcc, uint8_t event)
 {
-RedPortEventPipeItem *item = spice_malloc(sizeof(RedPortEventPipeItem));
+RedPortEventPipeItem *item = spice_new(RedPortEventPipeItem, 1);
 
 red_pipe_item_init(&item->base, RED_PIPE_ITEM_TYPE_PORT_EVENT);
 item->event = event;
-- 
2.9.3

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

[Spice-devel] [PATCH spice-server 3/6] spicevmc: Avoid computing some variable value if not necessary

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/spicevmc.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/server/spicevmc.c b/server/spicevmc.c
index a92ec7f..23e60b3 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -585,12 +585,13 @@ static uint8_t 
*spicevmc_red_channel_alloc_msg_rcv_buf(RedChannelClient *rcc,
uint32_t size)
 {
 RedVmcChannel *channel;
-RedClient *client = red_channel_client_get_client(rcc);
-
-channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
+RedClient *client;
 
 switch (type) {
 case SPICE_MSGC_SPICEVMC_DATA:
+client = red_channel_client_get_client(rcc);
+channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
+
 assert(!channel->recv_from_client_buf);
 
 channel->recv_from_client_buf = 
red_char_device_write_buffer_get(channel->chardev,
@@ -615,10 +616,9 @@ static void 
spicevmc_red_channel_release_msg_rcv_buf(RedChannelClient *rcc,
 {
 RedVmcChannel *channel;
 
-channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
-
 switch (type) {
 case SPICE_MSGC_SPICEVMC_DATA:
+channel = RED_VMC_CHANNEL(red_channel_client_get_channel(rcc));
 /* buffer wasn't pushed to device */
 red_char_device_write_buffer_release(channel->chardev, 
&channel->recv_from_client_buf);
 break;
-- 
2.9.3

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

Re: [Spice-devel] [PATCH spice-server v2] gitignore: Reduce html files exclusion

[Frediano Ziglio]

ping

> 
> Limit the html files ignored.
> Can happen that you are working on some html files on your main
> spice-server directory and it's not desirable to ignore them.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  .gitignore | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> Changes since v1:
> - remove redundant ignore;
> - move exclusion to get better order.
> 
> diff --git a/.gitignore b/.gitignore
> index cac10f9..85f922b 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -4,7 +4,6 @@
>  *.tar.bz2
>  *.tar.gz
>  *.pyc
> -*.html
>  aclocal.m4
>  autom4te.cache
>  compile
> @@ -34,7 +33,7 @@ INSTALL
>  .version
>  .tarball-version
>  docs/manual/manual.chunked/
> -docs/manual/manual.html
> +docs/**/*.html
>  .dirstamp
>  .deps
>  .libs

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

Re: [Spice-devel] [PATCH] RFC: Document display channel drawable trees, etc

[Frediano Ziglio]

> 
> ---
> I started looking into a bug related to surfaces and drawing, but after a
> little initial investigation, I realized that I didn't really understand any
> of
> the code in this part of spice server. I discussed it a little bit with
> several
> others who also didn't have a good understanding of this part of the code. So
> I
> decided it needed to be documented so that we can understand it and work on
> it
> effectively. I've spent several long days trying to understand the details
> and
> trying to document them as well as I can. There is a lot of confusing
> terminology, and there are things I still don't fully understand, but I
> wanted
> to send this out to get feedback from others. Is it helpful? Do you see any
> cases where I misinterpreted things? Can anybody solve any pieces of the
> puzzle
> that I didn't quite figure out? etc. etc.
> 
> I know it's rather dense and will probably require others to spend some
> serious
> time understanding the code, but I'd love to get some feedback on it.
> 

Take some time to find this post, so before get really buried on the ML...

Some part are quite good and should be split and merged, other,
particularly exclude_region have too much questions to solve.

>  server/display-channel-private.h |   5 +-
>  server/display-channel.c | 256
>  +++
>  server/display-channel.h |  10 +-
>  server/tree.c|  12 +-
>  server/tree.h|   6 +
>  5 files changed, 285 insertions(+), 4 deletions(-)
> 
> diff --git a/server/display-channel-private.h
> b/server/display-channel-private.h
> index 6e299b9..dc4d605 100644
> --- a/server/display-channel-private.h
> +++ b/server/display-channel-private.h
> @@ -32,7 +32,10 @@ struct DisplayChannelPrivate
>  int enable_jpeg;
>  int enable_zlib_glz_wrap;
>  
> -Ring current_list; /* Drawable */
> +/* A ring of pending drawables for this DisplayChannel, regardless of 
> which
> + * surface they're associated with. This list is mainly used to flush 
> older
> + * drawables when we need to make room for new drawables. */
> +Ring current_list;

is this list containing ALL drawables allocated ?
Could be renamed to drawable_list ?

Wondering what these "current" are...

>  uint32_t current_size;
>  
>  uint32_t drawable_count;
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 6069883..83e76c1 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -393,6 +393,8 @@ static void current_add_drawable(DisplayChannel *display,
>  drawable->refs++;
>  }
>  
> +/* Unrefs the drawable and removes it from any rings that it's in, as well as
> + * removing any associated shadow item */
>  static void current_remove_drawable(DisplayChannel *display, Drawable *item)

display_channel_remove_drawable ?
display_channel_unlink_drawable ?

>  {
>  /* todo: move all to unref? */
> @@ -422,6 +424,7 @@ static void drawable_remove_from_pipes(Drawable
> *drawable)
>  }
>  }
>  
> +/* This function should never be called for Shadow TreeItems */
>  static void current_remove(DisplayChannel *display, TreeItem *item)
>  {
>  TreeItem *now = item;
> @@ -439,22 +442,38 @@ static void current_remove(DisplayChannel *display,
> TreeItem *item)
>  } else {
>  Container *now_as_container = CONTAINER(now);
>  
> +/* This is a questionable assert. It relies several assumptions
> + * to ensure that this function is never called for a
> + * TREE_ITEM_TYPE_SHADOW item.  */
>  spice_assert(now->type == TREE_ITEM_TYPE_CONTAINER);
>  
>  if ((ring_item = ring_get_head(&now_as_container->items))) {
> +/* descend into the container's child ring and continue
> + * iterating and removing those children */
>  now = SPICE_CONTAINEROF(ring_item, TreeItem, siblings_link);
>  continue;
>  }
> +/* This item is a container but it has no children, so reset our
> + * iterator to the item's previous sibling and free this empty
> + * container */

Why a container without child? Maybe the result of other
processing?

>  ring_item = now->siblings_link.prev;
>  container_free(now_as_container);
>  }
>  if (now == item) {
> +/* This is true if the initial @item was a DRAWABLE, or if @item
> + * was a container and we've finished iterating over all of that
> + * container's children and returned back up to the parent and
> + * freed it (see below) */
>  return;
>  }
>  
> +/* Increment the iterator to the next sibling. Note that if an item 
> was
> + * removed above, ring_item will have been reset to the item before 
> the
> + * item that was removed */
>  if ((ring_item = ring_next(&cont

Re: [Spice-devel] [nsis] Fix driver path for win2k*

[Uri Lublin]

On 01/26/2017 09:47 AM, Christophe Fergeau wrote:

On the virtio-win ISO, the win2k* drivers are in a path of the form
'2k*', while the installer is looking for them in 'w2k*', and thus
failing.
This commit fixes the various paths

This should fix https://bugzilla.redhat.com/show_bug.cgi?id=1416533 and
https://bugzilla.redhat.com/show_bug.cgi?id=1416579


Hi Christophe,

Looks good to me.
Ack.

Thanks,
Uri.


---
 win-guest-tools.nsis | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/win-guest-tools.nsis b/win-guest-tools.nsis
index 5f212f7..288e8a3 100644
--- a/win-guest-tools.nsis
+++ b/win-guest-tools.nsis
@@ -288,17 +288,17 @@ Function GetDriverSubdir
${ElseIf} ${IsWin7}
   StrCpy $0 "$0\w7"
${ElseIf} ${IsWin2008}
-  StrCpy $0 "$0\w2k8"
+  StrCpy $0 "$0\2k8"
${ElseIf} ${IsWin2008R2}
-  StrCpy $0 "$0\w2k8R2"
+  StrCpy $0 "$0\2k8R2"
${ElseIf} ${IsWin8}
   StrCpy $0 "$0\w8"
${ElseIf} ${IsWin2012}
-  StrCpy $0 "$0\w2k12"
+  StrCpy $0 "$0\2k12"
${ElseIf} ${IsWin8.1}
   StrCpy $0 "$0\w8.1"
${ElseIf} ${IsWin2012R2}
-  StrCpy $0 "$0\w2k12r2"
+  StrCpy $0 "$0\2k12r2"
${ElseIf} ${IsWin10}
   StrCpy $0 "$0\w10"
${Else}



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

Re: [Spice-devel] [PATCH v6 1/5] Quiet uninitialized variable warning.

[Christophe Fergeau]

On Thu, Jan 26, 2017 at 03:05:18AM -0500, Frediano Ziglio wrote:
> > 
> > Signed-off-by: Michal Suchanek 
> > ---
> > v2:
> >  - use g_return_if_reached
> > ---
> >  src/vdagentd/vdagentd.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> > index a1faf23..e2d6159 100644
> > --- a/src/vdagentd/vdagentd.c
> > +++ b/src/vdagentd/vdagentd.c
> > @@ -305,6 +305,8 @@ static void do_client_file_xfer(struct
> > vdagent_virtio_port *vport,
> >  id = d->id;
> >  break;
> >  }
> > +default:
> > +g_return_if_reached();
> >  }
> >  
> >  conn = g_hash_table_lookup(active_xfers, GUINT_TO_POINTER(id));
> 
> This patch got different comments. Ack or nack?

At the very least, I'd add
https://lists.freedesktop.org/archives/spice-devel/2017-January/034864.html
to the log. Still a bit reluctant to add this as this is not happening
to me on gcc 6, but my gcc is not warning either if do_client_file_xfer()
is not handling all 3 values that it can be called with, so should not
hurt to have this.

Christophe


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

Re: [Spice-devel] [PATCH v6 1/5] Quiet uninitialized variable warning.

[Frediano Ziglio]

> 
> Signed-off-by: Michal Suchanek 
> ---
> v2:
>  - use g_return_if_reached
> ---
>  src/vdagentd/vdagentd.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> index a1faf23..e2d6159 100644
> --- a/src/vdagentd/vdagentd.c
> +++ b/src/vdagentd/vdagentd.c
> @@ -305,6 +305,8 @@ static void do_client_file_xfer(struct
> vdagent_virtio_port *vport,
>  id = d->id;
>  break;
>  }
> +default:
> +g_return_if_reached();
>  }
>  
>  conn = g_hash_table_lookup(active_xfers, GUINT_TO_POINTER(id));

This patch got different comments. Ack or nack?

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