[Spice-devel] [client] gtk: Temporarily ignore the keyboard/mouse grabbing deprecation warnings

[Francois Gouget]

Note that the *_IGNORE_DEPRECATIONS macros are treated as separate 
statements by the compiler so they need to be put in a proper code block 
where appropriate.

Signed-off-by: Francois Gouget 
---


 src/spice-widget.c | 27 ++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/src/spice-widget.c b/src/spice-widget.c
index 82adacf..72fbbc8 100644
--- a/src/spice-widget.c
+++ b/src/spice-widget.c
@@ -812,6 +812,9 @@ SpiceGrabSequence *spice_display_get_grab_keys(SpiceDisplay 
*display)
 return d->grabseq;
 }
 
+/* FIXME: gdk_keyboard_grab/ungrab() is deprecated */
+G_GNUC_BEGIN_IGNORE_DEPRECATIONS
+
 static void try_keyboard_grab(SpiceDisplay *display)
 {
 GtkWidget *widget = GTK_WIDGET(display);
@@ -878,6 +881,8 @@ static void try_keyboard_ungrab(SpiceDisplay *display)
 d->keyboard_grab_active = false;
 g_signal_emit(widget, signals[SPICE_DISPLAY_KEYBOARD_GRAB], 0, false);
 }
+G_GNUC_END_IGNORE_DEPRECATIONS
+
 
 static void update_keyboard_grab(SpiceDisplay *display)
 {
@@ -1002,6 +1007,8 @@ static gboolean do_pointer_grab(SpiceDisplay *display)
  * what window the pointer is actally over, so use 'FALSE' for
  * 'owner_events' parameter
  */
+/* FIXME: gdk_pointer_grab() is deprecated */
+G_GNUC_BEGIN_IGNORE_DEPRECATIONS
 status = gdk_pointer_grab(window, FALSE,
  GDK_POINTER_MOTION_MASK |
  GDK_BUTTON_PRESS_MASK |
@@ -1011,6 +1018,7 @@ static gboolean do_pointer_grab(SpiceDisplay *display)
  NULL,
  blank,
  GDK_CURRENT_TIME);
+G_GNUC_END_IGNORE_DEPRECATIONS
 grab_successful = (status == GDK_GRAB_SUCCESS);
 if (!grab_successful) {
 d->mouse_grab_active = false;
@@ -1101,8 +1109,11 @@ static void mouse_wrap(SpiceDisplay *display, 
GdkEventMotion *motion)
 /* FIXME: we try our best to ignore that next pointer move event.. */
 gdk_display_sync(gdk_screen_get_display(screen));
 
+/* FIXME: gdk_display_warp_pointer() is deprecated */
+G_GNUC_BEGIN_IGNORE_DEPRECATIONS
 gdk_display_warp_pointer(gtk_widget_get_display(GTK_WIDGET(display)),
  screen, xr, yr);
+G_GNUC_END_IGNORE_DEPRECATIONS
 d->mouse_last_x = -1;
 d->mouse_last_y = -1;
 }
@@ -1110,6 +1121,9 @@ static void mouse_wrap(SpiceDisplay *display, 
GdkEventMotion *motion)
 
 }
 
+/* FIXME: gdk_pointer_ungrab()/gdk_display_warp_pointer() are deprecated */
+G_GNUC_BEGIN_IGNORE_DEPRECATIONS
+
 static void try_mouse_ungrab(SpiceDisplay *display)
 {
 SpiceDisplayPrivate *d = display->priv;
@@ -1142,6 +1156,7 @@ static void try_mouse_ungrab(SpiceDisplay *display)
 g_signal_emit(display, signals[SPICE_DISPLAY_MOUSE_GRAB], 0, false);
 spice_gtk_session_set_pointer_grabbed(d->gtk_session, false);
 }
+G_GNUC_END_IGNORE_DEPRECATIONS
 
 static void update_mouse_grab(SpiceDisplay *display)
 {
@@ -1972,7 +1987,7 @@ static gboolean button_event(GtkWidget *widget, 
GdkEventButton *button)
 try_mouse_grab(display);
 return true;
 }
-} else
+} else {
 /* allow to drag and drop between windows/displays:
 
By default, X (and other window system) do a pointer grab
@@ -1985,7 +2000,11 @@ static gboolean button_event(GtkWidget *widget, 
GdkEventButton *button)
FIXME: should be multiple widget grab, but how?
or should know the position of the other widgets?
 */
+/* FIXME: gdk_pointer_ungrab() is deprecated */
+G_GNUC_BEGIN_IGNORE_DEPRECATIONS
 gdk_pointer_ungrab(GDK_CURRENT_TIME);
+G_GNUC_END_IGNORE_DEPRECATIONS
+}
 
 if (!d->inputs)
 return true;
@@ -2355,7 +2374,10 @@ static void update_mouse_mode(SpiceChannel *channel, 
gpointer data)
 
 if (window != NULL) {
 GdkModifierType modifiers;
+/* FIXME: gdk_window_get_pointer() is deprecated */
+G_GNUC_BEGIN_IGNORE_DEPRECATIONS
 gdk_window_get_pointer(window, NULL, NULL, );
+G_GNUC_END_IGNORE_DEPRECATIONS
 
 if (modifiers & SPICE_GDK_BUTTONS_MASK)
 try_mouse_grab(display);
@@ -2771,7 +2793,10 @@ static void gl_draw(SpiceDisplay *display,
 GtkWidget *gl = gtk_stack_get_child_by_name(d->stack, "gl-area");
 
 if (gtk_stack_get_visible_child(d->stack) == gl) {
+/* Ignore GLib's too-new warnings */
+G_GNUC_BEGIN_IGNORE_DEPRECATIONS
 gtk_gl_area_queue_render(GTK_GL_AREA(gl));
+G_GNUC_END_IGNORE_DEPRECATIONS
 d->egl.call_draw_done = TRUE;
 } else
 #endif
-- 
2.10.2
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [client v2 3/3] build-sys: Enable deprecation warnings instead of ignoring them entirely

[Francois Gouget]

On Wed, 23 Nov 2016, Christophe Fergeau wrote:

> On Wed, Nov 23, 2016 at 07:09:28AM +0100, Francois Gouget wrote:
> > For Spice-gtk most deprecation issues come from changes outside Spice
> > (GLib) and thus should not be treated as errors to not break
> > compilation for users who have newer third-party libraries.
> > However they must be visible otherwise Spice developers will not be
> > aware of them and thus will not fix them before breakage happens.
> 
> Also iirc -DXXX_VERSION_MIN_REQUIRED=yyy need deprecation warnings to be
> functional

Hmmm, I applied the patch below and I'm getting errors with or without 
this patchset.

diff --git a/configure.ac b/configure.ac
index f3e7f8d..4661e9f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -136,7 +136,7 @@ AS_IF([test "x$with_gtk" != "xno"],
   [AS_IF([test "x$os_win32" = "xyes"],
  [PKG_CHECK_MODULES(GTK, gtk+-3.0 >= $GTK_REQUIRED)],
  [PKG_CHECK_MODULES(GTK, gtk+-3.0 >= $GTK_REQUIRED epoxy)])]
-  [GTK_CFLAGS="$GTK_CFLAGS -DGDK_VERSION_MIN_REQUIRED=$GTK_ENCODED_VERSION 
\
+  [GTK_CFLAGS="$GTK_CFLAGS -DGDK_VERSION_MIN_REQUIRED=4 \

-DGDK_VERSION_MAX_ALLOWED=$GTK_ENCODED_VERSION"])
 SPICE_GTK_REQUIRES="${SPICE_GTK_REQUIRES} gtk+-3.0 >= $GTK_REQUIRED"
 


Btw, what's the status of this patchset?


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

Re: [Spice-devel] [v2 spice PATCH 0/3] 3 manual fixes

[Pavel Grunt]

Ack for all!

Thanks,
Pavel

On Wed, 2016-11-30 at 20:04 +0200, Uri Lublin wrote:
> The first two patches match the patches I sent against
> the spice-space-pages git repo (to be known as v1),
> and were already acked.
> 
> The third patch is new (kinda following patch 2/3).
> 
> Uri Lublin (3):
>   manual: smartcard: add id=ccid to qemu command line
>   manual: move usbdk to the client subsection
>   manual: usbredir: remove the note about usbclerk
> 
>  docs/manual/manual.txt | 10 +++---
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [v2 spice PATCH 3/3] manual: usbredir: remove the note about usbclerk

[Uri Lublin]

The usbdk driver is preferred over usbclerk.

Signed-off-by: Uri Lublin 
---
 docs/manual/manual.txt | 4 
 1 file changed, 4 deletions(-)

diff --git a/docs/manual/manual.txt b/docs/manual/manual.txt
index 7fe8049..20d2cc6 100644
--- a/docs/manual/manual.txt
+++ b/docs/manual/manual.txt
@@ -535,10 +535,6 @@ which are described when running remote-viewer with 
`--help-spice`.
 To get USB redirection working on Windows clients, you need to install
 http://www.spice-space.org/download/windows/usbdk/[UsbDk]
 
-[NOTE]
-You may need additional services running in the client, such as the
-Spice USB Clerk service on Windows.
-
 CAC smartcard redirection
 =
 
-- 
2.9.3

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

[Spice-devel] [v2 spice PATCH 2/3] manual: move usbdk to the client subsection

[Uri Lublin]

Also added "clients" to the sentence to make it clear
that usbdk is to be installed on the client machine.

Signed-off-by: Uri Lublin 
---
 docs/manual/manual.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/docs/manual/manual.txt b/docs/manual/manual.txt
index 1268b0e..7fe8049 100644
--- a/docs/manual/manual.txt
+++ b/docs/manual/manual.txt
@@ -532,6 +532,9 @@ remote-viewer, you can select which USB devices to redirect 
in
 established. There are also various command line redirection options
 which are described when running remote-viewer with `--help-spice`.
 
+To get USB redirection working on Windows clients, you need to install
+http://www.spice-space.org/download/windows/usbdk/[UsbDk]
+
 [NOTE]
 You may need additional services running in the client, such as the
 Spice USB Clerk service on Windows.
@@ -1023,9 +1026,6 @@ The recommended way of getting all the needed drivers 
installed is to
 use the all-in-one Spice guest tools installer which can be found on
 http://spice-space.org/download/windows/spice-guest-tools/[spice-space.org].
 
-To get USB redirection working on Windows, you need to install
-http://www.spice-space.org/download/windows/usbdk/[UsbDk]
-
 If you want to manually install them, the QXL driver can be downloaded
 from http://spice-space.org/download/windows/qxl/[this location] ,
 agent builds can be found
-- 
2.9.3

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

[Spice-devel] [v2 spice PATCH 1/3] manual: smartcard: add id=ccid to qemu command line

[Uri Lublin]

Add it to "-chardev spicevmc" option.

Without it I get:
qemu-system-x86_64: -chardev spicevmc,name=smartcard: chardev: no id specified

Signed-off-by: Uri Lublin 
---
 docs/manual/manual.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/manual/manual.txt b/docs/manual/manual.txt
index 9d394b6..1268b0e 100644
--- a/docs/manual/manual.txt
+++ b/docs/manual/manual.txt
@@ -576,7 +576,7 @@ With the qemu command line, you must add a USB CCID device, 
and a
 name "smartcard":
 
 [source,sh]
--device usb-ccid -chardev spicevmc,name=smartcard -device 
ccid-card-passthru,chardev=ccid
+-device usb-ccid -chardev spicevmc,name=smartcard,id=ccid -device 
ccid-card-passthru,chardev=ccid
 
 Client
 --
-- 
2.9.3

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

[Spice-devel] [v2 spice PATCH 0/3] 3 manual fixes

[Uri Lublin]

The first two patches match the patches I sent against
the spice-space-pages git repo (to be known as v1),
and were already acked.

The third patch is new (kinda following patch 2/3).

Uri Lublin (3):
  manual: smartcard: add id=ccid to qemu command line
  manual: move usbdk to the client subsection
  manual: usbredir: remove the note about usbclerk

 docs/manual/manual.txt | 10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

-- 
2.9.3

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

Re: [Spice-devel] [spice-gtk v2 2/2] channel-usbredir: prevent crash when calling without host

[Victor Toso]

Hi,

On Wed, Nov 30, 2016 at 06:45:45PM +0100, Pavel Grunt wrote:
> imho this should be in usbredir
>
> It is not nice when library crashes your program

I don't disagree. Libraries should use guards accordingly.

But it doesn't make this correct. We should have initialized
usbredirhost before calling this API, making this a spice-gtk mistake.

Either way is fine to me... maybe both :)

  toso

>
> Pavel
> 
> On Wed, 2016-11-30 at 18:36 +0100, Victor Toso wrote:
> > From: Victor Toso 
> > 
> > Similar fix was done in 5b252b0f499601bcf387c02a4dd35d27ed34c
> > 
> > We should log a critical message instead of crashing the
> > application.
> > Signed-off-by: Victor Toso 
> > ---
> >  src/channel-usbredir.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> > index 4837d68..392a35e 100644
> > --- a/src/channel-usbredir.c
> > +++ b/src/channel-usbredir.c
> > @@ -817,6 +817,8 @@ static void
> > spice_usbredir_channel_up(SpiceChannel *c)
> >  SpiceUsbredirChannel *channel = SPICE_USBREDIR_CHANNEL(c);
> >  SpiceUsbredirChannelPrivate *priv = channel->priv;
> >  
> > +g_return_if_fail(priv->host != NULL);
> > +
> >  /* Flush any pending writes */
> >  usbredirhost_write_guest_data(priv->host);
> >  }


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-gtk v1] channel-main: demote log level from warning to debug

[Victor Toso]

From: Victor Toso 

Failing to get playback or record volume async on startup is very
common making this warning too worrisome for users.

If the audio back-end does not cache the last volume used or if this
is the first time the application is launched, this message will be
seen.

Signed-off-by: Victor Toso 
---
 src/channel-main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/channel-main.c b/src/channel-main.c
index 72ca712..e632c8e 100644
--- a/src/channel-main.c
+++ b/src/channel-main.c
@@ -1171,7 +1171,7 @@ static void audio_playback_volume_info_cb(GObject 
*object, GAsyncResult *res, gp
   , );
 if (ret == FALSE || volume == NULL || nchannels == 0) {
 if (error != NULL) {
-g_warning("Failed to get playback async volume info: %s", 
error->message);
+spice_debug("Failed to get playback async volume info: %s", 
error->message);
 g_error_free(error);
 } else {
 SPICE_DEBUG("Failed to get playback async volume info");
@@ -1227,7 +1227,7 @@ static void audio_record_volume_info_cb(GObject *object, 
GAsyncResult *res, gpoi
 ret = spice_audio_get_record_volume_info_finish(audio, res, , 
, , );
 if (ret == FALSE || volume == NULL || nchannels == 0) {
 if (error != NULL) {
-g_warning("Failed to get record async volume info: %s", 
error->message);
+spice_debug("Failed to get record async volume info: %s", 
error->message);
 g_error_free(error);
 } else {
 SPICE_DEBUG("Failed to get record async volume info");
-- 
2.9.3

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

Re: [Spice-devel] [spice-gtk v2 2/2] channel-usbredir: prevent crash when calling without host

[Pavel Grunt]

imho this should be in usbredir

It is not nice when library crashes your program

Pavel

On Wed, 2016-11-30 at 18:36 +0100, Victor Toso wrote:
> From: Victor Toso 
> 
> Similar fix was done in 5b252b0f499601bcf387c02a4dd35d27ed34c
> 
> We should log a critical message instead of crashing the
> application.
> Signed-off-by: Victor Toso 
> ---
>  src/channel-usbredir.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> index 4837d68..392a35e 100644
> --- a/src/channel-usbredir.c
> +++ b/src/channel-usbredir.c
> @@ -817,6 +817,8 @@ static void
> spice_usbredir_channel_up(SpiceChannel *c)
>  SpiceUsbredirChannel *channel = SPICE_USBREDIR_CHANNEL(c);
>  SpiceUsbredirChannelPrivate *priv = channel->priv;
>  
> +g_return_if_fail(priv->host != NULL);
> +
>  /* Flush any pending writes */
>  usbredirhost_write_guest_data(priv->host);
>  }
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v2 14/19] Free QXL instances in spice_server_destroy

[Pavel Grunt]

On Fri, 2016-11-25 at 14:52 +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/reds.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/server/reds.c b/server/reds.c
> index 701aed6..553e7a8 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -3599,6 +3599,8 @@ SPICE_GNUC_VISIBLE void
> spice_server_destroy(SpiceServer *reds)
>  servers = g_list_remove(servers, reds);
>  pthread_mutex_unlock(_reds_lock);
>  
> +g_list_free_full(reds->qxl_instances,
> (GDestroyNotify)red_qxl_destroy);

Ah g_list_free_full is GLIB 2.28, we require 2.22

There is a few of them already, imo is ok to bump the required version
to 2.28 since it is in el6 
> +
>  if (reds->inputs_channel) {
>  reds_unregister_channel(reds, RED_CHANNEL(reds-
> >inputs_channel));
>  red_channel_destroy(RED_CHANNEL(reds->inputs_channel));
> @@ -3609,6 +3611,11 @@ SPICE_GNUC_VISIBLE void
> spice_server_destroy(SpiceServer *reds)
>  if (reds->mig_timer) {
>  reds_core_timer_remove(reds, reds->mig_timer);
>  }
> +
> +if (reds->main_dispatcher) {
> +g_object_unref(reds->main_dispatcher);
> +}
> +
>  reds_cleanup(reds);
>  #ifdef RED_STATISTICS
>  stat_file_free(reds->stat_file);
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [spice-gtk v2 1/2] channel-usbredir: Fix crash on channel-up

[Victor Toso]

From: Victor Toso 

SpiceSession does not initialize its SpiceUsbDeviceManager object on
startup that could lead to a race condition where channel-usbredir is
requested to flush data while it is uninitialized.

In a few places, spice_usb_device_manager_get() is called as in
usb-device-widget.c and spice-gtk-session.c but not used in
spicy-stats, making the tool to crash on startup.

 #0 in usbredirhost_write_guest_data (host=0x0) at 
usbredir/usbredirhost/usbredirhost.c:876
 #1 in spice_usbredir_channel_up (c=0x643830) at channel-usbredir.c:821
 #2 in spice_channel_up (channel=0x643830) at spice-channel.c:1238
 #3 in spice_channel_recv_auth (channel=0x643830) at spice-channel.c:1225
 #4 in spice_channel_coroutine (data=0x643830) at spice-channel.c:2580
 #5 in coroutine_trampoline (cc=0x642ec0) at coroutine_ucontext.c:63
 #6 in continuation_trampoline (i0=6565568, i1=0) at continuation.c:55

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1399838

Signed-off-by: Victor Toso 
Reported-by: Michael Cullen 
---
 src/spice-session.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/src/spice-session.c b/src/spice-session.c
index f900bd1..91e4f97 100644
--- a/src/spice-session.c
+++ b/src/spice-session.c
@@ -281,6 +281,7 @@ static void spice_session_init(SpiceSession *session)
 {
 SpiceSessionPrivate *s;
 gchar *channels;
+GError *err = NULL;
 
 SPICE_DEBUG("New session (compiled from package " PACKAGE_STRING ")");
 s = session->priv = SPICE_SESSION_GET_PRIVATE(session);
@@ -293,6 +294,12 @@ static void spice_session_init(SpiceSession *session)
 s->images = cache_image_new((GDestroyNotify)pixman_image_unref);
 s->glz_window = glz_decoder_window_new();
 update_proxy(session, NULL);
+
+spice_usb_device_manager_get(session, );
+if (err != NULL) {
+SPICE_DEBUG("Could not initialize SpiceUsbDeviceManager - %s", 
err->message);
+g_clear_error();
+}
 }
 
 static void
-- 
2.9.3

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

Re: [Spice-devel] [PATCH v2 08/19] Remove unused red_channel_get_first_socket

[Pavel Grunt]

On Fri, 2016-11-25 at 14:52 +, Frediano Ziglio wrote:
> This function assume there is only one client.
> Was used only by some obsolete functions.
> Avoid to use such function in the future.
> 
> Signed-off-by: Frediano Ziglio 
Acked-by: Pavel Grunt 
> ---
>  server/red-channel.c | 14 --
>  server/red-channel.h |  2 --
>  2 files changed, 16 deletions(-)
> 
> diff --git a/server/red-channel.c b/server/red-channel.c
> index 0936548..fb2c8c1 100644
> --- a/server/red-channel.c
> +++ b/server/red-channel.c
> @@ -611,20 +611,6 @@ int red_channel_any_blocked(RedChannel
> *channel)
>  return FALSE;
>  }
>  
> -int red_channel_get_first_socket(RedChannel *channel)
> -{
> -RedChannelClient *rcc;
> -RedsStream *stream;
> -
> -if (!channel || !channel->priv->clients) {
> -return -1;
> -}
> -rcc = g_list_nth_data(channel->priv->clients, 0);
> -stream = red_channel_client_get_stream(rcc);
> -
> -return stream->socket;
> -}
> -
>  int red_channel_no_item_being_sent(RedChannel *channel)
>  {
>  GListIter iter;
> diff --git a/server/red-channel.h b/server/red-channel.h
> index 0aa72c5..2c99139 100644
> --- a/server/red-channel.h
> +++ b/server/red-channel.h
> @@ -262,8 +262,6 @@ void red_channel_pipes_add_type(RedChannel
> *channel, int pipe_item_type);
>  
>  void red_channel_pipes_add_empty_msg(RedChannel *channel, int
> msg_type);
>  
> -int red_channel_get_first_socket(RedChannel *channel);
> -
>  /* return TRUE if all of the connected clients to this channel are
> blocked */
>  int red_channel_all_blocked(RedChannel *channel);
>  
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v2 06/19] Remove unused and obsolete main_channel_close

[Pavel Grunt]

On Fri, 2016-11-25 at 14:52 +, Frediano Ziglio wrote:
> This function wrongly close the first client.
> Wrongly as closing the file descriptor cause a dandling
> file descriptor in the object potentially leading
> to closing another file descriptor open later.
> 
> Signed-off-by: Frediano Ziglio 
Acked-by: Pavel Grunt 
> ---
>  server/main-channel.c | 10 --
>  server/main-channel.h |  1 -
>  2 files changed, 11 deletions(-)
> 
> diff --git a/server/main-channel.c b/server/main-channel.c
> index 6449c16..37b2f25 100644
> --- a/server/main-channel.c
> +++ b/server/main-channel.c
> @@ -318,16 +318,6 @@ int main_channel_getpeername(MainChannel
> *main_chan, struct sockaddr *sa, sockle
>  getpeername(red_channel_get_first_socket(RED_CHANNEL(main_c
> han)), sa, salen) : -1;
>  }
>  
> -// TODO: ? shouldn't it disconnect all clients? or shutdown all
> main_channels?
> -void main_channel_close(MainChannel *main_chan)
> -{
> -int socketfd;
> -
> -if (main_chan && (socketfd =
> red_channel_get_first_socket(RED_CHANNEL(main_chan))) != -1) {
> -close(socketfd);
> -}
> -}
> -
>  MainChannel* main_channel_new(RedsState *reds)
>  {
>  // TODO: set the migration flag of the channel
> diff --git a/server/main-channel.h b/server/main-channel.h
> index 19beb7c..cb53fb4 100644
> --- a/server/main-channel.h
> +++ b/server/main-channel.h
> @@ -61,7 +61,6 @@ RedClient
> *main_channel_get_client_by_link_id(MainChannel *main_chan, uint32_t
> l
>  MainChannelClient *main_channel_link(MainChannel *, RedClient
> *client,
>   RedsStream *stream, uint32_t link_id, int migration, int
> num_common_caps,
>   uint32_t *common_caps, int num_caps, uint32_t *caps);
> -void main_channel_close(MainChannel *main_chan); // not destroy,
> just socket close
>  void main_channel_push_mouse_mode(MainChannel *main_chan, int
> current_mode, int is_client_mouse_allowed);
>  void main_channel_push_agent_connected(MainChannel *main_chan);
>  void main_channel_push_agent_disconnected(MainChannel *main_chan);
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v2 04/19] Use red_channel_destroy to free main_channel

[Pavel Grunt]

On Fri, 2016-11-25 at 14:52 +, Frediano Ziglio wrote:
> This will close all clients and release the channel properly
> 
> Signed-off-by: Frediano Ziglio 
Acked-by: Pavel Grunt 
> ---
>  server/reds.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/server/reds.c b/server/reds.c
> index c5e84ec..1a812e4 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -3590,7 +3590,7 @@ SPICE_GNUC_VISIBLE void
> spice_server_destroy(SpiceServer *reds)
>  g_array_unref(reds->config->video_codecs);
>  free(reds->config);
>  if (reds->main_channel) {
> -main_channel_close(reds->main_channel);
> +red_channel_destroy(RED_CHANNEL(reds->main_channel));
>  }
>  reds_cleanup(reds);
>  
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v2 12/19] Support QXL remove on spice_server_remove_interface

[Pavel Grunt]

Could be called in replay ? Or is it called indirectly ?

Pavel

On Fri, 2016-11-25 at 14:52 +, Frediano Ziglio wrote:
Signed-off-by: Frediano Ziglio 
> ---
>  server/reds.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/server/reds.c b/server/reds.c
> index bc0cc01..05afb7c 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -3321,6 +3321,13 @@ SPICE_GNUC_VISIBLE int
> spice_server_remove_interface(SpiceBaseInstance *sin)
>  SpiceCharDeviceInstance *char_device =
> SPICE_CONTAINEROF(sin, SpiceCharDeviceInstance, base);
>  reds = red_char_device_get_server(char_device->st);
>  spice_server_char_device_remove_interface(reds, sin);
> +} else if (strcmp(interface->type, SPICE_INTERFACE_QXL) == 0) {
> +QXLInstance *qxl;
> +
> +qxl = SPICE_CONTAINEROF(sin, QXLInstance, base);
> +reds = red_qxl_get_server(qxl->st);
> +reds->qxl_instances = g_list_remove(reds->qxl_instances,
> qxl);
> +red_qxl_destroy(qxl);
>  } else {
>  spice_warning("VD_INTERFACE_REMOVING unsupported");
>  return -1;
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v2 17/19] Allows reds_core_timer_remove to accept NULL for timer

[Pavel Grunt]

On Fri, 2016-11-25 at 14:52 +, Frediano Ziglio wrote:
> Most of the times the check is done externally
> so moving inside the function reduce the code.
> This is similar to the way free(3) works.
> 
OK

> Signed-off-by: Frediano Ziglio 
> ---
>  server/char-device.c | 20 
>  server/inputs-channel.c  |  5 ++---
>  server/main-channel-client.c |  4 +---
>  server/reds.c|  4 +++-
>  4 files changed, 14 insertions(+), 19 deletions(-)
> 
> diff --git a/server/char-device.c b/server/char-device.c
> index 3b70066..45ce548 100644
> --- a/server/char-device.c
> +++ b/server/char-device.c
> @@ -176,10 +176,8 @@ static void
> red_char_device_client_free(RedCharDevice *dev,
>  {
>  GList *l, *next;
>  
> -if (dev_client->wait_for_tokens_timer) {
> -reds_core_timer_remove(dev->priv->reds, dev_client-
> >wait_for_tokens_timer);
> -dev_client->wait_for_tokens_timer = NULL;
> -}
> +reds_core_timer_remove(dev->priv->reds, dev_client-
> >wait_for_tokens_timer);
> +dev_client->wait_for_tokens_timer = NULL;
>  
>  g_queue_free_full(dev_client->send_queue,
> (GDestroyNotify)red_pipe_item_unref);
>  
> @@ -990,10 +988,9 @@ static void
> red_char_device_init_device_instance(RedCharDevice *self)
>  
>  g_return_if_fail(self->priv->reds);
>  
> -if (self->priv->write_to_dev_timer) {
> -reds_core_timer_remove(self->priv->reds, self->priv-
> >write_to_dev_timer);
> -self->priv->write_to_dev_timer = NULL;
> -}
> +reds_core_timer_remove(self->priv->reds, self->priv-
> >write_to_dev_timer);
> +self->priv->write_to_dev_timer = NULL;
> +
>  if (self->priv->sin == NULL) {
> return;
>  }
> @@ -1081,10 +1078,9 @@ red_char_device_finalize(GObject *object)
>  {
>  RedCharDevice *self = RED_CHAR_DEVICE(object);
>  
> -if (self->priv->write_to_dev_timer) {
> -reds_core_timer_remove(self->priv->reds, self->priv-
> >write_to_dev_timer);
> -self->priv->write_to_dev_timer = NULL;
> -}
> +reds_core_timer_remove(self->priv->reds, self->priv-
> >write_to_dev_timer);
> +self->priv->write_to_dev_timer = NULL;
> +
>  write_buffers_queue_free(>priv->write_queue);
>  write_buffers_queue_free(>priv->write_bufs_pool);
>  self->priv->cur_pool_size = 0;
> diff --git a/server/inputs-channel.c b/server/inputs-channel.c
> index 99c2888..bea0ddf 100644
> --- a/server/inputs-channel.c
> +++ b/server/inputs-channel.c
> @@ -625,9 +625,8 @@ inputs_channel_finalize(GObject *object)
>  InputsChannel *self = INPUTS_CHANNEL(object);
>  RedsState *reds = red_channel_get_server(RED_CHANNEL(self));
>  
> -if (self->key_modifiers_timer) {
> -reds_core_timer_remove(reds, self->key_modifiers_timer);
> -}
> +reds_core_timer_remove(reds, self->key_modifiers_timer);
> +
>  G_OBJECT_CLASS(inputs_channel_parent_class)->finalize(object);
>  }
>  
> diff --git a/server/main-channel-client.c b/server/main-channel-
> client.c
> index 576b31f..15e168d 100644
> --- a/server/main-channel-client.c
> +++ b/server/main-channel-client.c
> @@ -189,9 +189,7 @@ static void main_channel_client_finalize(GObject
> *object)
>  RedsState *reds =
>  red_channel_get_server(red_channel_client_get_channel(RED_C
> HANNEL_CLIENT(object)));
>  
> -if (self->priv->ping_timer) {
> -reds_core_timer_remove(reds, self->priv->ping_timer);
> -}
> +reds_core_timer_remove(reds, self->priv->ping_timer);
>  #endif
>  G_OBJECT_CLASS(main_channel_client_parent_class)-
> >finalize(object);
>  }
> diff --git a/server/reds.c b/server/reds.c
> index 553e7a8..28127e8 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -4214,7 +4214,9 @@ void reds_core_timer_remove(RedsState *reds,
> g_return_if_fail(reds != NULL);
> g_return_if_fail(reds->core.timer_remove != NULL);
>  
> -   return reds->core.timer_remove(>core, timer);
> +if (timer) {
> +reds->core.timer_remove(>core, timer);
> +}

I would return early before the g_return_if_fail checks - see fixup

Besides that - Ack

Pavel

>  }
>  
>  void reds_update_client_mouse_allowed(RedsState *reds)
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH spice] fixup! Allows reds_core_timer_remove to accept NULL for timer

[Pavel Grunt]

---
to really be able to clean without any warnings it is needed to return as early 
as possible
---
 server/reds.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/server/reds.c b/server/reds.c
index 28127e8..8c86ece 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -3608,9 +3608,7 @@ SPICE_GNUC_VISIBLE void spice_server_destroy(SpiceServer 
*reds)
 if (reds->main_channel) {
 red_channel_destroy(RED_CHANNEL(reds->main_channel));
 }
-if (reds->mig_timer) {
-reds_core_timer_remove(reds, reds->mig_timer);
-}
+reds_core_timer_remove(reds, reds->mig_timer);
 
 if (reds->main_dispatcher) {
 g_object_unref(reds->main_dispatcher);
@@ -4211,12 +4209,14 @@ void reds_core_timer_cancel(RedsState *reds,
 void reds_core_timer_remove(RedsState *reds,
 SpiceTimer *timer)
 {
-   g_return_if_fail(reds != NULL);
-   g_return_if_fail(reds->core.timer_remove != NULL);
-
-if (timer) {
-reds->core.timer_remove(>core, timer);
+if (timer == NULL) {
+return;
 }
+
+g_return_if_fail(reds != NULL);
+g_return_if_fail(reds->core.timer_remove != NULL);
+
+reds->core.timer_remove(>core, timer);
 }
 
 void reds_update_client_mouse_allowed(RedsState *reds)
-- 
2.10.2

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

Re: [Spice-devel] [PATCH v2 19/19] Free properly primary surface during replay

[Pavel Grunt]

On Fri, 2016-11-25 at 14:52 +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
Acked-by: Pavel Grunt 
> ---
>  server/red-replay-qxl.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
> index 2176068..aeaa545 100644
> --- a/server/red-replay-qxl.c
> +++ b/server/red-replay-qxl.c
> @@ -48,6 +48,7 @@ struct SpiceReplay {
>  GArray *id_map; // record id -> replay id
>  GArray *id_map_inv; // replay id -> record id
>  GArray *id_free; // free list
> +uint8_t *primary_mem;
>  int nsurfaces;
>  int end_pos;
>  
> @@ -1254,6 +1255,8 @@ static void
> replay_handle_create_primary(QXLWorker *worker, SpiceReplay *replay)
>  }
>  read_binary(replay, "data", , , 0);
>  surface.group_id = 0;
> +free(replay->primary_mem);
> +replay->primary_mem = mem;
>  surface.mem = QXLPHYSICAL_FROM_PTR(mem);
>  worker->create_primary_surface(worker, 0, );
>  }
> @@ -1269,6 +1272,8 @@ static void replay_handle_dev_input(QXLWorker
> *worker, SpiceReplay *replay,
>  case RED_WORKER_MESSAGE_DESTROY_PRIMARY_SURFACE:
>  replay->created_primary = FALSE;
>  worker->destroy_primary_surface(worker, 0);
> +free(replay->primary_mem);
> +replay->primary_mem = NULL;
>  break;
>  case RED_WORKER_MESSAGE_DESTROY_SURFACES:
>  replay->created_primary = FALSE;
> @@ -1454,6 +1459,7 @@ SPICE_GNUC_VISIBLE void
> spice_replay_free(SpiceReplay *replay)
>  g_array_free(replay->id_map, TRUE);
>  g_array_free(replay->id_map_inv, TRUE);
>  g_array_free(replay->id_free, TRUE);
> +free(replay->primary_mem);
>  fclose(replay->fd);
>  free(replay);
>  }
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v4 05/14] sound: Convert SndChannel to GObject

[Frediano Ziglio]

> 
> On Wed, Nov 30, 2016 at 12:34:49PM +, Frediano Ziglio wrote:
> > NOTE: this is not supposed to run!
> > 
> > Just make easy the review, should be squashed to following 2
> > patches.
> 
> This works reasonably well (I get sound in a VM) after adding the patch
> below.
> I'd keep it separate, and add a commit log (this adds the GObject
> boilerplate,
> and also stops using the dummy channel, however sending of data still goes
> through DummyChannelClient which is why are good with no implementation of
> some
> of the required vfuncs)
> 

Merged and tested. Both play & record work.

Frediano

> 
> diff --git a/server/sound.c b/server/sound.c
> index a7622be..e02e9dc 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -1017,6 +1017,30 @@ static void
> snd_disconnect_channel_client(RedChannelClient *rcc)
>  }
>  }
> 
> +static int snd_channel_config_socket(RedChannelClient *rcc)
> +{
> +g_assert_not_reached();
> +}
> +
> +static void snd_channel_on_disconnect(RedChannelClient *rcc)
> +{
> +g_assert_not_reached();
> +}
> +
> +static uint8_t*
> +snd_channel_client_alloc_recv_buf(RedChannelClient *rcc, uint16_t type,
> uint32_t size)
> +{
> +g_assert_not_reached();
> +}
> +
> +static void
> +snd_channel_client_release_recv_buf(RedChannelClient *rcc, uint16_t type,
> uint32_t size,
> +uint8_t *msg)
> +{
> +g_assert_not_reached();
> +}
> +
> +
>  static void snd_set_command(SndChannelClient *client, uint32_t command)
>  {
>  if (!client) {
> @@ -1543,6 +1567,12 @@ snd_channel_init(SndChannel *self)
>  static void
>  snd_channel_class_init(SndChannelClass *klass)
>  {
> +RedChannelClass *channel_class = RED_CHANNEL_CLASS(klass);
> +
> +channel_class->config_socket = snd_channel_config_socket;
> +channel_class->alloc_recv_buf = snd_channel_client_alloc_recv_buf;
> +channel_class->release_recv_buf = snd_channel_client_release_recv_buf;
> +channel_class->on_disconnect = snd_channel_on_disconnect;
>  }
> 
>  static void
> 
> 
> 
> 
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  server/sound.c | 195 +-
> >  1 file changed, 130 insertions(+), 65 deletions(-)
> > 
> > diff --git a/server/sound.c b/server/sound.c
> > index 5b66f3a..de46be5 100644
> > --- a/server/sound.c
> > +++ b/server/sound.c
> > @@ -144,9 +144,14 @@ typedef struct SpiceVolumeState {
> >  int mute;
> >  } SpiceVolumeState;
> >  
> > +#define TYPE_SND_CHANNEL snd_channel_get_type()
> > +#define SND_CHANNEL(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj),
> > TYPE_SND_CHANNEL, SndChannel))
> > +GType snd_channel_get_type(void) G_GNUC_CONST;
> > +
> >  /* Base class for SpicePlaybackState and SpiceRecordState */
> >  struct SndChannel {
> > -RedChannel *base_channel;
> > +RedChannel parent;
> > +
> >  SndChannelClient *connection; /* Only one client is supported */
> >  SndChannel *next; /* For the global SndChannel list */
> >  
> > @@ -155,14 +160,40 @@ struct SndChannel {
> >  uint32_t frequency;
> >  };
> >  
> > +typedef RedChannelClass SndChannelClass;
> > +
> > +G_DEFINE_TYPE(SndChannel, snd_channel, RED_TYPE_CHANNEL)
> > +
> > +
> > +typedef struct SpicePlaybackState PlaybackChannel;
> > +typedef SndChannelClass PlaybackChannelClass;
> > +
> > +#define TYPE_PLAYBACK_CHANNEL playback_channel_get_type()
> > +#define PLAYBACK_CHANNEL(obj) \
> > +(G_TYPE_CHECK_INSTANCE_CAST((obj), TYPE_PLAYBACK_CHANNEL,
> > PlaybackChannel))
> > +GType playback_channel_get_type(void) G_GNUC_CONST;
> > +
> >  struct SpicePlaybackState {
> > -struct SndChannel channel;
> > +SndChannel channel;
> >  };
> >  
> > +G_DEFINE_TYPE(PlaybackChannel, playback_channel, TYPE_SND_CHANNEL)
> > +
> > +
> > +typedef struct SpiceRecordState RecordChannel;
> > +typedef SndChannelClass RecordChannelClass;
> > +
> > +#define TYPE_RECORD_CHANNEL record_channel_get_type()
> > +#define RECORD_CHANNEL(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj),
> > TYPE_RECORD_CHANNEL, RecordChannel))
> > +GType record_channel_get_type(void) G_GNUC_CONST;
> > +
> >  struct SpiceRecordState {
> > -struct SndChannel channel;
> > +SndChannel channel;
> >  };
> >  
> > +G_DEFINE_TYPE(RecordChannel, record_channel, TYPE_SND_CHANNEL)
> > +
> > +
> >  typedef struct RecordChannelClient {
> >  SndChannelClient base;
> >  uint32_t samples[RECORD_SAMPLES_SIZE];
> > @@ -201,7 +232,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(client->channel->base_channel);
> > +return red_channel_get_server(RED_CHANNEL(client->channel));
> >  }
> >  
> >  static void snd_disconnect_channel(SndChannelClient *client)
> > @@ -892,7 +923,7 @@ static SndChannelClient *__new_channel(SndChannel
> > *channel, int size, uint32_t c
> >  

Re: [Spice-devel] [qxl 0/5] Various patches in preparation for 0.1.5 release

[Frediano Ziglio]

> 
> On Wed, Nov 30, 2016 at 03:43:06PM +0100, Christophe Fergeau wrote:
> > Still no volunteer? :)
> > Distros are upgrading to Xorg 1.19, and are having some issues because
> > we don't have an official release known to work with it..
> 
> On second thought, the only significant patch from me has been reviewed
> by Jeremy, and the other ones are either from Dave or trivial/doc, so
> I'll just go ahead with the release unless somebody objects.
> 
> Christophe
> 

Agreed.

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

Re: [Spice-devel] [qxl 3/5] Fix "calles" typo in comment

[Frediano Ziglio]

> 
> ---
>  src/spiceqxl_main_loop.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/spiceqxl_main_loop.c b/src/spiceqxl_main_loop.c
> index db89b6d..49b715a 100644
> --- a/src/spiceqxl_main_loop.c
> +++ b/src/spiceqxl_main_loop.c
> @@ -274,7 +274,7 @@ static void xspice_block_handler(pointer data, OSTimePtr
> timeout, pointer readma
>  }
>  
>  /*
> - * xserver only calles wakeup_handler with the read fd_set, so we
> + * xserver only calls wakeup_handler with the read fd_set, so we
>   * must either patch it or do a polling select ourselves, this is the
>   * later approach. Since we are already doing a polling select, we
>   * already select on all (we could avoid selecting on the read since

Acked

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

Re: [Spice-devel] [PATCH spice-server] Move some include from header to source

[Jonathon Jongsma]

Acked-by: Jonathon Jongsma 


On Mon, 2016-11-28 at 14:27 +, Frediano Ziglio wrote:
> Now that RedStatFile is private there is no need
> to include some headers in stat-file.h.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/stat-file.c | 2 ++
>  server/stat-file.h | 3 +--
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/server/stat-file.c b/server/stat-file.c
> index d1e25ce..57b6110 100644
> --- a/server/stat-file.c
> +++ b/server/stat-file.c
> @@ -25,7 +25,9 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> diff --git a/server/stat-file.h b/server/stat-file.h
> index 903b68b..0affb90 100644
> --- a/server/stat-file.h
> +++ b/server/stat-file.h
> @@ -18,8 +18,7 @@
>  #ifndef STAT_FILE_H_
>  #define STAT_FILE_H_
>  
> -#include 
> -#include 
> +#include 
>  
>  typedef uint32_t StatNodeRef;
>  #define INVALID_STAT_REF (~(StatNodeRef)0)
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [qxl 0/5] Various patches in preparation for 0.1.5 release

[Christophe Fergeau]

On Wed, Nov 30, 2016 at 03:43:06PM +0100, Christophe Fergeau wrote:
> Still no volunteer? :)
> Distros are upgrading to Xorg 1.19, and are having some issues because
> we don't have an official release known to work with it..

On second thought, the only significant patch from me has been reviewed
by Jeremy, and the other ones are either from Dave or trivial/doc, so
I'll just go ahead with the release unless somebody objects.

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] [qxl 0/5] Various patches in preparation for 0.1.5 release

[Christophe Fergeau]

Still no volunteer? :)
Distros are upgrading to Xorg 1.19, and are having some issues because
we don't have an official release known to work with it..

Christophe

On Wed, Nov 16, 2016 at 05:14:14PM +0100, Christophe Fergeau wrote:
> Any taker for reviewing this one?
> 
> Christophe
> 
> On Fri, Oct 28, 2016 at 12:18:20PM +0200, Christophe Fergeau wrote:
> > Hey,
> > 
> > It has been a long time since the last QXL Xorg driver release, would be 
> > nice
> > to have one soon :)
> > Given that Xspice is probably not working with Xorg 1.19, I've added a
> > configure.ac patch erroring out when both are in use.
> > 
> > I've also added two patches that we have been carrying for a long time in 
> > the
> > Fedora package. These disable surfaces with the KMS driver as surfaces have
> > never been working properly, and are causing graphical artifacts 
> > (disappearing
> > characters, ...) when they are used.
> > 
> > Christophe
> > 
> > ___
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel



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



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

[Spice-devel] [PATCH 2/2] Manual: add info about QEMU_AUDIO_DRV

[Jonathon Jongsma]

hramrach on IRC had an issue with audio not working. He was using
suse and his qemu defaulted to using the pulseaudio audio driver instead
of spice.  Explicitly setting the audio driver to spice using the
QEMU_AUDIO_DRV environment variable solved the problem. Add this tip to
the manual for others having the same issue in the future.
---

Fix typo found by Victor

 docs/manual/manual.txt | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/docs/manual/manual.txt b/docs/manual/manual.txt
index 8685715..fd23540 100644
--- a/docs/manual/manual.txt
+++ b/docs/manual/manual.txt
@@ -257,6 +257,12 @@ communication with the client. The Spice option 
`disable-ticketing` is
 specifying that ticketing (simple authentication method) is not
 used. The virtio and chardev devices are required by the guest agent.
 
+The `-soundhw hda` option provides an audio device for the guest to use for
+audio playback and recording. In order for spice audio to work properly, qemu
+must use the 'spice' audio driver. Depending on how qemu was built, however,
+this might not be the default audio driver. To ensure qemu uses the spice audio
+driver, you can set the environment variable `QEMU_AUDIO_DRV=spice`.
+
 Basic configuration
 ---
 
-- 
2.7.4

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

[Spice-devel] [PATCH 1/2] Switch example sound device to 'hda'

[Jonathon Jongsma]

Recommended sound device for newer operating systems is Intel HD audio
(hda). Use this in the commandline examples.
---

Not sure whether this needs to be a separate patch, but since it's technically 
unrelated...

 docs/manual/manual.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/manual/manual.txt b/docs/manual/manual.txt
index 9d394b6..8685715 100644
--- a/docs/manual/manual.txt
+++ b/docs/manual/manual.txt
@@ -241,7 +241,7 @@ favourite operating system so you can use it for 
installation.
 
 [source,sh]
 host$ qemu-kvm -boot order=dc -vga qxl \
--spice port=3001,disable-ticketing -soundhw ac97 \
+-spice port=3001,disable-ticketing -soundhw hda \
 -device virtio-serial -chardev 
spicevmc,id=vdagent,debug=0,name=vdagent \
 -device virtserialport,chardev=vdagent,name=com.redhat.spice.0 \
 -cdrom /path/to/your.iso /path/to/your.img
-- 
2.7.4

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

Re: [Spice-devel] [PATCH v4 05/14] sound: Convert SndChannel to GObject

[Christophe Fergeau]

On Wed, Nov 30, 2016 at 12:34:49PM +, Frediano Ziglio wrote:
> NOTE: this is not supposed to run!
> 
> Just make easy the review, should be squashed to following 2
> patches.

This works reasonably well (I get sound in a VM) after adding the patch below.
I'd keep it separate, and add a commit log (this adds the GObject boilerplate,
and also stops using the dummy channel, however sending of data still goes
through DummyChannelClient which is why are good with no implementation of some
of the required vfuncs)


diff --git a/server/sound.c b/server/sound.c
index a7622be..e02e9dc 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -1017,6 +1017,30 @@ static void 
snd_disconnect_channel_client(RedChannelClient *rcc)
 }
 }

+static int snd_channel_config_socket(RedChannelClient *rcc)
+{
+g_assert_not_reached();
+}
+
+static void snd_channel_on_disconnect(RedChannelClient *rcc)
+{
+g_assert_not_reached();
+}
+
+static uint8_t*
+snd_channel_client_alloc_recv_buf(RedChannelClient *rcc, uint16_t type, 
uint32_t size)
+{
+g_assert_not_reached();
+}
+
+static void
+snd_channel_client_release_recv_buf(RedChannelClient *rcc, uint16_t type, 
uint32_t size,
+uint8_t *msg)
+{
+g_assert_not_reached();
+}
+
+
 static void snd_set_command(SndChannelClient *client, uint32_t command)
 {
 if (!client) {
@@ -1543,6 +1567,12 @@ snd_channel_init(SndChannel *self)
 static void
 snd_channel_class_init(SndChannelClass *klass)
 {
+RedChannelClass *channel_class = RED_CHANNEL_CLASS(klass);
+
+channel_class->config_socket = snd_channel_config_socket;
+channel_class->alloc_recv_buf = snd_channel_client_alloc_recv_buf;
+channel_class->release_recv_buf = snd_channel_client_release_recv_buf;
+channel_class->on_disconnect = snd_channel_on_disconnect;
 }

 static void




> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/sound.c | 195 +-
>  1 file changed, 130 insertions(+), 65 deletions(-)
> 
> diff --git a/server/sound.c b/server/sound.c
> index 5b66f3a..de46be5 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -144,9 +144,14 @@ typedef struct SpiceVolumeState {
>  int mute;
>  } SpiceVolumeState;
>  
> +#define TYPE_SND_CHANNEL snd_channel_get_type()
> +#define SND_CHANNEL(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj), 
> TYPE_SND_CHANNEL, SndChannel))
> +GType snd_channel_get_type(void) G_GNUC_CONST;
> +
>  /* Base class for SpicePlaybackState and SpiceRecordState */
>  struct SndChannel {
> -RedChannel *base_channel;
> +RedChannel parent;
> +
>  SndChannelClient *connection; /* Only one client is supported */
>  SndChannel *next; /* For the global SndChannel list */
>  
> @@ -155,14 +160,40 @@ struct SndChannel {
>  uint32_t frequency;
>  };
>  
> +typedef RedChannelClass SndChannelClass;
> +
> +G_DEFINE_TYPE(SndChannel, snd_channel, RED_TYPE_CHANNEL)
> +
> +
> +typedef struct SpicePlaybackState PlaybackChannel;
> +typedef SndChannelClass PlaybackChannelClass;
> +
> +#define TYPE_PLAYBACK_CHANNEL playback_channel_get_type()
> +#define PLAYBACK_CHANNEL(obj) \
> +(G_TYPE_CHECK_INSTANCE_CAST((obj), TYPE_PLAYBACK_CHANNEL, 
> PlaybackChannel))
> +GType playback_channel_get_type(void) G_GNUC_CONST;
> +
>  struct SpicePlaybackState {
> -struct SndChannel channel;
> +SndChannel channel;
>  };
>  
> +G_DEFINE_TYPE(PlaybackChannel, playback_channel, TYPE_SND_CHANNEL)
> +
> +
> +typedef struct SpiceRecordState RecordChannel;
> +typedef SndChannelClass RecordChannelClass;
> +
> +#define TYPE_RECORD_CHANNEL record_channel_get_type()
> +#define RECORD_CHANNEL(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj), 
> TYPE_RECORD_CHANNEL, RecordChannel))
> +GType record_channel_get_type(void) G_GNUC_CONST;
> +
>  struct SpiceRecordState {
> -struct SndChannel channel;
> +SndChannel channel;
>  };
>  
> +G_DEFINE_TYPE(RecordChannel, record_channel, TYPE_SND_CHANNEL)
> +
> +
>  typedef struct RecordChannelClient {
>  SndChannelClient base;
>  uint32_t samples[RECORD_SAMPLES_SIZE];
> @@ -201,7 +232,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(client->channel->base_channel);
> +return red_channel_get_server(RED_CHANNEL(client->channel));
>  }
>  
>  static void snd_disconnect_channel(SndChannelClient *client)
> @@ -892,7 +923,7 @@ static SndChannelClient *__new_channel(SndChannel 
> *channel, int size, uint32_t c
>  #endif
>  int tos;
>  MainChannelClient *mcc = red_client_get_main(red_client);
> -RedsState *reds = red_channel_get_server(channel->base_channel);
> +RedsState *reds = red_channel_get_server(RED_CHANNEL(channel));
>  
>  spice_assert(stream);
>  if ((flags = fcntl(stream->socket, F_GETFL)) == -1) {
> @@ -953,7 +984,7 @@ static 

Re: [Spice-devel] [spice-gtk v1] channel-usbredir: Fix crash on channel-up

[Victor Toso]

Hi,

On Wed, Nov 30, 2016 at 03:33:13PM +0100, Victor Toso wrote:
> Hi,
>
> On Wed, Nov 30, 2016 at 11:20:42AM +0100, Christophe Fergeau wrote:
> > On Tue, Nov 29, 2016 at 10:46:24PM +0100, Victor Toso wrote:
> > > From: Victor Toso 
> > >
> > > Seems that usbredirhost initialization can take longer then
> > > the channel_up call from spice-channel.
> > >
> > > As this seems to be a race, a simple check for NULL pointer should be
> > > enough. Similar fix was done in 5b252b0f499601bcf387c02a4dd35d27ed34c
> >
> > Too many "seems" for my taste in this commit log when this mentions a
> > race condition :(
> 
> Ah, yeah, sorry about that. I wrote it as I was thinking about other
> possibilities too, leading the commit log to not be so certain about the
> issue.
> 
> > priv->host is initialized in spice_usbredir_channel_set_context() which
> > is called from SpiceUsbDeviceManager::channel_new. SpiceUsbDeviceManager
> > will only start listening for the "channel-new" signal in
> > spice_usb_device_manager_initable_init().
> >
> > I think what you are suggesting is that
> > spice_usb_device_manager_initable_init() is taking too long, and the
> > usbredir is up and running before we get a chance to set priv->host?
> 
> Hm, it is a bit too hard to say for sure what is slow without more debug
> info.
> 
> As you said, what does initialize priv->host is:
> 1-) channel-new [signal] -> channel_new() [usb-device-manager]
> 2-) spice_usbredir_channel_set_context()
> 3-) priv->host = usbredirhost_open_full(...)
> 
> As this did not happen at the time of the crash, yes, maybe
> spice_usb_device_manager_initable_init() failed or was not called
> which would mean that spice_usb_device_manager_get() was not called.
> 
> Not sure who should be calling spice_usb_device_manager_get() first?
> - src/usb-device-widget.c at spice_usb_device_widget_constructor()
> - src/spice-option.c  at spice_set_session_option()
> - src/spice-gtk-session.c at spice_gtk_session_request_auto_usbredir()
> 
> But I'm sure it can't be on src/channel-usbredir.c at
> spice_usbredir_channel_open_device() as this needs priv->host already :)
> 
> > NB: channel_up is missing from the backtrace but is what is called
> > between #0 and #1.
> 
> Indeed, I wonder why is it missing?
> 
> > Can we miss some data if we skip this flushing of pending writes? I
> > guess we cannot have any since priv->host does not exist yet?
> 
> I don't think we can miss anything if the device is not even open yet.
> Maybe fixing this crash can trigger a different issue later on,
> hopefully it would be easier to track.
> 
> Let me know if I should improve the commit log but after writing this
> email, the 'seems' are likely correct as I'm not 100% sure how this was
> triggered.
> 
> >
> > Christophe
> 
> Cheers,
>   toso

I missed a information on bugzilla, see:

Additional info:
(...)
  cmdline:spicy-stats --uri=spice://localhost:5900
  crash_function: usbredirhost_write_guest_data
  executable: /usr/bin/spicy-stats

That should narrow down how the bug was triggered...

> 
> > 
> > > 
> > >  #0 in usbredirhost_write_guest_data (host=0x0) at usbredirhost.c:876
> > >  #1 in spice_channel_recv_auth (channel=0x55af5f3b8f50 
> > > [SpiceUsbredirChannel]) at spice-channel.c:1225
> > >  #2 in spice_channel_coroutine (data=0x55af5f3b8f50) at 
> > > spice-channel.c:2580
> > >  #3 in coroutine_trampoline (cc=0x55af5f3b85e0) at coroutine_ucontext.c:63
> > > 
> > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1399838
> > > 
> > > Signed-off-by: Victor Toso 
> > > ---
> > >  src/channel-usbredir.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> > > index 4837d68..561f486 100644
> > > --- a/src/channel-usbredir.c
> > > +++ b/src/channel-usbredir.c
> > > @@ -817,6 +817,9 @@ static void spice_usbredir_channel_up(SpiceChannel *c)
> > >  SpiceUsbredirChannel *channel = SPICE_USBREDIR_CHANNEL(c);
> > >  SpiceUsbredirChannelPrivate *priv = channel->priv;
> > >  
> > > +if (priv->host == NULL)
> > > +  return;
> > > +
> > >  /* Flush any pending writes */
> > >  usbredirhost_write_guest_data(priv->host);
> > >  }
> > > -- 
> > > 2.9.3
> > > 
> > > ___
> > > 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



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] [spice-gtk v1] channel-usbredir: Fix crash on channel-up

[Victor Toso]

Hi,

On Wed, Nov 30, 2016 at 11:20:42AM +0100, Christophe Fergeau wrote:
> On Tue, Nov 29, 2016 at 10:46:24PM +0100, Victor Toso wrote:
> > From: Victor Toso 
> >
> > Seems that usbredirhost initialization can take longer then
> > the channel_up call from spice-channel.
> >
> > As this seems to be a race, a simple check for NULL pointer should be
> > enough. Similar fix was done in 5b252b0f499601bcf387c02a4dd35d27ed34c
>
> Too many "seems" for my taste in this commit log when this mentions a
> race condition :(

Ah, yeah, sorry about that. I wrote it as I was thinking about other
possibilities too, leading the commit log to not be so certain about the
issue.

> priv->host is initialized in spice_usbredir_channel_set_context() which
> is called from SpiceUsbDeviceManager::channel_new. SpiceUsbDeviceManager
> will only start listening for the "channel-new" signal in
> spice_usb_device_manager_initable_init().
>
> I think what you are suggesting is that
> spice_usb_device_manager_initable_init() is taking too long, and the
> usbredir is up and running before we get a chance to set priv->host?

Hm, it is a bit too hard to say for sure what is slow without more debug
info.

As you said, what does initialize priv->host is:
1-) channel-new [signal] -> channel_new() [usb-device-manager]
2-) spice_usbredir_channel_set_context()
3-) priv->host = usbredirhost_open_full(...)

As this did not happen at the time of the crash, yes, maybe
spice_usb_device_manager_initable_init() failed or was not called
which would mean that spice_usb_device_manager_get() was not called.

Not sure who should be calling spice_usb_device_manager_get() first?
- src/usb-device-widget.c at spice_usb_device_widget_constructor()
- src/spice-option.c  at spice_set_session_option()
- src/spice-gtk-session.c at spice_gtk_session_request_auto_usbredir()

But I'm sure it can't be on src/channel-usbredir.c at
spice_usbredir_channel_open_device() as this needs priv->host already :)

> NB: channel_up is missing from the backtrace but is what is called
> between #0 and #1.

Indeed, I wonder why is it missing?

> Can we miss some data if we skip this flushing of pending writes? I
> guess we cannot have any since priv->host does not exist yet?

I don't think we can miss anything if the device is not even open yet.
Maybe fixing this crash can trigger a different issue later on,
hopefully it would be easier to track.

Let me know if I should improve the commit log but after writing this
email, the 'seems' are likely correct as I'm not 100% sure how this was
triggered.

>
> Christophe

Cheers,
  toso

> 
> > 
> >  #0 in usbredirhost_write_guest_data (host=0x0) at usbredirhost.c:876
> >  #1 in spice_channel_recv_auth (channel=0x55af5f3b8f50 
> > [SpiceUsbredirChannel]) at spice-channel.c:1225
> >  #2 in spice_channel_coroutine (data=0x55af5f3b8f50) at spice-channel.c:2580
> >  #3 in coroutine_trampoline (cc=0x55af5f3b85e0) at coroutine_ucontext.c:63
> > 
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1399838
> > 
> > Signed-off-by: Victor Toso 
> > ---
> >  src/channel-usbredir.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> > index 4837d68..561f486 100644
> > --- a/src/channel-usbredir.c
> > +++ b/src/channel-usbredir.c
> > @@ -817,6 +817,9 @@ static void spice_usbredir_channel_up(SpiceChannel *c)
> >  SpiceUsbredirChannel *channel = SPICE_USBREDIR_CHANNEL(c);
> >  SpiceUsbredirChannelPrivate *priv = channel->priv;
> >  
> > +if (priv->host == NULL)
> > +  return;
> > +
> >  /* Flush any pending writes */
> >  usbredirhost_write_guest_data(priv->host);
> >  }
> > -- 
> > 2.9.3
> > 
> > ___
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel




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 v2 11/19] Add red_qxl_destroy function

[Frediano Ziglio]

> 
> Hi,
> 
> On Fri, 2016-11-25 at 14:52 +, Frediano Ziglio wrote:
> > Allows to destroy a QXL object
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  server/red-qxl.c| 25 ++---
> >  server/red-qxl.h|  1 +
> >  server/red-worker.c | 35 ++-
> >  server/red-worker.h |  1 +
> >  4 files changed, 58 insertions(+), 4 deletions(-)
> > 
> > diff --git a/server/red-qxl.c b/server/red-qxl.c
> > index 19cff95..de0546f 100644
> > --- a/server/red-qxl.c
> > +++ b/server/red-qxl.c
> > @@ -58,6 +58,7 @@ struct QXLState {
> >  QXLDevSurfaceCreate surface_create;
> >  unsigned int max_monitors;
> >  RedsState *reds;
> > +RedWorker *worker;
> >  
> >  pthread_mutex_t scanout_mutex;
> >  SpiceMsgDisplayGlScanoutUnix scanout;
> > @@ -1006,11 +1007,29 @@ void red_qxl_init(RedsState *reds,
> > QXLInstance *qxl)
> >  client_display_cbs.disconnect =
> > red_qxl_disconnect_display_peer;
> >  client_display_cbs.migrate = red_qxl_display_migrate;
> >  
> > -// TODO: reference and free
> > -RedWorker *worker = red_worker_new(qxl, _cursor_cbs,
> > +qxl_state->worker = red_worker_new(qxl, _cursor_cbs,
> > _display_cbs);
> >  
> > -red_worker_run(worker);
> > +red_worker_run(qxl_state->worker);
> > +}
> > +
> > +void red_qxl_destroy(QXLInstance *qxl)
> > +{
> > +spice_return_if_fail(qxl->st != NULL && qxl->st->dispatcher !=
> > NULL);
> > +
> > +QXLState *qxl_state = qxl->st;
> > +
> > +/* send message to close thread */
> > +RedWorkerMessageClose message;
> > +dispatcher_send_message(qxl_state->dispatcher,
> > +RED_WORKER_MESSAGE_CLOSE_WORKER,
> > +);
> > +red_worker_free(qxl_state->worker);
> > +g_object_unref(qxl_state->dispatcher);
> > +/* this must be done after calling red_worker_free */
> > +qxl->st = NULL;
> > +pthread_mutex_destroy(_state->scanout_mutex);
> > +free(qxl_state);
> >  }
> >  
> >  Dispatcher *red_qxl_get_dispatcher(QXLInstance *qxl)
> > diff --git a/server/red-qxl.h b/server/red-qxl.h
> > index 65357b1..7743124 100644
> > --- a/server/red-qxl.h
> > +++ b/server/red-qxl.h
> > @@ -24,6 +24,7 @@
> >  typedef struct AsyncCommand AsyncCommand;
> >  
> >  void red_qxl_init(SpiceServer *reds, QXLInstance *qxl);
> > +void red_qxl_destroy(QXLInstance *qxl);
> >  
> >  void red_qxl_on_ic_change(QXLInstance *qxl, SpiceImageCompression
> > ic);
> >  void red_qxl_on_sv_change(QXLInstance *qxl, int sv);
> > diff --git a/server/red-worker.c b/server/red-worker.c
> > index d699bd6..5dac0ec 100644
> > --- a/server/red-worker.c
> > +++ b/server/red-worker.c
> > @@ -1410,7 +1410,6 @@ static void *red_worker_main(void *arg)
> >  g_main_loop_unref(loop);
> >  worker->loop = NULL;
> >  
> > -/* FIXME: free worker, and join threads */
> >  return NULL;
> >  }
> >  
> > @@ -1435,3 +1434,37 @@ bool red_worker_run(RedWorker *worker)
> >  
> >  return r == 0;
> >  }
> > +
> > +static void red_worker_close_channel(RedChannel *channel)
> > +{
> > +red_channel_reset_thread_id(channel);
> 
> for what is it needed ^ ? It sets the thread id, but this information
> is not used in red_channel_destroy()
> 

A channel should be run in a single thread. As we are doing operations
in another thread (the main one) it's a safe think to tell the
channel that we switch it.
There are mainly some checks on the thread.

> Pavel
> 

Frediano

> > +red_channel_destroy(channel);
> > +}
> > +
> > +void red_worker_free(RedWorker *worker)
> > +{
> > +RedsState *reds = red_qxl_get_server(worker->qxl->st);
> > +
> > +/* prevent any possible future attempt to connect to new
> > clients */
> > +reds_unregister_channel(reds, RED_CHANNEL(worker-
> > >cursor_channel));
> > +reds_unregister_channel(reds, RED_CHANNEL(worker-
> > >display_channel));
> > +
> > +pthread_join(worker->thread, NULL);
> > +
> > +red_worker_close_channel(RED_CHANNEL(worker->cursor_channel));
> > +worker->cursor_channel = NULL;
> > +red_worker_close_channel(RED_CHANNEL(worker->display_channel));
> > +worker->display_channel = NULL;
> > +
> > +if (worker->dispatch_watch) {
> > +worker->core.watch_remove(>core, worker-
> > >dispatch_watch);
> > +}
> > +
> > +g_main_context_unref(worker->core.main_context);
> > +
> > +if (worker->record) {
> > +red_record_free(worker->record);
> > +}
> > +memslot_info_destroy(>mem_slots);
> > +free(worker);
> > +}
> > diff --git a/server/red-worker.h b/server/red-worker.h
> > index 53b92b3..d5b5a78 100644
> > --- a/server/red-worker.h
> > +++ b/server/red-worker.h
> > @@ -29,5 +29,6 @@ RedWorker* red_worker_new(QXLInstance *qxl,
> >    const ClientCbs *client_cursor_cbs,
> >    const ClientCbs *client_display_cbs);
> >  bool   

Re: [Spice-devel] [PATCH v2 11/19] Add red_qxl_destroy function

[Pavel Grunt]

Hi,

On Fri, 2016-11-25 at 14:52 +, Frediano Ziglio wrote:
> Allows to destroy a QXL object
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/red-qxl.c| 25 ++---
>  server/red-qxl.h|  1 +
>  server/red-worker.c | 35 ++-
>  server/red-worker.h |  1 +
>  4 files changed, 58 insertions(+), 4 deletions(-)
> 
> diff --git a/server/red-qxl.c b/server/red-qxl.c
> index 19cff95..de0546f 100644
> --- a/server/red-qxl.c
> +++ b/server/red-qxl.c
> @@ -58,6 +58,7 @@ struct QXLState {
>  QXLDevSurfaceCreate surface_create;
>  unsigned int max_monitors;
>  RedsState *reds;
> +RedWorker *worker;
>  
>  pthread_mutex_t scanout_mutex;
>  SpiceMsgDisplayGlScanoutUnix scanout;
> @@ -1006,11 +1007,29 @@ void red_qxl_init(RedsState *reds,
> QXLInstance *qxl)
>  client_display_cbs.disconnect =
> red_qxl_disconnect_display_peer;
>  client_display_cbs.migrate = red_qxl_display_migrate;
>  
> -// TODO: reference and free
> -RedWorker *worker = red_worker_new(qxl, _cursor_cbs,
> +qxl_state->worker = red_worker_new(qxl, _cursor_cbs,
> _display_cbs);
>  
> -red_worker_run(worker);
> +red_worker_run(qxl_state->worker);
> +}
> +
> +void red_qxl_destroy(QXLInstance *qxl)
> +{
> +spice_return_if_fail(qxl->st != NULL && qxl->st->dispatcher !=
> NULL);
> +
> +QXLState *qxl_state = qxl->st;
> +
> +/* send message to close thread */
> +RedWorkerMessageClose message;
> +dispatcher_send_message(qxl_state->dispatcher,
> +RED_WORKER_MESSAGE_CLOSE_WORKER,
> +);
> +red_worker_free(qxl_state->worker);
> +g_object_unref(qxl_state->dispatcher);
> +/* this must be done after calling red_worker_free */
> +qxl->st = NULL;
> +pthread_mutex_destroy(_state->scanout_mutex);
> +free(qxl_state);
>  }
>  
>  Dispatcher *red_qxl_get_dispatcher(QXLInstance *qxl)
> diff --git a/server/red-qxl.h b/server/red-qxl.h
> index 65357b1..7743124 100644
> --- a/server/red-qxl.h
> +++ b/server/red-qxl.h
> @@ -24,6 +24,7 @@
>  typedef struct AsyncCommand AsyncCommand;
>  
>  void red_qxl_init(SpiceServer *reds, QXLInstance *qxl);
> +void red_qxl_destroy(QXLInstance *qxl);
>  
>  void red_qxl_on_ic_change(QXLInstance *qxl, SpiceImageCompression
> ic);
>  void red_qxl_on_sv_change(QXLInstance *qxl, int sv);
> diff --git a/server/red-worker.c b/server/red-worker.c
> index d699bd6..5dac0ec 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -1410,7 +1410,6 @@ static void *red_worker_main(void *arg)
>  g_main_loop_unref(loop);
>  worker->loop = NULL;
>  
> -/* FIXME: free worker, and join threads */
>  return NULL;
>  }
>  
> @@ -1435,3 +1434,37 @@ bool red_worker_run(RedWorker *worker)
>  
>  return r == 0;
>  }
> +
> +static void red_worker_close_channel(RedChannel *channel)
> +{
> +red_channel_reset_thread_id(channel);

for what is it needed ^ ? It sets the thread id, but this information
is not used in red_channel_destroy()

Pavel

> +red_channel_destroy(channel);
> +}
> +
> +void red_worker_free(RedWorker *worker)
> +{
> +RedsState *reds = red_qxl_get_server(worker->qxl->st);
> +
> +/* prevent any possible future attempt to connect to new
> clients */
> +reds_unregister_channel(reds, RED_CHANNEL(worker-
> >cursor_channel));
> +reds_unregister_channel(reds, RED_CHANNEL(worker-
> >display_channel));
> +
> +pthread_join(worker->thread, NULL);
> +
> +red_worker_close_channel(RED_CHANNEL(worker->cursor_channel));
> +worker->cursor_channel = NULL;
> +red_worker_close_channel(RED_CHANNEL(worker->display_channel));
> +worker->display_channel = NULL;
> +
> +if (worker->dispatch_watch) {
> +worker->core.watch_remove(>core, worker-
> >dispatch_watch);
> +}
> +
> +g_main_context_unref(worker->core.main_context);
> +
> +if (worker->record) {
> +red_record_free(worker->record);
> +}
> +memslot_info_destroy(>mem_slots);
> +free(worker);
> +}
> diff --git a/server/red-worker.h b/server/red-worker.h
> index 53b92b3..d5b5a78 100644
> --- a/server/red-worker.h
> +++ b/server/red-worker.h
> @@ -29,5 +29,6 @@ RedWorker* red_worker_new(QXLInstance *qxl,
>    const ClientCbs *client_cursor_cbs,
>    const ClientCbs *client_display_cbs);
>  bool   red_worker_run(RedWorker *worker);
> +void red_worker_free(RedWorker *worker);
>  
>  #endif
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server v3] Return invalid value from very old obsolete functions

[Pavel Grunt]

On Wed, 2016-11-30 at 12:58 +, Frediano Ziglio wrote:
> These functions are not used since years and are not supporting
> multiple clients.
> 
> Signed-off-by: Frediano Ziglio 
Acked-by: Pavel Grunt 

Thanks,
Pavel

> ---
>  server/main-channel.c | 12 
>  server/main-channel.h |  2 --
>  server/reds.c | 12 
>  3 files changed, 4 insertions(+), 22 deletions(-)
> 
> Changes since v2:
> - propagate error to remove some internal functions.
> 
> diff --git a/server/main-channel.c b/server/main-channel.c
> index 37b2f25..24dd448 100644
> --- a/server/main-channel.c
> +++ b/server/main-channel.c
> @@ -306,18 +306,6 @@ MainChannelClient
> *main_channel_link(MainChannel *channel, RedClient *client,
>  return mcc;
>  }
>  
> -int main_channel_getsockname(MainChannel *main_chan, struct
> sockaddr *sa, socklen_t *salen)
> -{
> -return main_chan ?
> -getsockname(red_channel_get_first_socket(RED_CHANNEL(main_c
> han)), sa, salen) : -1;
> -}
> -
> -int main_channel_getpeername(MainChannel *main_chan, struct
> sockaddr *sa, socklen_t *salen)
> -{
> -return main_chan ?
> -getpeername(red_channel_get_first_socket(RED_CHANNEL(main_c
> han)), sa, salen) : -1;
> -}
> -
>  MainChannel* main_channel_new(RedsState *reds)
>  {
>  // TODO: set the migration flag of the channel
> diff --git a/server/main-channel.h b/server/main-channel.h
> index cb53fb4..b70649c 100644
> --- a/server/main-channel.h
> +++ b/server/main-channel.h
> @@ -65,8 +65,6 @@ void main_channel_push_mouse_mode(MainChannel
> *main_chan, int current_mode, int
>  void main_channel_push_agent_connected(MainChannel *main_chan);
>  void main_channel_push_agent_disconnected(MainChannel *main_chan);
>  void main_channel_push_multi_media_time(MainChannel *main_chan, int
> time);
> -int main_channel_getsockname(MainChannel *main_chan, struct
> sockaddr *sa, socklen_t *salen);
> -int main_channel_getpeername(MainChannel *main_chan, struct
> sockaddr *sa, socklen_t *salen);
>  
>  int main_channel_is_connected(MainChannel *main_chan);
>  
> diff --git a/server/reds.c b/server/reds.c
> index bc0cc01..5b18162 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -3848,20 +3848,16 @@ SPICE_GNUC_VISIBLE int
> spice_server_set_channel_security(SpiceServer *s, const c
>  return -1;
>  }
>  
> +/* very obsolete and old function, retain only for ABI */
>  SPICE_GNUC_VISIBLE int spice_server_get_sock_info(SpiceServer
> *reds, struct sockaddr *sa, socklen_t *salen)
>  {
> -if (main_channel_getsockname(reds->main_channel, sa, salen) <
> 0) {
> -return -1;
> -}
> -return 0;
> +return -1;
>  }
>  
> +/* very obsolete and old function, retain only for ABI */
>  SPICE_GNUC_VISIBLE int spice_server_get_peer_info(SpiceServer
> *reds, struct sockaddr *sa, socklen_t *salen)
>  {
> -if (main_channel_getpeername(reds->main_channel, sa, salen) <
> 0) {
> -return -1;
> -}
> -return 0;
> +return -1;
>  }
>  
>  SPICE_GNUC_VISIBLE int spice_server_is_server_mouse(SpiceServer
> *reds)
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] Processing of rendering operations in separate thread

[Javier Celaya]

Hi Yuri
El mié, 30-11-2016 a las 15:02 +0200, Yuri Benditovich escribió:
> Hello Javier,
> 
> After implementing the pushing thread in current qxl-wddm-dod I
> measure the CPU consumption in PresentDisplayOnly call and in the
> thread on pushing drawables to the device. The results show that the
> time to push drawables is negligible in relation to time of copying
> dirty rects to the device memory (in average the proportion is ~
> 1/500) and in typical case the pushing thread serves only single
> 'present' operation and then waits for next operation.
> I tried in on Win10 with 2-3 G memory and 1-2 CPUs with regular user
> activity (opening windows, redrawing, scrolling etc)
> 
> Do I miss something?
No, sounds ok to me. I wanted to experiment with multithreading, but I
never got this far with my tests. So it's fairly possible that you get
no performance improvement over using just one thread.
> > Thanks,
> Yuri
> 

> On Fri, Nov 25, 2016 at 11:11 AM, Javier Celaya > 
> >  wrote:
> > Hello Yuri
> > > > El vie, 25-11-2016 a las 01:08 +0200, Yuri Benditovich escribió:
> > > I'm porting to [qxl-wddm-dod] set of flexvdi changes
> > > related to execution of '> > > present display only' events
> > > in separate thread. There are 2 questions below I'd like to ask and k> > 
> > > > now your opinion.

> > > I see there 2 aspects:
> > > - reliability
> > > - performance
> > > 
> > > Reliability:
> > > I see in flexvdi mailing list existing report of
> > > BSOD upon system shutdown. Possible cause is lack of
> > > synchronization between system flows, hardware availability and worker 
> > > thread state > > > (last patch in flexvdi 'Terminate working thread on 
> > > exit' > > > introduces termination procedure but nobody calls it, > > > 
> > > as I can see)
> > > The lack of synchronization may cause also races in
> > > power> > >  management flows and (possible) on changing
> > > operating mode.

> > > Question 1:
> > > Do you have some additional recommendation which
> > > flows shall be specially checked for races with
> > > rendering thread?
> > > > Unfortunately, the truth is, we have not thoroughly tested our code to 
> > > > remove these races yet. The clients this driver was intended for are 
> > > > still stuck using Windows XP/7, and our development is stalled. So, I 
> > > > cannot think of any situation you should check that you do not know 
> > > > about yet.
> > > > > > > > Performance:
> > > It looks like > > > the change should not affect total CPU consumption for
> > > the rendering, it splits more or less the same operations over
> > > 2 different threads. > > > It is still possible that the > > > change can 
> > > improve
> > > common user experience > > > due to > > > faster indication of operation 
> > > completion to > > > the OS.
> > > > We were not trying to reduce total CPU consumption. After all, the 
> > > > driver just copies rects from main memory to VRAM and passes them to 
> > > > the spice server; there is little to reduce there. Rather, we tried to 
> > > > increase the throughput of graphic operations, by not locking the 
> > > > DirectX subsystem while we wait for the spice server to accept new 
> > > > drawables. That is, we do not mind using more CPU if that results in 
> > > > painting faster.
> > On the other hand, I was thinking that maybe we could get the DirectX 
> > subsystem to provide the rects already in VRAM if we described it as a 
> > linear memory segment on driver initialization. In that way, the copying 
> > operation could also be removed. However, I am not sure if this actually 
> > works or even how to do it, it is just an idea.
> > 
> > > Question 2:
> > > Do you have some ideas how to make quantitive
> > > evaluation of this possible > > > improvement of user experience? 

> > > I think about: 
> > > - finding scenarios when we receive > > > rendering calls 
> > > (PresentDisplayOnly) when the worker > > > thread is still processing 
> > > previous operation. If they exist this can mean that some bottleneck 
> > > solved in GDI.
> > > - writing or getting tool that loads the graphics
> > > adapter by heavy operations (like continuos moving of window / scrolling 
> > > etc) with CPU consumption measurement
> > > > We used a simple tool to measure the performance: it creates a window 
> > > > and continuously issues WM_PAINT events where the full background is 
> > > > filled with color, then measures the number of processed events per 
> > > > second (not CPU). It is quite naive, but it provides a good starting 
> > > > reference, since the tool, with the XDDM QXL driver in Windows 7, 
> > > > outputs almost twice as much paint events as executing it in Windows 8 
> > > > with the WDDM QXL driver. There are other measurements you can try to 
> > > > obtain, like how much time does it take until a paint event gets to the 
> > > > spice server queue, ready to be sent to the client (although I'm not 
> > > > sure how to measure it). 

Re: [Spice-devel] Processing of rendering operations in separate thread

[Frediano Ziglio]

As already stated by Javier the difference was not the CPU usage but user 
experience. 
I think that mainly the Windows engine is free to do its own job while our 
driver complete 
the operation. Looking at Windows calls to our driver looks like Windows keep 
another 
frame buffer and periodically (didn't try to understand when exactly or how 
often) pass 
dirty rects to update the card (QXL in this case) frame buffer. 
I have the feeling (don't know how to verify) that if the driver returns 
STATUS_PENDING 
Windows will collapse dirty rects while the driver is doing its job basically 
decreasing calls 
to our driver but doing the proper frame buffer updates. 

Frediano 

- Original Message -

> Hello Javier,

> After implementing the pushing thread in current qxl-wddm-dod I measure the
> CPU consumption in PresentDisplayOnly call and in the thread on pushing
> drawables to the device. The results show that the time to push drawables is
> negligible in relation to time of copying dirty rects to the device memory
> (in average the proportion is ~ 1/500) and in typical case the pushing
> thread serves only single 'present' operation and then waits for next
> operation.
> I tried in on Win10 with 2-3 G memory and 1-2 CPUs with regular user activity
> (opening windows, redrawing, scrolling etc)

> Do I miss something?

> Thanks,
> Yuri

> On Fri, Nov 25, 2016 at 11:11 AM, Javier Celaya < javier.cel...@flexvdi.com >
> wrote:

> > Hello Yuri
> 

> > El vie, 25-11-2016 a las 01:08 +0200, Yuri Benditovich escribió:
> 

> > > I'm porting to [qxl-wddm-dod] set of flexvdi changes
> > 
> 
> > > related to execution of ' present display only' events
> > 
> 
> > > in separate thread. There are 2 questions below I'd like to ask and k now
> > > your opinion.
> > 
> 

> > > I see there 2 aspects:
> > 
> 
> > > - reliability
> > 
> 
> > > - performance
> > 
> 

> > > Reliability:
> > 
> 
> > > I see in flexvdi mailing list existing report of
> > 
> 
> > > BSOD upon system shutdown. Possible cause is lack of
> > 
> 
> > > synchronization between system flows, hardware availability and worker
> > > thread
> > > state (last patch in flexvdi 'Terminate working thread on exit'
> > > introduces
> > > termination procedure but nobody calls it, as I can see)
> > 
> 
> > > The lack of synchronization may cause also races in
> > 
> 
> > > power management flows and (possible) on changing
> > 
> 
> > > operating mode.
> > 
> 

> > > Question 1:
> > 
> 
> > > Do you have some additional recommendation which
> > 
> 
> > > flows shall be specially checked for races with
> > 
> 
> > > rendering thread?
> > 
> 

> > Unfortunately, the truth is, we have not thoroughly tested our code to
> > remove
> > these races yet. The clients this driver was intended for are still stuck
> > using Windows XP/7, and our development is stalled. So, I cannot think of
> > any situation you should check that you do not know about yet.
> 

> > > Performance:
> > 
> 
> > > It looks like the change should not affect total CPU consumption for
> > 
> 
> > > the rendering, it splits more or less the same operations over
> > 
> 
> > > 2 different threads. It is still possible that the change can improve
> > 
> 
> > > common user experience due to faster indication of operation completion
> > > to
> > > the OS.
> > 
> 

> > We were not trying to reduce total CPU consumption. After all, the driver
> > just copies rects from main memory to VRAM and passes them to the spice
> > server; there is little to reduce there. Rather, we tried to increase the
> > throughput of graphic operations, by not locking the DirectX subsystem
> > while
> > we wait for the spice server to accept new drawables. That is, we do not
> > mind using more CPU if that results in painting faster.
> 
> > On the other hand, I was thinking that maybe we could get the DirectX
> > subsystem to provide the rects already in VRAM if we described it as a
> > linear memory segment on driver initialization. In that way, the copying
> > operation could also be removed. However, I am not sure if this actually
> > works or even how to do it, it is just an idea.
> 

> > > Question 2:
> > 
> 
> > > Do you have some ideas how to make quantitive
> > 
> 
> > > evaluation of this possible improvement of user experience?
> > 
> 

> > > I think about:
> > 
> 
> > > - finding scenarios when we receive rendering calls (PresentDisplayOnly)
> > > when
> > > the worker thread is still processing previous operation. If they exist
> > > this
> > > can mean that some bottleneck solved in GDI.
> > 
> 
> > > - writing or getting tool that loads the graphics
> > 
> 
> > > adapter by heavy operations (like continuos moving of window / scrolling
> > > etc)
> > > with CPU consumption measurement
> > 
> 

> > We used a simple tool to measure the performance: it creates a window and
> > continuously issues WM_PAINT events where the full background is filled
> > with
> > color, then measures the number of processed events per 

Re: [Spice-devel] Processing of rendering operations in separate thread

[Yuri Benditovich]

Hello Javier,

After implementing the pushing thread in current qxl-wddm-dod I measure the
CPU consumption in PresentDisplayOnly call and in the thread on pushing
drawables to the device. The results show that the time to push drawables
is negligible in relation to time of copying dirty rects to the device
memory (in average the proportion is ~ 1/500) and in typical case the
pushing thread serves only single 'present' operation and then waits for
next operation.
I tried in on Win10 with 2-3 G memory and 1-2 CPUs with regular user
activity (opening windows, redrawing, scrolling etc)

Do I miss something?

Thanks,
Yuri


On Fri, Nov 25, 2016 at 11:11 AM, Javier Celaya 
wrote:

> Hello Yuri
>
> El vie, 25-11-2016 a las 01:08 +0200, Yuri Benditovich escribió:
>
> I'm porting to [qxl-wddm-dod] set of flexvdi changes
> related to execution of 'present display only' events
> in separate thread. There are 2* questions below* I'd like to ask and know
> your opinion.
>
> I see there 2 aspects:
> - reliability
> - performance
>
> *Reliability:*
> I see in flexvdi mailing list existing report of
> BSOD upon system shutdown. Possible cause is lack of
> synchronization between system flows, hardware availability and worker
> thread state (last patch in flexvdi 'Terminate working thread on exit' 
> introduces
> termination procedure but nobody calls it, as I can see)
> The lack of synchronization may cause also races in
> power management flows and (possible) on changing
> operating mode.
>
> *Question 1:*
> *Do you have some additional recommendation which*
> *flows shall be specially checked for races with*
> *rendering thread?*
>
>
> Unfortunately, the truth is, we have not thoroughly tested our code to
> remove these races yet. The clients this driver was intended for are still
> stuck using Windows XP/7, and our development is stalled. So, I cannot
> think of any situation you should check that you do not know about yet.
>
>
> *Performance:*
> It looks like the change should not affect total CPU consumption for
> the rendering, it splits more or less the same operations over
> 2 different threads. It is still possible that the change can improve
> common user experience due to faster indication of operation completion
> to the OS.
>
>
> We were not trying to reduce total CPU consumption. After all, the driver
> just copies rects from main memory to VRAM and passes them to the spice
> server; there is little to reduce there. Rather, we tried to increase the
> throughput of graphic operations, by not locking the DirectX subsystem
> while we wait for the spice server to accept new drawables. That is, we do
> not mind using more CPU if that results in painting faster.
> On the other hand, I was thinking that maybe we could get the DirectX
> subsystem to provide the rects already in VRAM if we described it as a
> linear memory segment on driver initialization. In that way, the copying
> operation could also be removed. However, I am not sure if this actually
> works or even how to do it, it is just an idea.
>
>
> *Question 2:*
> *Do you have some ideas how to make quantitive*
> *evaluation of this possible **improvement of user experience? *
>
> I think about:
> - finding scenarios when we receive rendering calls (PresentDisplayOnly)
> when the worker thread is still processing previous operation. If they
> exist this can mean that some bottleneck solved in GDI.
> - writing or getting tool that loads the graphics
> adapter by heavy operations (like continuos moving of window / scrolling
> etc) with CPU consumption measurement
>
>
> We used a simple tool to measure the performance: it creates a window and
> continuously issues WM_PAINT events where the full background is filled
> with color, then measures the number of processed events per second (not
> CPU). It is quite naive, but it provides a good starting reference, since
> the tool, with the XDDM QXL driver in Windows 7, outputs almost twice as
> much paint events as executing it in Windows 8 with the WDDM QXL driver.
> There are other measurements you can try to obtain, like how much time does
> it take until a paint event gets to the spice server queue, ready to be
> sent to the client (although I'm not sure how to measure it). This delay
> affects the user perception of performance.
>
>
>
> Please share your thoughts.
>
> Thanks,
> Yuri
>
>
>
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH spice-server v3] Return invalid value from very old obsolete functions

[Frediano Ziglio]

These functions are not used since years and are not supporting
multiple clients.

Signed-off-by: Frediano Ziglio 
---
 server/main-channel.c | 12 
 server/main-channel.h |  2 --
 server/reds.c | 12 
 3 files changed, 4 insertions(+), 22 deletions(-)

Changes since v2:
- propagate error to remove some internal functions.

diff --git a/server/main-channel.c b/server/main-channel.c
index 37b2f25..24dd448 100644
--- a/server/main-channel.c
+++ b/server/main-channel.c
@@ -306,18 +306,6 @@ MainChannelClient *main_channel_link(MainChannel *channel, 
RedClient *client,
 return mcc;
 }
 
-int main_channel_getsockname(MainChannel *main_chan, struct sockaddr *sa, 
socklen_t *salen)
-{
-return main_chan ?
-getsockname(red_channel_get_first_socket(RED_CHANNEL(main_chan)), sa, 
salen) : -1;
-}
-
-int main_channel_getpeername(MainChannel *main_chan, struct sockaddr *sa, 
socklen_t *salen)
-{
-return main_chan ?
-getpeername(red_channel_get_first_socket(RED_CHANNEL(main_chan)), sa, 
salen) : -1;
-}
-
 MainChannel* main_channel_new(RedsState *reds)
 {
 // TODO: set the migration flag of the channel
diff --git a/server/main-channel.h b/server/main-channel.h
index cb53fb4..b70649c 100644
--- a/server/main-channel.h
+++ b/server/main-channel.h
@@ -65,8 +65,6 @@ void main_channel_push_mouse_mode(MainChannel *main_chan, int 
current_mode, int
 void main_channel_push_agent_connected(MainChannel *main_chan);
 void main_channel_push_agent_disconnected(MainChannel *main_chan);
 void main_channel_push_multi_media_time(MainChannel *main_chan, int time);
-int main_channel_getsockname(MainChannel *main_chan, struct sockaddr *sa, 
socklen_t *salen);
-int main_channel_getpeername(MainChannel *main_chan, struct sockaddr *sa, 
socklen_t *salen);
 
 int main_channel_is_connected(MainChannel *main_chan);
 
diff --git a/server/reds.c b/server/reds.c
index bc0cc01..5b18162 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -3848,20 +3848,16 @@ SPICE_GNUC_VISIBLE int 
spice_server_set_channel_security(SpiceServer *s, const c
 return -1;
 }
 
+/* very obsolete and old function, retain only for ABI */
 SPICE_GNUC_VISIBLE int spice_server_get_sock_info(SpiceServer *reds, struct 
sockaddr *sa, socklen_t *salen)
 {
-if (main_channel_getsockname(reds->main_channel, sa, salen) < 0) {
-return -1;
-}
-return 0;
+return -1;
 }
 
+/* very obsolete and old function, retain only for ABI */
 SPICE_GNUC_VISIBLE int spice_server_get_peer_info(SpiceServer *reds, struct 
sockaddr *sa, socklen_t *salen)
 {
-if (main_channel_getpeername(reds->main_channel, sa, salen) < 0) {
-return -1;
-}
-return 0;
+return -1;
 }
 
 SPICE_GNUC_VISIBLE int spice_server_is_server_mouse(SpiceServer *reds)
-- 
2.9.3

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

Re: [Spice-devel] [PATCH v2 07/19] Return invalid value from very old obsolete functions

[Frediano Ziglio]

> 
> On Wed, 2016-11-30 at 07:43 -0500, Frediano Ziglio wrote:
> > > 
> > > Hi,
> > > 
> > > Why not return -1 in spice_server_get_sock_info() and
> > > spice_server_get_peer_info() and remove these main channel
> > > functions.
> > > 
> > 
> > Next commit remove this function.
> 
> It doesn't
> 
> spice_server_get_peer_info and spice_server_get_sock_info are using
> main_channel_get{peer, sock}name function
> 

Sorry, I completely misread your reply

Yes, I'll post an update.

Frediano

> > 
> > > Also the public function should be marked as deprecated if their
> > > usage
> > >  should be avoided
> > > 
> > 
> > They already are.
> > 
> > > Pavel
> > > 
> > 
> > Frediano
> > 
> > > On Fri, 2016-11-25 at 14:52 +, Frediano Ziglio wrote:
> > > > These functions are not used since years and are not supporting
> > > > multiple clients.
> > > > 
> > > > Signed-off-by: Frediano Ziglio 
> > > > ---
> > > >  server/main-channel.c | 8 
> > > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/server/main-channel.c b/server/main-channel.c
> > > > index 37b2f25..3d107c4 100644
> > > > --- a/server/main-channel.c
> > > > +++ b/server/main-channel.c
> > > > @@ -306,16 +306,16 @@ MainChannelClient
> > > > *main_channel_link(MainChannel *channel, RedClient *client,
> > > >  return mcc;
> > > >  }
> > > >  
> > > > +/* very obsolete and old function, retain only for ABI */
> > > >  int main_channel_getsockname(MainChannel *main_chan, struct
> > > > sockaddr *sa, socklen_t *salen)
> > > >  {
> > > > -return main_chan ?
> > > > -getsockname(red_channel_get_first_socket(RED_CHANNEL(ma
> > > > in_c
> > > > han)), sa, salen) : -1;
> > > > +return -1;
> > > >  }
> > > >  
> > > > +/* very obsolete and old function, retain only for ABI */
> > > >  int main_channel_getpeername(MainChannel *main_chan, struct
> > > > sockaddr *sa, socklen_t *salen)
> > > >  {
> > > > -return main_chan ?
> > > > -getpeername(red_channel_get_first_socket(RED_CHANNEL(ma
> > > > in_c
> > > > han)), sa, salen) : -1;
> > > > +return -1;
> > > >  }
> > > >  
> > > >  MainChannel* main_channel_new(RedsState *reds)
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v2 09/19] Avoid to leak timer in InputsChannel

[Pavel Grunt]

On Fri, 2016-11-25 at 14:52 +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
Acked-by: Pavel Grunt 
> ---
>  server/inputs-channel.c | 13 +
>  1 file changed, 13 insertions(+)
> 
> diff --git a/server/inputs-channel.c b/server/inputs-channel.c
> index 7c397a4..99c2888 100644
> --- a/server/inputs-channel.c
> +++ b/server/inputs-channel.c
> @@ -620,6 +620,18 @@ inputs_channel_constructed(GObject *object)
>  }
>  
>  static void
> +inputs_channel_finalize(GObject *object)
> +{
> +InputsChannel *self = INPUTS_CHANNEL(object);
> +RedsState *reds = red_channel_get_server(RED_CHANNEL(self));
> +
> +if (self->key_modifiers_timer) {
> +reds_core_timer_remove(reds, self->key_modifiers_timer);
> +}
> +G_OBJECT_CLASS(inputs_channel_parent_class)->finalize(object);
> +}
> +
> +static void
>  inputs_channel_init(InputsChannel *self)
>  {
>  }
> @@ -632,6 +644,7 @@ inputs_channel_class_init(InputsChannelClass
> *klass)
>  RedChannelClass *channel_class = RED_CHANNEL_CLASS(klass);
>  
>  object_class->constructed = inputs_channel_constructed;
> +object_class->finalize = inputs_channel_finalize;
>  
>  channel_class->parser =
> spice_get_client_channel_parser(SPICE_CHANNEL_INPUTS, NULL);
>  channel_class->handle_parsed = inputs_channel_handle_parsed;
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v2 16/19] cursor: Avoid cursor item leak

[Pavel Grunt]

On Fri, 2016-11-25 at 14:52 +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
Acked-by: Pavel Grunt 
> ---
>  server/cursor-channel.c | 15 +++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index 202ec89..e421bf7 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -438,10 +438,25 @@ void cursor_channel_connect(CursorChannel
> *cursor, RedClient *client, RedsStream
>  }
>  
>  static void
> +cursor_channel_finalize(GObject *object)
> +{
> +CursorChannel *self = CURSOR_CHANNEL(object);
> +
> +if (self->item) {
> +cursor_item_unref(self->item);
> +}
> +
> +G_OBJECT_CLASS(cursor_channel_parent_class)->finalize(object);
> +}
> +
> +static void
>  cursor_channel_class_init(CursorChannelClass *klass)
>  {
> +GObjectClass *object_class = G_OBJECT_CLASS(klass);
>  RedChannelClass *channel_class = RED_CHANNEL_CLASS(klass);
>  
> +object_class->finalize = cursor_channel_finalize;
> +
>  channel_class->parser =
> spice_get_client_channel_parser(SPICE_CHANNEL_CURSOR, NULL);
>  channel_class->handle_parsed =
> red_channel_client_handle_message;
>  
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v2 07/19] Return invalid value from very old obsolete functions

[Pavel Grunt]

On Wed, 2016-11-30 at 07:43 -0500, Frediano Ziglio wrote:
> > 
> > Hi,
> > 
> > Why not return -1 in spice_server_get_sock_info() and
> > spice_server_get_peer_info() and remove these main channel
> > functions.
> > 
> 
> Next commit remove this function.

It doesn't

spice_server_get_peer_info and spice_server_get_sock_info are using
main_channel_get{peer, sock}name function

> 
> > Also the public function should be marked as deprecated if their
> > usage
> >  should be avoided
> > 
> 
> They already are.
> 
> > Pavel
> > 
> 
> Frediano
> 
> > On Fri, 2016-11-25 at 14:52 +, Frediano Ziglio wrote:
> > > These functions are not used since years and are not supporting
> > > multiple clients.
> > > 
> > > Signed-off-by: Frediano Ziglio 
> > > ---
> > >  server/main-channel.c | 8 
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/server/main-channel.c b/server/main-channel.c
> > > index 37b2f25..3d107c4 100644
> > > --- a/server/main-channel.c
> > > +++ b/server/main-channel.c
> > > @@ -306,16 +306,16 @@ MainChannelClient
> > > *main_channel_link(MainChannel *channel, RedClient *client,
> > >  return mcc;
> > >  }
> > >  
> > > +/* very obsolete and old function, retain only for ABI */
> > >  int main_channel_getsockname(MainChannel *main_chan, struct
> > > sockaddr *sa, socklen_t *salen)
> > >  {
> > > -return main_chan ?
> > > -getsockname(red_channel_get_first_socket(RED_CHANNEL(ma
> > > in_c
> > > han)), sa, salen) : -1;
> > > +return -1;
> > >  }
> > >  
> > > +/* very obsolete and old function, retain only for ABI */
> > >  int main_channel_getpeername(MainChannel *main_chan, struct
> > > sockaddr *sa, socklen_t *salen)
> > >  {
> > > -return main_chan ?
> > > -getpeername(red_channel_get_first_socket(RED_CHANNEL(ma
> > > in_c
> > > han)), sa, salen) : -1;
> > > +return -1;
> > >  }
> > >  
> > >  MainChannel* main_channel_new(RedsState *reds)
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v4 11/14] Make RedChannelClient::incoming private

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/red-channel-client-private.h | 11 +++
 server/red-channel-client.c | 12 ++--
 server/red-channel-client.h | 13 -
 3 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/server/red-channel-client-private.h 
b/server/red-channel-client-private.h
index 38759ff..5f31ed2 100644
--- a/server/red-channel-client-private.h
+++ b/server/red-channel-client-private.h
@@ -50,6 +50,16 @@ typedef struct OutgoingHandler {
 int size;
 } OutgoingHandler;
 
+typedef struct IncomingHandler {
+IncomingHandlerInterface *cb;
+void *opaque;
+uint8_t header_buf[MAX_HEADER_SIZE];
+SpiceDataHeaderOpaque header;
+uint32_t header_pos;
+uint8_t *msg; // data of the msg following the header. allocated by 
alloc_msg_buf.
+uint32_t msg_pos;
+} IncomingHandler;
+
 struct RedChannelClientPrivate
 {
 RedChannel *channel;
@@ -99,6 +109,7 @@ struct RedChannelClientPrivate
 RedChannelClientLatencyMonitor latency_monitor;
 RedChannelClientConnectivityMonitor connectivity_monitor;
 
+IncomingHandler incoming;
 OutgoingHandler outgoing;
 };
 
diff --git a/server/red-channel-client.c b/server/red-channel-client.c
index 24b2fb0..2b1843a 100644
--- a/server/red-channel-client.c
+++ b/server/red-channel-client.c
@@ -267,8 +267,8 @@ static void red_channel_client_constructed(GObject *object)
 {
 RedChannelClient *self =  RED_CHANNEL_CLIENT(object);
 
-self->incoming.opaque = self;
-self->incoming.cb = red_channel_get_incoming_handler(self->priv->channel);
+self->priv->incoming.opaque = self;
+self->priv->incoming.cb = 
red_channel_get_incoming_handler(self->priv->channel);
 
 self->priv->outgoing.opaque = self;
 self->priv->outgoing.cb = 
red_channel_get_outgoing_handler(self->priv->channel);
@@ -276,15 +276,15 @@ static void red_channel_client_constructed(GObject 
*object)
 self->priv->outgoing.size = 0;
 
 if (red_channel_client_test_remote_common_cap(self, 
SPICE_COMMON_CAP_MINI_HEADER)) {
-self->incoming.header = mini_header_wrapper;
+self->priv->incoming.header = mini_header_wrapper;
 self->priv->send_data.header = mini_header_wrapper;
 self->priv->is_mini_header = TRUE;
 } else {
-self->incoming.header = full_header_wrapper;
+self->priv->incoming.header = full_header_wrapper;
 self->priv->send_data.header = full_header_wrapper;
 self->priv->is_mini_header = FALSE;
 }
-self->incoming.header.data = self->incoming.header_buf;
+self->priv->incoming.header.data = self->priv->incoming.header_buf;
 }
 
 static void red_channel_client_class_init(RedChannelClientClass *klass)
@@ -1189,7 +1189,7 @@ static void red_peer_handle_incoming(RedsStream *stream, 
IncomingHandler *handle
 void red_channel_client_receive(RedChannelClient *rcc)
 {
 g_object_ref(rcc);
-red_peer_handle_incoming(rcc->priv->stream, >incoming);
+red_peer_handle_incoming(rcc->priv->stream, >priv->incoming);
 g_object_unref(rcc);
 }
 
diff --git a/server/red-channel-client.h b/server/red-channel-client.h
index 94b4f58..13ddf30 100644
--- a/server/red-channel-client.h
+++ b/server/red-channel-client.h
@@ -190,23 +190,10 @@ gboolean 
red_channel_client_set_migration_seamless(RedChannelClient *rcc);
 void red_channel_client_set_destroying(RedChannelClient *rcc);
 gboolean red_channel_client_is_destroying(RedChannelClient *rcc);
 
-typedef struct IncomingHandler {
-IncomingHandlerInterface *cb;
-void *opaque;
-uint8_t header_buf[MAX_HEADER_SIZE];
-SpiceDataHeaderOpaque header;
-uint32_t header_pos;
-uint8_t *msg; // data of the msg following the header. allocated by 
alloc_msg_buf.
-uint32_t msg_pos;
-} IncomingHandler;
-
 struct RedChannelClient
 {
 GObject parent;
 
-/* protected */
-IncomingHandler incoming;
-
 RedChannelClientPrivate *priv;
 };
 
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v4 12/14] sound: Free more on SndChannel finalize

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/sound.c | 20 +---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index 0606fa0..ebbe821 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -1425,9 +1425,26 @@ snd_channel_init(SndChannel *self)
 }
 
 static void
+snd_channel_finalize(GObject *object)
+{
+SndChannel *channel = SND_CHANNEL(object);
+
+remove_channel(channel);
+
+free(channel->volume.volume);
+channel->volume.volume = NULL;
+
+G_OBJECT_CLASS(snd_channel_parent_class)->finalize(object);
+}
+
+static void
 snd_channel_class_init(SndChannelClass *klass)
 {
+GObjectClass *object_class = G_OBJECT_CLASS(klass);
 RedChannelClass *channel_class = RED_CHANNEL_CLASS(klass);
+
+object_class->finalize = snd_channel_finalize;
+
 channel_class->config_socket = snd_channel_config_socket;
 channel_class->alloc_recv_buf = snd_channel_client_alloc_recv_buf;
 channel_class->release_recv_buf = snd_channel_client_release_recv_buf;
@@ -1543,10 +1560,7 @@ static void snd_detach_common(SndChannel *channel)
 }
 RedsState *reds = red_channel_get_server(RED_CHANNEL(channel));
 
-remove_channel(channel);
 reds_unregister_channel(reds, RED_CHANNEL(channel));
-free(channel->volume.volume);
-channel->volume.volume = NULL;
 red_channel_destroy(RED_CHANNEL(channel));
 }
 
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v4 04/14] sound: Rename SndWorker to SndChannel

[Frediano Ziglio]

SndWorker has been historically based on RedChannel, initial git commit
has:
struct SndWorker {
 Channel base;
 ...
};

Signed-off-by: Frediano Ziglio 
---
 server/sound.c | 312 +-
 1 file changed, 156 insertions(+), 156 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index bafdd1e..5b66f3a 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -79,12 +79,12 @@ typedef int 
(*snd_channel_handle_message_proc)(SndChannelClient *client, size_t 
 typedef void (*snd_channel_on_message_done_proc)(SndChannelClient *client);
 typedef void (*snd_channel_cleanup_channel_proc)(SndChannelClient *client);
 
-typedef struct SndWorker SndWorker;
+typedef struct SndChannel SndChannel;
 
 /* Connects an audio client to a Spice client */
 struct SndChannelClient {
 RedsStream *stream;
-SndWorker *worker;
+SndChannel *channel;
 spice_parse_channel_func_t parser;
 int refs;
 
@@ -145,10 +145,10 @@ typedef struct SpiceVolumeState {
 } SpiceVolumeState;
 
 /* Base class for SpicePlaybackState and SpiceRecordState */
-struct SndWorker {
+struct SndChannel {
 RedChannel *base_channel;
 SndChannelClient *connection; /* Only one client is supported */
-SndWorker *next; /* For the global SndWorker list */
+SndChannel *next; /* For the global SndChannel list */
 
 int active;
 SpiceVolumeState volume;
@@ -156,11 +156,11 @@ struct SndWorker {
 };
 
 struct SpicePlaybackState {
-struct SndWorker worker;
+struct SndChannel channel;
 };
 
 struct SpiceRecordState {
-struct SndWorker worker;
+struct SndChannel channel;
 };
 
 typedef struct RecordChannelClient {
@@ -176,11 +176,11 @@ typedef struct RecordChannelClient {
 } RecordChannelClient;
 
 /* A list of all Spice{Playback,Record}State objects */
-static SndWorker *workers;
+static SndChannel *snd_channels;
 
 static void snd_receive(SndChannelClient *client);
-static void snd_playback_start(SndWorker *worker);
-static void snd_record_start(SndWorker *worker);
+static void snd_playback_start(SndChannel *channel);
+static void snd_record_start(SndChannel *channel);
 
 static SndChannelClient *snd_channel_ref(SndChannelClient *client)
 {
@@ -201,12 +201,12 @@ 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(client->worker->base_channel);
+return red_channel_get_server(client->channel->base_channel);
 }
 
 static void snd_disconnect_channel(SndChannelClient *client)
 {
-SndWorker *worker;
+SndChannel *channel;
 RedsState *reds;
 RedChannel *red_channel;
 uint32_t type;
@@ -220,17 +220,17 @@ static void snd_disconnect_channel(SndChannelClient 
*client)
 g_object_get(red_channel, "channel-type", , NULL);
 spice_debug("SndChannelClient=%p rcc=%p type=%d",
  client, client->channel_client, type);
-worker = client->worker;
+channel = client->channel;
 client->cleanup(client);
-red_channel_client_disconnect(worker->connection->channel_client);
-worker->connection->channel_client = NULL;
+red_channel_client_disconnect(channel->connection->channel_client);
+channel->connection->channel_client = NULL;
 reds_core_watch_remove(reds, client->stream->watch);
 client->stream->watch = NULL;
 reds_stream_free(client->stream);
 client->stream = NULL;
 spice_marshaller_destroy(client->send_data.marshaller);
 snd_channel_unref(client);
-worker->connection = NULL;
+channel->connection = NULL;
 }
 
 static void snd_playback_free_frame(PlaybackChannelClient *playback_client, 
AudioFrame *frame)
@@ -384,11 +384,11 @@ static int snd_record_handle_message(SndChannelClient 
*client, size_t size, uint
 return snd_record_handle_write((RecordChannelClient *)client, size, 
message);
 case SPICE_MSGC_RECORD_MODE: {
 SpiceMsgcRecordMode *mode = (SpiceMsgcRecordMode *)message;
-SndWorker *worker = client->worker;
+SndChannel *channel = client->channel;
 record_client->mode_time = mode->time;
 if (mode->mode != SPICE_AUDIO_DATA_MODE_RAW) {
-if (snd_codec_is_capable(mode->mode, worker->frequency)) {
-if (snd_codec_create(_client->codec, mode->mode, 
worker->frequency,
+if (snd_codec_is_capable(mode->mode, channel->frequency)) {
+if (snd_codec_create(_client->codec, mode->mode, 
channel->frequency,
  SND_CODEC_DECODE) == SND_CODEC_OK) {
 record_client->mode = mode->mode;
 } else {
@@ -571,7 +571,7 @@ static int snd_send_volume(SndChannelClient *client, 
uint32_t cap, int msg)
 {
 SpiceMsgAudioVolume *vol;
 uint8_t c;
-SpiceVolumeState *st = >worker->volume;
+SpiceVolumeState *st = >channel->volume;
 
 if 

Re: [Spice-devel] [PATCH v2 07/19] Return invalid value from very old obsolete functions

[Pavel Grunt]

Hi,

Why not return -1 in spice_server_get_sock_info() and
spice_server_get_peer_info() and remove these main channel functions.

Also the public function should be marked as deprecated if their usage
 should be avoided

Pavel

On Fri, 2016-11-25 at 14:52 +, Frediano Ziglio wrote:
> These functions are not used since years and are not supporting
> multiple clients.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/main-channel.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/server/main-channel.c b/server/main-channel.c
> index 37b2f25..3d107c4 100644
> --- a/server/main-channel.c
> +++ b/server/main-channel.c
> @@ -306,16 +306,16 @@ MainChannelClient
> *main_channel_link(MainChannel *channel, RedClient *client,
>  return mcc;
>  }
>  
> +/* very obsolete and old function, retain only for ABI */
>  int main_channel_getsockname(MainChannel *main_chan, struct
> sockaddr *sa, socklen_t *salen)
>  {
> -return main_chan ?
> -getsockname(red_channel_get_first_socket(RED_CHANNEL(main_c
> han)), sa, salen) : -1;
> +return -1;
>  }
>  
> +/* very obsolete and old function, retain only for ABI */
>  int main_channel_getpeername(MainChannel *main_chan, struct
> sockaddr *sa, socklen_t *salen)
>  {
> -return main_chan ?
> -getpeername(red_channel_get_first_socket(RED_CHANNEL(main_c
> han)), sa, salen) : -1;
> +return -1;
>  }
>  
>  MainChannel* main_channel_new(RedsState *reds)
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v4 14/14] sound: Reuse code for snd_set_{playback, record}_peer

[Frediano Ziglio]

Almost identical beside the type.

Signed-off-by: Frediano Ziglio 
---
 server/sound.c | 53 +++
 1 file changed, 16 insertions(+), 37 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index 486fc95..0304fec 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -1071,9 +1071,9 @@ playback_channel_client_constructed(GObject *object)
 snd_send(SND_CHANNEL_CLIENT(playback_client));
 }
 
-static void snd_set_playback_peer(RedChannel *red_channel, RedClient *client, 
RedsStream *stream,
-  int migration, int num_common_caps, uint32_t 
*common_caps,
-  int num_caps, uint32_t *caps)
+static void snd_set_peer(RedChannel *red_channel, RedClient *client, 
RedsStream *stream,
+ int migration, int num_common_caps, uint32_t 
*common_caps,
+ int num_caps, uint32_t *caps, GType type)
 {
 SndChannel *channel = SND_CHANNEL(red_channel);
 GArray *common_caps_array = NULL, *caps_array = NULL;
@@ -1093,7 +1093,7 @@ static void snd_set_playback_peer(RedChannel 
*red_channel, RedClient *client, Re
 g_array_append_vals(caps_array, caps, num_caps);
 }
 
-g_initable_new(TYPE_PLAYBACK_CHANNEL_CLIENT,
+g_initable_new(type,
NULL, NULL,
"channel", channel,
"client", client,
@@ -1110,6 +1110,15 @@ static void snd_set_playback_peer(RedChannel 
*red_channel, RedClient *client, Re
 }
 }
 
+static void snd_set_playback_peer(RedChannel *red_channel, RedClient *client, 
RedsStream *stream,
+  int migration, int num_common_caps, uint32_t 
*common_caps,
+  int num_caps, uint32_t *caps)
+{
+snd_set_peer(red_channel, client, stream, migration,
+ num_common_caps, common_caps, num_caps, caps,
+ TYPE_PLAYBACK_CHANNEL_CLIENT);
+}
+
 static void snd_record_migrate_channel_client(RedChannelClient *rcc)
 {
 SndChannel *channel;
@@ -1328,39 +1337,9 @@ static void snd_set_record_peer(RedChannel *red_channel, 
RedClient *client, Reds
 int migration, int num_common_caps, uint32_t 
*common_caps,
 int num_caps, uint32_t *caps)
 {
-SndChannel *channel = SND_CHANNEL(red_channel);
-GArray *common_caps_array = NULL, *caps_array = NULL;
-
-if (channel->connection) {
-red_channel_client_disconnect(RED_CHANNEL_CLIENT(channel->connection));
-channel->connection = NULL;
-}
-
-if (common_caps) {
-common_caps_array = g_array_sized_new(FALSE, FALSE, sizeof 
(*common_caps),
-  num_common_caps);
-g_array_append_vals(common_caps_array, common_caps, num_common_caps);
-}
-if (caps) {
-caps_array = g_array_sized_new(FALSE, FALSE, sizeof (*caps), num_caps);
-g_array_append_vals(caps_array, caps, num_caps);
-}
-
-g_initable_new(TYPE_RECORD_CHANNEL_CLIENT,
-   NULL, NULL,
-   "channel", channel,
-   "client", client,
-   "stream", stream,
-   "caps", caps_array,
-   "common-caps", common_caps_array,
-   NULL);
-
-if (caps_array) {
-g_array_unref(caps_array);
-}
-if (common_caps_array) {
-g_array_unref(common_caps_array);
-}
+snd_set_peer(red_channel, client, stream, migration,
+ num_common_caps, common_caps, num_caps, caps,
+ TYPE_RECORD_CHANNEL_CLIENT);
 }
 
 static void snd_playback_migrate_channel_client(RedChannelClient *rcc)
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v4 03/14] sound: Rename {Record, Playback}Channel to *ChannelClient

[Frediano Ziglio]

Make easier to understand that they refer to client and not
all channel.

Specifically:
- RecordChannel -> RecordChannelClient
- PlaybackChannel -> PlaybackChannelClient
- playback_channel -> playback_client
- record_channel -> record_client

Signed-off-by: Frediano Ziglio 
---
 server/sound.c | 368 +-
 1 file changed, 184 insertions(+), 184 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index 02adf79..bafdd1e 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -116,17 +116,17 @@ struct SndChannelClient {
 snd_channel_cleanup_channel_proc cleanup;
 };
 
-typedef struct PlaybackChannel PlaybackChannel;
+typedef struct PlaybackChannelClient PlaybackChannelClient;
 
 typedef struct AudioFrame AudioFrame;
 struct AudioFrame {
 uint32_t time;
 uint32_t samples[SND_CODEC_MAX_FRAME_SIZE];
-PlaybackChannel *channel;
+PlaybackChannelClient *client;
 AudioFrame *next;
 };
 
-struct PlaybackChannel {
+struct PlaybackChannelClient {
 SndChannelClient base;
 AudioFrame frames[3];
 AudioFrame *free_frames;
@@ -163,7 +163,7 @@ struct SpiceRecordState {
 struct SndWorker worker;
 };
 
-typedef struct RecordChannel {
+typedef struct RecordChannelClient {
 SndChannelClient base;
 uint32_t samples[RECORD_SAMPLES_SIZE];
 uint32_t write_pos;
@@ -173,7 +173,7 @@ typedef struct RecordChannel {
 uint32_t start_time;
 SndCodec codec;
 uint8_t  decode_buf[SND_CODEC_MAX_FRAME_BYTES];
-} RecordChannel;
+} RecordChannelClient;
 
 /* A list of all Spice{Playback,Record}State objects */
 static SndWorker *workers;
@@ -233,20 +233,20 @@ static void snd_disconnect_channel(SndChannelClient 
*client)
 worker->connection = NULL;
 }
 
-static void snd_playback_free_frame(PlaybackChannel *playback_channel, 
AudioFrame *frame)
+static void snd_playback_free_frame(PlaybackChannelClient *playback_client, 
AudioFrame *frame)
 {
-frame->channel = playback_channel;
-frame->next = playback_channel->free_frames;
-playback_channel->free_frames = frame;
+frame->client = playback_client;
+frame->next = playback_client->free_frames;
+playback_client->free_frames = frame;
 }
 
 static void snd_playback_on_message_done(SndChannelClient *client)
 {
-PlaybackChannel *playback_channel = (PlaybackChannel *)client;
-if (playback_channel->in_progress) {
-snd_playback_free_frame(playback_channel, 
playback_channel->in_progress);
-playback_channel->in_progress = NULL;
-if (playback_channel->pending_frame) {
+PlaybackChannelClient *playback_client = (PlaybackChannelClient *)client;
+if (playback_client->in_progress) {
+snd_playback_free_frame(playback_client, playback_client->in_progress);
+playback_client->in_progress = NULL;
+if (playback_client->pending_frame) {
 client->command |= SND_PLAYBACK_PCM_MASK;
 }
 }
@@ -311,7 +311,7 @@ static int snd_send_data(SndChannelClient *client)
 return TRUE;
 }
 
-static int snd_record_handle_write(RecordChannel *record_channel, size_t size, 
void *message)
+static int snd_record_handle_write(RecordChannelClient *record_client, size_t 
size, void *message)
 {
 SpiceMsgcRecordPacket *packet;
 uint32_t write_pos;
@@ -319,39 +319,39 @@ static int snd_record_handle_write(RecordChannel 
*record_channel, size_t size, v
 uint32_t len;
 uint32_t now;
 
-if (!record_channel) {
+if (!record_client) {
 return FALSE;
 }
 
 packet = (SpiceMsgcRecordPacket *)message;
 
-if (record_channel->mode == SPICE_AUDIO_DATA_MODE_RAW) {
+if (record_client->mode == SPICE_AUDIO_DATA_MODE_RAW) {
 data = (uint32_t *)packet->data;
 size = packet->data_size >> 2;
 size = MIN(size, RECORD_SAMPLES_SIZE);
  } else {
 int decode_size;
-decode_size = sizeof(record_channel->decode_buf);
-if (snd_codec_decode(record_channel->codec, packet->data, 
packet->data_size,
-record_channel->decode_buf, _size) != SND_CODEC_OK)
+decode_size = sizeof(record_client->decode_buf);
+if (snd_codec_decode(record_client->codec, packet->data, 
packet->data_size,
+record_client->decode_buf, _size) != SND_CODEC_OK)
 return FALSE;
-data = (uint32_t *) record_channel->decode_buf;
+data = (uint32_t *) record_client->decode_buf;
 size = decode_size >> 2;
 }
 
-write_pos = record_channel->write_pos % RECORD_SAMPLES_SIZE;
-record_channel->write_pos += size;
+write_pos = record_client->write_pos % RECORD_SAMPLES_SIZE;
+record_client->write_pos += size;
 len = RECORD_SAMPLES_SIZE - write_pos;
 now = MIN(len, size);
 size -= now;
-memcpy(record_channel->samples + write_pos, data, now << 2);
+memcpy(record_client->samples + write_pos, data, now << 2);
 
 if (size) {
-

[Spice-devel] [PATCH v4 13/14] sound: Use default disconnect for client channels

[Frediano Ziglio]

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

diff --git a/server/sound.c b/server/sound.c
index ebbe821..486fc95 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -804,25 +804,6 @@ snd_channel_client_release_recv_buf(RedChannelClient *rcc, 
uint16_t type, uint32
 }
 }
 
-// TODO remove and just use red_channel_client_disconnect ??
-static void snd_disconnect_channel_client(RedChannelClient *rcc)
-{
-SndChannel *channel;
-RedChannel *red_channel = red_channel_client_get_channel(rcc);
-uint32_t type;
-
-spice_assert(red_channel);
-channel = SND_CHANNEL(red_channel);
-spice_assert(channel);
-g_object_get(red_channel, "channel-type", , NULL);
-
-spice_debug("channel-type=%d", type);
-if (channel->connection) {
-spice_assert(RED_CHANNEL_CLIENT(channel->connection) == rcc);
-red_channel_client_disconnect(rcc);
-}
-}
-
 static void snd_set_command(SndChannelClient *client, uint32_t command)
 {
 if (!client) {
@@ -1466,7 +1447,6 @@ playback_channel_constructed(GObject *object)
 G_OBJECT_CLASS(playback_channel_parent_class)->constructed(object);
 
 client_cbs.connect = snd_set_playback_peer;
-client_cbs.disconnect = snd_disconnect_channel_client;
 client_cbs.migrate = snd_playback_migrate_channel_client;
 red_channel_register_client_cbs(RED_CHANNEL(self), _cbs, self);
 
@@ -1517,7 +1497,6 @@ record_channel_constructed(GObject *object)
 G_OBJECT_CLASS(record_channel_parent_class)->constructed(object);
 
 client_cbs.connect = snd_set_record_peer;
-client_cbs.disconnect = snd_disconnect_channel_client;
 client_cbs.migrate = snd_record_migrate_channel_client;
 red_channel_register_client_cbs(RED_CHANNEL(self), _cbs, self);
 
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v4 08/14] sound: Implement on_disconnect RedChannel callback

[Frediano Ziglio]

Avoid having dandling pointer to a client.

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

diff --git a/server/sound.c b/server/sound.c
index a5b960b..00eab67 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -773,6 +773,14 @@ static int snd_channel_config_socket(RedChannelClient *rcc)
 return TRUE;
 }
 
+static void snd_channel_on_disconnect(RedChannelClient *rcc)
+{
+SndChannel *channel = SND_CHANNEL(red_channel_client_get_channel(rcc));
+if (channel->connection && rcc == RED_CHANNEL_CLIENT(channel->connection)) 
{
+channel->connection = NULL;
+}
+}
+
 static uint8_t*
 snd_channel_client_alloc_recv_buf(RedChannelClient *rcc, uint16_t type, 
uint32_t size)
 {
@@ -1415,6 +1423,7 @@ snd_channel_class_init(SndChannelClass *klass)
 channel_class->config_socket = snd_channel_config_socket;
 channel_class->alloc_recv_buf = snd_channel_client_alloc_recv_buf;
 channel_class->release_recv_buf = snd_channel_client_release_recv_buf;
+channel_class->on_disconnect = snd_channel_on_disconnect;
 }
 
 static void
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v4 01/14] sound: Use worker directly

[Frediano Ziglio]

SpicePlaybackState and SpiceRecordState have same structures
changing only slightly the behaviour.
Using SndWorker instead allows some minor simplification and
more code reuse.

Signed-off-by: Frediano Ziglio 
---
 server/sound.c | 26 --
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index 8eddd2b..beab473 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -179,8 +179,8 @@ typedef struct RecordChannel {
 static SndWorker *workers;
 
 static void snd_receive(SndChannel *channel);
-static void snd_playback_start(SpicePlaybackState *st);
-static void snd_record_start(SpiceRecordState *st);
+static void snd_playback_start(SndWorker *worker);
+static void snd_record_start(SndWorker *worker);
 
 static SndChannel *snd_channel_ref(SndChannel *channel)
 {
@@ -1026,11 +1026,11 @@ SPICE_GNUC_VISIBLE void 
spice_server_playback_set_mute(SpicePlaybackInstance *si
 snd_playback_send_mute(playback_channel);
 }
 
-static void snd_playback_start(SpicePlaybackState *st)
+static void snd_playback_start(SndWorker *worker)
 {
-SndChannel *channel = st->worker.connection;
+SndChannel *channel = worker->connection;
 
-st->worker.active = 1;
+worker->active = 1;
 if (!channel)
 return;
 spice_assert(!channel->active);
@@ -1046,7 +1046,7 @@ static void snd_playback_start(SpicePlaybackState *st)
 
 SPICE_GNUC_VISIBLE void spice_server_playback_start(SpicePlaybackInstance *sin)
 {
-return snd_playback_start(sin->st);
+return snd_playback_start(>st->worker);
 }
 
 SPICE_GNUC_VISIBLE void spice_server_playback_stop(SpicePlaybackInstance *sin)
@@ -1195,7 +1195,6 @@ static void snd_set_playback_peer(RedChannel *channel, 
RedClient *client, RedsSt
 {
 SndWorker *worker = g_object_get_data(G_OBJECT(channel), "sound-worker");
 PlaybackChannel *playback_channel;
-SpicePlaybackState *st = SPICE_CONTAINEROF(worker, SpicePlaybackState, 
worker);
 
 snd_disconnect_channel(worker->connection);
 
@@ -1240,7 +1239,7 @@ static void snd_set_playback_peer(RedChannel *channel, 
RedClient *client, RedsSt
 }
 
 if (worker->active) {
-snd_playback_start(st);
+snd_playback_start(worker);
 }
 snd_playback_send(worker->connection);
 }
@@ -1294,12 +1293,12 @@ SPICE_GNUC_VISIBLE void 
spice_server_record_set_mute(SpiceRecordInstance *sin, u
 snd_record_send_mute(record_channel);
 }
 
-static void snd_record_start(SpiceRecordState *st)
+static void snd_record_start(SndWorker *worker)
 {
-SndChannel *channel = st->worker.connection;
+SndChannel *channel = worker->connection;
 RecordChannel *record_channel = SPICE_CONTAINEROF(channel, RecordChannel, 
base);
 
-st->worker.active = 1;
+worker->active = 1;
 if (!channel)
 return;
 spice_assert(!channel->active);
@@ -1316,7 +1315,7 @@ static void snd_record_start(SpiceRecordState *st)
 
 SPICE_GNUC_VISIBLE void spice_server_record_start(SpiceRecordInstance *sin)
 {
-snd_record_start(sin->st);
+snd_record_start(>st->worker);
 }
 
 SPICE_GNUC_VISIBLE void spice_server_record_stop(SpiceRecordInstance *sin)
@@ -1442,7 +1441,6 @@ static void snd_set_record_peer(RedChannel *channel, 
RedClient *client, RedsStre
 {
 SndWorker *worker = g_object_get_data(G_OBJECT(channel), "sound-worker");
 RecordChannel *record_channel;
-SpiceRecordState *st = SPICE_CONTAINEROF(worker, SpiceRecordState, worker);
 
 snd_disconnect_channel(worker->connection);
 
@@ -1465,7 +1463,7 @@ static void snd_set_record_peer(RedChannel *channel, 
RedClient *client, RedsStre
 
 on_new_record_channel(worker, _channel->base);
 if (worker->active) {
-snd_record_start(st);
+snd_record_start(worker);
 }
 snd_record_send(worker->connection);
 }
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v4 06/14] sound: Implements config_socket RedChannel callback

[Frediano Ziglio]

This code is the same inside __new_channel but will set the
RedsStream from RedChannel.

Signed-off-by: Frediano Ziglio 
---
 server/sound.c | 51 +++-
 1 file changed, 51 insertions(+)

diff --git a/server/sound.c b/server/sound.c
index de46be5..83a87c9 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -999,6 +999,55 @@ error1:
 return NULL;
 }
 
+static int snd_channel_config_socket(RedChannelClient *rcc)
+{
+int delay_val;
+int flags;
+#ifdef SO_PRIORITY
+int priority;
+#endif
+int tos;
+RedsStream *stream = red_channel_client_get_stream(rcc);
+RedClient *red_client = red_channel_client_get_client(rcc);
+MainChannelClient *mcc = red_client_get_main(red_client);
+
+if ((flags = fcntl(stream->socket, F_GETFL)) == -1) {
+spice_printerr("accept failed, %s", strerror(errno));
+return FALSE;
+}
+
+#ifdef SO_PRIORITY
+priority = 6;
+if (setsockopt(stream->socket, SOL_SOCKET, SO_PRIORITY, (void*),
+   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*), 
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, _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));
+return FALSE;
+}
+
+return TRUE;
+}
+
 static void snd_disconnect_channel_client(RedChannelClient *rcc)
 {
 SndChannel *channel;
@@ -1543,6 +1592,8 @@ snd_channel_init(SndChannel *self)
 static void
 snd_channel_class_init(SndChannelClass *klass)
 {
+RedChannelClass *channel_class = RED_CHANNEL_CLASS(klass);
+channel_class->config_socket = snd_channel_config_socket;
 }
 
 static void
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v4 02/14] sound: Rename SndChannel to SndChannelClient

[Frediano Ziglio]

SndWorker has been historically based on RedChannel, initial git commit
has:
struct SndWorker {
 Channel base;
 ...
};

SndChannel, contrary to what its name may imply is more focused on
marshalling/sending of sound data, which is the responsibility of
RedChannelClient for the other SPICE channels.

This commit and following ones make the naming of these 2 types more
consistent with how the rest of the code is structured.

Signed-off-by: Frediano Ziglio 
---
 server/sound.c | 522 +-
 1 file changed, 261 insertions(+), 261 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index beab473..02adf79 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -73,16 +73,16 @@ enum PlaybackCommand {
 #define SND_PLAYBACK_PCM_MASK (1 << SND_PLAYBACK_PCM)
 #define SND_PLAYBACK_LATENCY_MASK ( 1 << SND_PLAYBACK_LATENCY)
 
-typedef struct SndChannel SndChannel;
+typedef struct SndChannelClient SndChannelClient;
 typedef void (*snd_channel_send_messages_proc)(void *in_channel);
-typedef int (*snd_channel_handle_message_proc)(SndChannel *channel, size_t 
size, uint32_t type, void *message);
-typedef void (*snd_channel_on_message_done_proc)(SndChannel *channel);
-typedef void (*snd_channel_cleanup_channel_proc)(SndChannel *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);
 
 typedef struct SndWorker SndWorker;
 
-/* Connects an audio channel to a Spice client */
-struct SndChannel {
+/* Connects an audio client to a Spice client */
+struct SndChannelClient {
 RedsStream *stream;
 SndWorker *worker;
 spice_parse_channel_func_t parser;
@@ -127,7 +127,7 @@ struct AudioFrame {
 };
 
 struct PlaybackChannel {
-SndChannel base;
+SndChannelClient base;
 AudioFrame frames[3];
 AudioFrame *free_frames;
 AudioFrame *in_progress;   /* Frame being sent to the client */
@@ -147,7 +147,7 @@ typedef struct SpiceVolumeState {
 /* Base class for SpicePlaybackState and SpiceRecordState */
 struct SndWorker {
 RedChannel *base_channel;
-SndChannel *connection; /* Only one client is supported */
+SndChannelClient *connection; /* Only one client is supported */
 SndWorker *next; /* For the global SndWorker list */
 
 int active;
@@ -164,7 +164,7 @@ struct SpiceRecordState {
 };
 
 typedef struct RecordChannel {
-SndChannel base;
+SndChannelClient base;
 uint32_t samples[RECORD_SAMPLES_SIZE];
 uint32_t write_pos;
 uint32_t read_pos;
@@ -178,58 +178,58 @@ typedef struct RecordChannel {
 /* A list of all Spice{Playback,Record}State objects */
 static SndWorker *workers;
 
-static void snd_receive(SndChannel *channel);
+static void snd_receive(SndChannelClient *client);
 static void snd_playback_start(SndWorker *worker);
 static void snd_record_start(SndWorker *worker);
 
-static SndChannel *snd_channel_ref(SndChannel *channel)
+static SndChannelClient *snd_channel_ref(SndChannelClient *client)
 {
-channel->refs++;
-return channel;
+client->refs++;
+return client;
 }
 
-static SndChannel *snd_channel_unref(SndChannel *channel)
+static SndChannelClient *snd_channel_unref(SndChannelClient *client)
 {
-if (!--channel->refs) {
-spice_printerr("SndChannel=%p freed", channel);
-free(channel);
+if (!--client->refs) {
+spice_printerr("SndChannelClient=%p freed", client);
+free(client);
 return NULL;
 }
-return channel;
+return client;
 }
 
-static RedsState* snd_channel_get_server(SndChannel *channel)
+static RedsState* snd_channel_get_server(SndChannelClient *client)
 {
-g_return_val_if_fail(channel != NULL, NULL);
-return red_channel_get_server(channel->worker->base_channel);
+g_return_val_if_fail(client != NULL, NULL);
+return red_channel_get_server(client->worker->base_channel);
 }
 
-static void snd_disconnect_channel(SndChannel *channel)
+static void snd_disconnect_channel(SndChannelClient *client)
 {
 SndWorker *worker;
 RedsState *reds;
 RedChannel *red_channel;
 uint32_t type;
 
-if (!channel || !channel->stream) {
+if (!client || !client->stream) {
 spice_debug("not connected");
 return;
 }
-red_channel = red_channel_client_get_channel(channel->channel_client);
-reds = snd_channel_get_server(channel);
+red_channel = red_channel_client_get_channel(client->channel_client);
+reds = snd_channel_get_server(client);
 g_object_get(red_channel, "channel-type", , NULL);
-spice_debug("SndChannel=%p rcc=%p type=%d",
- channel, channel->channel_client, type);
-worker = channel->worker;
-channel->cleanup(channel);
+spice_debug("SndChannelClient=%p rcc=%p type=%d",
+ client, 

[Spice-devel] [PATCH v4 10/14] sound: Reduce message buffer

[Frediano Ziglio]

Sound messages are not that big.
This limit RecordChannelClient to 64K so the entire
structure can be a GObject (which are currently limited
to 64K).

Signed-off-by: Frediano Ziglio 
---
 server/sound.c | 14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index 00eab67..0606fa0 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -98,7 +98,9 @@ struct SndChannelClient {
 
 uint32_t command;
 
-uint8_t receive_buf[SND_RECEIVE_BUF_SIZE];
+/* 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;
 
 snd_channel_on_message_done_proc on_message_done;
@@ -785,8 +787,10 @@ static uint8_t*
 snd_channel_client_alloc_recv_buf(RedChannelClient *rcc, uint16_t type, 
uint32_t size)
 {
 SndChannelClient *client = SND_CHANNEL_CLIENT(rcc);
-// TODO isn't too much the buffer ?? return NULL ??
-spice_assert(size < sizeof(client->receive_buf));
+// If message is to big allocate one, this should never happen
+if (size > sizeof(client->receive_buf)) {
+return spice_malloc(size);
+}
 return client->receive_buf;
 }
 
@@ -794,6 +798,10 @@ static void
 snd_channel_client_release_recv_buf(RedChannelClient *rcc, uint16_t type, 
uint32_t size,
 uint8_t *msg)
 {
+SndChannelClient *client = SND_CHANNEL_CLIENT(rcc);
+if (msg != client->receive_buf) {
+free(msg);
+}
 }
 
 // TODO remove and just use red_channel_client_disconnect ??
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v4 00/14] Remove DummyChannel* objects

[Frediano Ziglio]

These objects were used by the sound channel as
this channel read/write to/from client directly.
This make the code of this channel quite different
from the other ones.
Also this reduce code duplication from RedChannelClient
and increase encapsulation.
First set of patches attempt to rename fields/structures
to prepare for the conversion to RedChannel/RedChannelClient
and GObject.
Than other patches basically do the conversion and
some cleanup.

Changes since v3:
- updated some commit message;
- rename structures and related fields in the same commit.
- reverted wrong field rename (RedChannel *client).

Changes since v2:
- send playback data as soon as possible;
- split renames putting client renames first then worker ones;
- some commit message changes.

Changes since v1:
- split some long lines;
- do not attempt to free in progress frame before is full sent.
  This could cause raw frames to be overridden.

Frediano Ziglio (14):
  sound: Use worker directly
  sound: Rename SndChannel to SndChannelClient
  sound: Rename {Record,Playback}Channel to *ChannelClient
  sound: Rename SndWorker to SndChannel
  sound: Convert SndChannel to GObject
  sound: Implements config_socket RedChannel callback
  sound: Convert SndChannelClient to GObject
  sound: Implement on_disconnect RedChannel callback
  Remove DummyChannel* objects
  sound: Reduce message buffer
  Make RedChannelClient::incoming private
  sound: Free more on SndChannel finalize
  sound: Use default disconnect for client channels
  sound: Reuse code for snd_set_{playback,record}_peer

 server/Makefile.am  |4 +-
 server/dummy-channel-client.c   |  138 +--
 server/dummy-channel-client.h   |   64 +-
 server/dummy-channel.c  |   94 +--
 server/dummy-channel.h  |   60 +-
 server/red-channel-client-private.h |   11 +-
 server/red-channel-client.c |   12 +-
 server/red-channel-client.h |   13 +-
 server/sound.c  | 1701 +---
 9 files changed, 858 insertions(+), 1239 deletions(-)
 delete mode 100644 server/dummy-channel-client.c
 delete mode 100644 server/dummy-channel-client.h
 delete mode 100644 server/dummy-channel.c
 delete mode 100644 server/dummy-channel.h

base-commit: 29d6642430a21eba53a11f5120590d13a857edf4
-- 
git-series 0.9.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v4 09/14] Remove DummyChannel* objects

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/Makefile.am|   4 +-
 server/dummy-channel-client.c | 138 +---
 server/dummy-channel-client.h |  64 +
 server/dummy-channel.c|  94 +
 server/dummy-channel.h|  60 +---
 5 files changed, 360 deletions(-)
 delete mode 100644 server/dummy-channel-client.c
 delete mode 100644 server/dummy-channel-client.h
 delete mode 100644 server/dummy-channel.c
 delete mode 100644 server/dummy-channel.h

diff --git a/server/Makefile.am b/server/Makefile.am
index ae79ed7..1442bf1 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -104,10 +104,6 @@ libserver_la_SOURCES = \
red-channel-client-private.h\
red-client.c\
red-client.h\
-   dummy-channel.c \
-   dummy-channel.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 61d5683..000
--- a/server/dummy-channel-client.c
+++ /dev/null
@@ -1,138 +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, _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", , "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 = dummy_channel_client_disconnect;
-}
-
-static void
-dummy_channel_client_init(DummyChannelClient *self)
-{
-self->priv = 

[Spice-devel] [PATCH v4 05/14] sound: Convert SndChannel to GObject

[Frediano Ziglio]

NOTE: this is not supposed to run!

Just make easy the review, should be squashed to following 2
patches.

Signed-off-by: Frediano Ziglio 
---
 server/sound.c | 195 +-
 1 file changed, 130 insertions(+), 65 deletions(-)

diff --git a/server/sound.c b/server/sound.c
index 5b66f3a..de46be5 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -144,9 +144,14 @@ typedef struct SpiceVolumeState {
 int mute;
 } SpiceVolumeState;
 
+#define TYPE_SND_CHANNEL snd_channel_get_type()
+#define SND_CHANNEL(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj), TYPE_SND_CHANNEL, 
SndChannel))
+GType snd_channel_get_type(void) G_GNUC_CONST;
+
 /* Base class for SpicePlaybackState and SpiceRecordState */
 struct SndChannel {
-RedChannel *base_channel;
+RedChannel parent;
+
 SndChannelClient *connection; /* Only one client is supported */
 SndChannel *next; /* For the global SndChannel list */
 
@@ -155,14 +160,40 @@ struct SndChannel {
 uint32_t frequency;
 };
 
+typedef RedChannelClass SndChannelClass;
+
+G_DEFINE_TYPE(SndChannel, snd_channel, RED_TYPE_CHANNEL)
+
+
+typedef struct SpicePlaybackState PlaybackChannel;
+typedef SndChannelClass PlaybackChannelClass;
+
+#define TYPE_PLAYBACK_CHANNEL playback_channel_get_type()
+#define PLAYBACK_CHANNEL(obj) \
+(G_TYPE_CHECK_INSTANCE_CAST((obj), TYPE_PLAYBACK_CHANNEL, PlaybackChannel))
+GType playback_channel_get_type(void) G_GNUC_CONST;
+
 struct SpicePlaybackState {
-struct SndChannel channel;
+SndChannel channel;
 };
 
+G_DEFINE_TYPE(PlaybackChannel, playback_channel, TYPE_SND_CHANNEL)
+
+
+typedef struct SpiceRecordState RecordChannel;
+typedef SndChannelClass RecordChannelClass;
+
+#define TYPE_RECORD_CHANNEL record_channel_get_type()
+#define RECORD_CHANNEL(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj), 
TYPE_RECORD_CHANNEL, RecordChannel))
+GType record_channel_get_type(void) G_GNUC_CONST;
+
 struct SpiceRecordState {
-struct SndChannel channel;
+SndChannel channel;
 };
 
+G_DEFINE_TYPE(RecordChannel, record_channel, TYPE_SND_CHANNEL)
+
+
 typedef struct RecordChannelClient {
 SndChannelClient base;
 uint32_t samples[RECORD_SAMPLES_SIZE];
@@ -201,7 +232,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(client->channel->base_channel);
+return red_channel_get_server(RED_CHANNEL(client->channel));
 }
 
 static void snd_disconnect_channel(SndChannelClient *client)
@@ -892,7 +923,7 @@ static SndChannelClient *__new_channel(SndChannel *channel, 
int size, uint32_t c
 #endif
 int tos;
 MainChannelClient *mcc = red_client_get_main(red_client);
-RedsState *reds = red_channel_get_server(channel->base_channel);
+RedsState *reds = red_channel_get_server(RED_CHANNEL(channel));
 
 spice_assert(stream);
 if ((flags = fcntl(stream->socket, F_GETFL)) == -1) {
@@ -953,7 +984,7 @@ static SndChannelClient *__new_channel(SndChannel *channel, 
int size, uint32_t c
 client->cleanup = cleanup;
 
 client->channel_client =
-dummy_channel_client_create(channel->base_channel, red_client,
+dummy_channel_client_create(RED_CHANNEL(channel), red_client,
 num_common_caps, common_caps, num_caps, 
caps);
 if (!client->channel_client) {
 goto error2;
@@ -975,7 +1006,7 @@ static void snd_disconnect_channel_client(RedChannelClient 
*rcc)
 uint32_t type;
 
 spice_assert(red_channel);
-channel = (SndChannel *)g_object_get_data(G_OBJECT(red_channel), 
"sound-channel");
+channel = SND_CHANNEL(red_channel);
 spice_assert(channel);
 g_object_get(red_channel, "channel-type", , NULL);
 
@@ -1126,7 +1157,7 @@ void snd_set_playback_latency(RedClient *client, uint32_t 
latency)
 
 for (; now; now = now->next) {
 uint32_t type;
-g_object_get(now->base_channel, "channel-type", , NULL);
+g_object_get(RED_CHANNEL(now), "channel-type", , NULL);
 if (type == SPICE_CHANNEL_PLAYBACK && now->connection &&
 red_channel_client_get_client(now->connection->channel_client) == 
client) {
 
@@ -1161,7 +1192,7 @@ static int snd_desired_audio_mode(int 
playback_compression, int frequency,
 
 static void on_new_playback_channel(SndChannel *channel, SndChannelClient 
*snd_channel)
 {
-RedsState *reds = red_channel_get_server(channel->base_channel);
+RedsState *reds = red_channel_get_server(RED_CHANNEL(channel));
 
 spice_assert(snd_channel);
 
@@ -1193,7 +1224,7 @@ static void snd_set_playback_peer(RedChannel 
*red_channel, RedClient *client, Re
   int migration, int num_common_caps, uint32_t 
*common_caps,
   int num_caps, uint32_t *caps)
 {
-SndChannel *channel = g_object_get_data(G_OBJECT(red_channel), 

Re: [Spice-devel] [PATCH v2 05/19] Do not leak mig_timer

[Pavel Grunt]

On Fri, 2016-11-25 at 14:52 +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
Acked-by: Pavel Grunt 
> ---
>  server/reds.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/server/reds.c b/server/reds.c
> index 1a812e4..bc0cc01 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -3594,6 +3594,10 @@ SPICE_GNUC_VISIBLE void
> spice_server_destroy(SpiceServer *reds)
>  }
>  reds_cleanup(reds);
>  
> +if (reds->mig_timer) {
> +reds_core_timer_remove(reds, reds->mig_timer);
> +}
> +
>  /* remove the server from the list of servers so that we don't
> attempt to
>   * free it again at exit */
>  pthread_mutex_lock(_reds_lock);
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v2 03/19] Free state on spice_server_destroy

[Pavel Grunt]

On Fri, 2016-11-25 at 14:52 +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
Acked-by: Pavel Grunt 
> ---
>  server/reds.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/server/reds.c b/server/reds.c
> index 1b1c737..c5e84ec 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -3603,6 +3603,7 @@ SPICE_GNUC_VISIBLE void
> spice_server_destroy(SpiceServer *reds)
>  #ifdef RED_STATISTICS
>  stat_file_free(reds->stat_file);
>  #endif
> +free(reds);
>  }
>  
>  SPICE_GNUC_VISIBLE spice_compat_version_t
> spice_get_current_compat_version(void)
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v2 02/19] Free statistic file on spice_server_destroy

[Pavel Grunt]

On Fri, 2016-11-25 at 14:52 +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
Acked-by: Pavel Grunt 
> ---
>  server/reds.c |  4 
>  server/stat-file.c| 17 +
>  server/stat-file.h|  1 +
>  server/tests/test-stat-file.c |  2 ++
>  4 files changed, 24 insertions(+)
> 
> diff --git a/server/reds.c b/server/reds.c
> index 0e68973..1b1c737 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -3599,6 +3599,10 @@ SPICE_GNUC_VISIBLE void
> spice_server_destroy(SpiceServer *reds)
>  pthread_mutex_lock(_reds_lock);
>  servers = g_list_remove(servers, reds);
>  pthread_mutex_unlock(_reds_lock);
> +
> +#ifdef RED_STATISTICS
> +stat_file_free(reds->stat_file);
> +#endif
>  }
>  
>  SPICE_GNUC_VISIBLE spice_compat_version_t
> spice_get_current_compat_version(void)
> diff --git a/server/stat-file.c b/server/stat-file.c
> index d1e25ce..beed77a 100644
> --- a/server/stat-file.c
> +++ b/server/stat-file.c
> @@ -80,6 +80,23 @@ cleanup:
>  return NULL;
>  }
>  
> +void stat_file_free(RedStatFile *stat_file)
> +{
> +if (!stat_file) {
> +return;
> +}
> +
> +stat_file_unlink(stat_file);
> +/* TODO other part of the code is not ready for this! */
> +#if 0
> +size_t shm_size = STAT_SHM_SIZE(stat_file->max_nodes);
> +munmap(stat_file->stat, shm_size);
> +#endif
> +
> +pthread_mutex_destroy(_file->lock);
> +free(stat_file);
> +}
> +
>  const char *stat_file_get_shm_name(RedStatFile *stat_file)
>  {
>  return stat_file->shm_name;
> diff --git a/server/stat-file.h b/server/stat-file.h
> index 903b68b..0454e2e 100644
> --- a/server/stat-file.h
> +++ b/server/stat-file.h
> @@ -27,6 +27,7 @@ typedef uint32_t StatNodeRef;
>  typedef struct RedStatFile RedStatFile;
>  
>  RedStatFile *stat_file_new(unsigned int max_nodes);
> +void stat_file_free(RedStatFile *stat_file);
>  void stat_file_unlink(RedStatFile *file_stat);
>  const char *stat_file_get_shm_name(RedStatFile *stat_file);
>  StatNodeRef stat_file_add_node(RedStatFile *stat_file, StatNodeRef
> parent,
> diff --git a/server/tests/test-stat-file.c b/server/tests/test-stat-
> file.c
> index 09b0c7a..40cf37d 100644
> --- a/server/tests/test-stat-file.c
> +++ b/server/tests/test-stat-file.c
> @@ -93,6 +93,8 @@ static void stat_file(void)
>  g_assert_null(stat_file_get_shm_name(stat_file));
>  g_assert_cmpint(access(filename, F_OK),==,-1);
>  free(filename);
> +
> +stat_file_free(stat_file);
>  }
>  
>  int main(int argc, char *argv[])
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v3 07/17] sound: Rename worker to channel

[Christophe Fergeau]

Same comment as before, I would move this and the previous patch down to
patch #5

Christophe

On Tue, Nov 29, 2016 at 02:57:07PM +, Frediano Ziglio wrote:
> No more sense to still call it worker.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/sound.c | 294 +-
>  1 file changed, 147 insertions(+), 147 deletions(-)
> 
> diff --git a/server/sound.c b/server/sound.c
> index 737f8b9..d9b69af 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -84,7 +84,7 @@ typedef struct SndChannel SndChannel;
>  /* Connects an audio client to a Spice client */
>  struct SndChannelClient {
>  RedsStream *stream;
> -SndChannel *worker;
> +SndChannel *channel;
>  spice_parse_channel_func_t parser;
>  int refs;
>  
> @@ -156,11 +156,11 @@ struct SndChannel {
>  };
>  
>  struct SpicePlaybackState {
> -struct SndChannel worker;
> +struct SndChannel channel;
>  };
>  
>  struct SpiceRecordState {
> -struct SndChannel worker;
> +struct SndChannel channel;
>  };
>  
>  typedef struct RecordChannelClient {
> @@ -179,8 +179,8 @@ typedef struct RecordChannelClient {
>  static SndChannel *snd_channels;
>  
>  static void snd_receive(SndChannelClient *client);
> -static void snd_playback_start(SndChannel *worker);
> -static void snd_record_start(SndChannel *worker);
> +static void snd_playback_start(SndChannel *channel);
> +static void snd_record_start(SndChannel *channel);
>  
>  static SndChannelClient *snd_channel_ref(SndChannelClient *client)
>  {
> @@ -201,12 +201,12 @@ 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(client->worker->base_channel);
> +return red_channel_get_server(client->channel->base_channel);
>  }
>  
>  static void snd_disconnect_channel(SndChannelClient *client)
>  {
> -SndChannel *worker;
> +SndChannel *channel;
>  RedsState *reds;
>  RedChannel *red_channel;
>  uint32_t type;
> @@ -220,17 +220,17 @@ static void snd_disconnect_channel(SndChannelClient 
> *client)
>  g_object_get(red_channel, "channel-type", , NULL);
>  spice_debug("SndChannelClient=%p rcc=%p type=%d",
>   client, client->channel_client, type);
> -worker = client->worker;
> +channel = client->channel;
>  client->cleanup(client);
> -red_channel_client_disconnect(worker->connection->channel_client);
> -worker->connection->channel_client = NULL;
> +red_channel_client_disconnect(channel->connection->channel_client);
> +channel->connection->channel_client = NULL;
>  reds_core_watch_remove(reds, client->stream->watch);
>  client->stream->watch = NULL;
>  reds_stream_free(client->stream);
>  client->stream = NULL;
>  spice_marshaller_destroy(client->send_data.marshaller);
>  snd_channel_unref(client);
> -worker->connection = NULL;
> +channel->connection = NULL;
>  }
>  
>  static void snd_playback_free_frame(PlaybackChannelClient *playback_client, 
> AudioFrame *frame)
> @@ -384,11 +384,11 @@ static int snd_record_handle_message(SndChannelClient 
> *client, size_t size, uint
>  return snd_record_handle_write((RecordChannelClient *)client, size, 
> message);
>  case SPICE_MSGC_RECORD_MODE: {
>  SpiceMsgcRecordMode *mode = (SpiceMsgcRecordMode *)message;
> -SndChannel *worker = client->worker;
> +SndChannel *channel = client->channel;
>  record_client->mode_time = mode->time;
>  if (mode->mode != SPICE_AUDIO_DATA_MODE_RAW) {
> -if (snd_codec_is_capable(mode->mode, worker->frequency)) {
> -if (snd_codec_create(_client->codec, mode->mode, 
> worker->frequency,
> +if (snd_codec_is_capable(mode->mode, channel->frequency)) {
> +if (snd_codec_create(_client->codec, mode->mode, 
> channel->frequency,
>   SND_CODEC_DECODE) == SND_CODEC_OK) {
>  record_client->mode = mode->mode;
>  } else {
> @@ -571,7 +571,7 @@ static int snd_send_volume(SndChannelClient *client, 
> uint32_t cap, int msg)
>  {
>  SpiceMsgAudioVolume *vol;
>  uint8_t c;
> -SpiceVolumeState *st = >worker->volume;
> +SpiceVolumeState *st = >channel->volume;
>  
>  if (!red_channel_client_test_remote_cap(client->channel_client, cap)) {
>  return TRUE;
> @@ -600,7 +600,7 @@ static int snd_playback_send_volume(PlaybackChannelClient 
> *playback_client)
>  static int snd_send_mute(SndChannelClient *client, uint32_t cap, int msg)
>  {
>  SpiceMsgAudioMute mute;
> -SpiceVolumeState *st = >worker->volume;
> +SpiceVolumeState *st = >channel->volume;
>  
>  if (!red_channel_client_test_remote_cap(client->channel_client, cap)) {
>  return TRUE;
> @@ -645,7 +645,7 @@ static 

Re: [Spice-devel] [PATCH v3 05/17] sound: Rename SndWorker to SndChannel

[Christophe Fergeau]

I would reuse part of the log from the 2nd commit to explain why thi
smakes sense..

Christophe

On Tue, Nov 29, 2016 at 02:57:05PM +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/sound.c | 68 +--
>  1 file changed, 34 insertions(+), 34 deletions(-)
> 
> diff --git a/server/sound.c b/server/sound.c
> index c3bb566..6650094 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -79,12 +79,12 @@ typedef int 
> (*snd_channel_handle_message_proc)(SndChannelClient *client, size_t 
>  typedef void (*snd_channel_on_message_done_proc)(SndChannelClient *client);
>  typedef void (*snd_channel_cleanup_channel_proc)(SndChannelClient *client);
>  
> -typedef struct SndWorker SndWorker;
> +typedef struct SndChannel SndChannel;
>  
>  /* Connects an audio client to a Spice client */
>  struct SndChannelClient {
>  RedsStream *stream;
> -SndWorker *worker;
> +SndChannel *worker;
>  spice_parse_channel_func_t parser;
>  int refs;
>  
> @@ -145,10 +145,10 @@ typedef struct SpiceVolumeState {
>  } SpiceVolumeState;
>  
>  /* Base class for SpicePlaybackState and SpiceRecordState */
> -struct SndWorker {
> +struct SndChannel {
>  RedChannel *base_channel;
>  SndChannelClient *connection; /* Only one client is supported */
> -SndWorker *next; /* For the global SndWorker list */
> +SndChannel *next; /* For the global SndChannel list */
>  
>  int active;
>  SpiceVolumeState volume;
> @@ -156,11 +156,11 @@ struct SndWorker {
>  };
>  
>  struct SpicePlaybackState {
> -struct SndWorker worker;
> +struct SndChannel worker;
>  };
>  
>  struct SpiceRecordState {
> -struct SndWorker worker;
> +struct SndChannel worker;
>  };
>  
>  typedef struct RecordChannelClient {
> @@ -176,11 +176,11 @@ typedef struct RecordChannelClient {
>  } RecordChannelClient;
>  
>  /* A list of all Spice{Playback,Record}State objects */
> -static SndWorker *workers;
> +static SndChannel *workers;
>  
>  static void snd_receive(SndChannelClient *client);
> -static void snd_playback_start(SndWorker *worker);
> -static void snd_record_start(SndWorker *worker);
> +static void snd_playback_start(SndChannel *worker);
> +static void snd_record_start(SndChannel *worker);
>  
>  static SndChannelClient *snd_channel_ref(SndChannelClient *client)
>  {
> @@ -206,7 +206,7 @@ static RedsState* snd_channel_get_server(SndChannelClient 
> *client)
>  
>  static void snd_disconnect_channel(SndChannelClient *client)
>  {
> -SndWorker *worker;
> +SndChannel *worker;
>  RedsState *reds;
>  RedChannel *red_channel;
>  uint32_t type;
> @@ -384,7 +384,7 @@ static int snd_record_handle_message(SndChannelClient 
> *client, size_t size, uint
>  return snd_record_handle_write((RecordChannelClient *)client, size, 
> message);
>  case SPICE_MSGC_RECORD_MODE: {
>  SpiceMsgcRecordMode *mode = (SpiceMsgcRecordMode *)message;
> -SndWorker *worker = client->worker;
> +SndChannel *worker = client->worker;
>  record_client->mode_time = mode->time;
>  if (mode->mode != SPICE_AUDIO_DATA_MODE_RAW) {
>  if (snd_codec_is_capable(mode->mode, worker->frequency)) {
> @@ -873,7 +873,7 @@ static void snd_record_send(void* data)
>  }
>  }
>  
> -static SndChannelClient *__new_channel(SndWorker *worker, int size, uint32_t 
> channel_id,
> +static SndChannelClient *__new_channel(SndChannel *worker, int size, 
> uint32_t channel_id,
>   RedClient *red_client,
>   RedsStream *stream,
>   int migrate,
> @@ -970,12 +970,12 @@ error1:
>  
>  static void snd_disconnect_channel_client(RedChannelClient *rcc)
>  {
> -SndWorker *worker;
> +SndChannel *worker;
>  RedChannel *channel = red_channel_client_get_channel(rcc);
>  uint32_t type;
>  
>  spice_assert(channel);
> -worker = (SndWorker *)g_object_get_data(G_OBJECT(channel), 
> "sound-worker");
> +worker = (SndChannel *)g_object_get_data(G_OBJECT(channel), 
> "sound-worker");
>  spice_assert(worker);
>  g_object_get(channel, "channel-type", , NULL);
>  
> @@ -1026,7 +1026,7 @@ SPICE_GNUC_VISIBLE void 
> spice_server_playback_set_mute(SpicePlaybackInstance *si
>  snd_playback_send_mute(playback_client);
>  }
>  
> -static void snd_playback_start(SndWorker *worker)
> +static void snd_playback_start(SndChannel *worker)
>  {
>  SndChannelClient *client = worker->connection;
>  
> @@ -1122,7 +1122,7 @@ SPICE_GNUC_VISIBLE void 
> spice_server_playback_put_samples(SpicePlaybackInstance 
>  
>  void snd_set_playback_latency(RedClient *client, uint32_t latency)
>  {
> -SndWorker *now = workers;
> +SndChannel *now = workers;
>  
>  for (; now; now = now->next) {
>  uint32_t type;
> @@ -1159,7 +1159,7 @@ static int snd_desired_audio_mode(int 
> playback_compression, 

Re: [Spice-devel] [PATCH v3 03/17] sound: Rename {Record, Playback}Channel to *ChannelClient

[Christophe Fergeau]

On Tue, Nov 29, 2016 at 02:57:03PM +, Frediano Ziglio wrote:
>  static void snd_record_start(SndWorker *worker)
>  {
>  SndChannelClient *channel = worker->connection;
> -RecordChannel *record_channel = SPICE_CONTAINEROF(channel, 
> RecordChannel, base);
> +RecordChannelClient *record_client = SPICE_CONTAINEROF(channel, 
> RecordChannelClient, base);
>  
>  worker->active = 1;
>  if (!channel)
>  return;
>  spice_assert(!channel->active);
> -record_channel->read_pos = record_channel->write_pos = 0;   //todo: 
> improve by
> +record_client->read_pos = record_client->write_pos = 0;   //todo: 
> improve by
>  //stream 
> generation

Alignment gets broken here



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 v3 04/17] sound: Rename channel to client if variable used for clients

[Christophe Fergeau]

I would split this among the 2nd and 3rd patches..

Christophe

On Tue, Nov 29, 2016 at 02:57:04PM +, Frediano Ziglio wrote:
> Due to object rename using channel to store a client is
> quite confusing.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/sound.c | 520 +-
>  1 file changed, 260 insertions(+), 260 deletions(-)
> 
> diff --git a/server/sound.c b/server/sound.c
> index 6c02754..c3bb566 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -75,13 +75,13 @@ enum PlaybackCommand {
>  
>  typedef struct SndChannelClient SndChannelClient;
>  typedef void (*snd_channel_send_messages_proc)(void *in_channel);
> -typedef int (*snd_channel_handle_message_proc)(SndChannelClient *channel, 
> size_t size, uint32_t type, void *message);
> -typedef void (*snd_channel_on_message_done_proc)(SndChannelClient *channel);
> -typedef void (*snd_channel_cleanup_channel_proc)(SndChannelClient *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);
>  
>  typedef struct SndWorker SndWorker;
>  
> -/* Connects an audio channel to a Spice client */
> +/* Connects an audio client to a Spice client */
>  struct SndChannelClient {
>  RedsStream *stream;
>  SndWorker *worker;
> @@ -122,7 +122,7 @@ typedef struct AudioFrame AudioFrame;
>  struct AudioFrame {
>  uint32_t time;
>  uint32_t samples[SND_CODEC_MAX_FRAME_SIZE];
> -PlaybackChannelClient *channel;
> +PlaybackChannelClient *client;
>  AudioFrame *next;
>  };
>  
> @@ -178,135 +178,135 @@ typedef struct RecordChannelClient {
>  /* A list of all Spice{Playback,Record}State objects */
>  static SndWorker *workers;
>  
> -static void snd_receive(SndChannelClient *channel);
> +static void snd_receive(SndChannelClient *client);
>  static void snd_playback_start(SndWorker *worker);
>  static void snd_record_start(SndWorker *worker);
>  
> -static SndChannelClient *snd_channel_ref(SndChannelClient *channel)
> +static SndChannelClient *snd_channel_ref(SndChannelClient *client)
>  {
> -channel->refs++;
> -return channel;
> +client->refs++;
> +return client;
>  }
>  
> -static SndChannelClient *snd_channel_unref(SndChannelClient *channel)
> +static SndChannelClient *snd_channel_unref(SndChannelClient *client)
>  {
> -if (!--channel->refs) {
> -spice_printerr("SndChannelClient=%p freed", channel);
> -free(channel);
> +if (!--client->refs) {
> +spice_printerr("SndChannelClient=%p freed", client);
> +free(client);
>  return NULL;
>  }
> -return channel;
> +return client;
>  }
>  
> -static RedsState* snd_channel_get_server(SndChannelClient *channel)
> +static RedsState* snd_channel_get_server(SndChannelClient *client)
>  {
> -g_return_val_if_fail(channel != NULL, NULL);
> -return red_channel_get_server(channel->worker->base_channel);
> +g_return_val_if_fail(client != NULL, NULL);
> +return red_channel_get_server(client->worker->base_channel);
>  }
>  
> -static void snd_disconnect_channel(SndChannelClient *channel)
> +static void snd_disconnect_channel(SndChannelClient *client)
>  {
>  SndWorker *worker;
>  RedsState *reds;
>  RedChannel *red_channel;
>  uint32_t type;
>  
> -if (!channel || !channel->stream) {
> +if (!client || !client->stream) {
>  spice_debug("not connected");
>  return;
>  }
> -red_channel = red_channel_client_get_channel(channel->channel_client);
> -reds = snd_channel_get_server(channel);
> +red_channel = red_channel_client_get_channel(client->channel_client);
> +reds = snd_channel_get_server(client);
>  g_object_get(red_channel, "channel-type", , NULL);
>  spice_debug("SndChannelClient=%p rcc=%p type=%d",
> - channel, channel->channel_client, type);
> -worker = channel->worker;
> -channel->cleanup(channel);
> + client, client->channel_client, type);
> +worker = client->worker;
> +client->cleanup(client);
>  red_channel_client_disconnect(worker->connection->channel_client);
>  worker->connection->channel_client = NULL;
> -reds_core_watch_remove(reds, channel->stream->watch);
> -channel->stream->watch = NULL;
> -reds_stream_free(channel->stream);
> -channel->stream = NULL;
> -spice_marshaller_destroy(channel->send_data.marshaller);
> -snd_channel_unref(channel);
> +reds_core_watch_remove(reds, client->stream->watch);
> +client->stream->watch = NULL;
> +reds_stream_free(client->stream);
> +client->stream = NULL;
> +spice_marshaller_destroy(client->send_data.marshaller);
> +snd_channel_unref(client);
>  worker->connection = NULL;
>  }
>  
>  static void 

Re: [Spice-devel] udscs: Improve the udscs API documentation

[Christophe Fergeau]

On Wed, Nov 30, 2016 at 09:00:00AM +0100, Francois Gouget wrote:
> Document what happens whenever passing a NULL server or connection
> pointer is allowed.
> Make it clear that only pathname Unix domain sockets are supported.
> Also document when no_types and the type_to_string array are used
> and what for.


Acked-by: Christophe Fergeau 
and pushed
(I made some minor changes, main one is replacing 'pathname unix domain
socket' with an extra sentence 'Only sockets bound to a pathname are
supported').

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] udscs: Improve the udscs API documentation

[Francois Gouget]

Document what happens whenever passing a NULL server or connection
pointer is allowed.
Make it clear that only pathname Unix domain sockets are supported.
Also document when no_types and the type_to_string array are used
and what for.

Signed-off-by: Francois Gouget 
---
 src/udscs.h | 58 +++---
 1 file changed, 47 insertions(+), 11 deletions(-)

diff --git a/src/udscs.h b/src/udscs.h
index 6e960f8..ed0e061 100644
--- a/src/udscs.h
+++ b/src/udscs.h
@@ -57,21 +57,38 @@ typedef void (*udscs_read_callback)(struct udscs_connection 
**connp,
  */
 typedef void (*udscs_disconnect_callback)(struct udscs_connection *conn);
 
-/* Connect to a unix domain socket named name. */
+/* Connect to the pathname unix domain socket specified by socketname.
+ *
+ * If debug is true then the events on this connection will be traced.
+ * This includes the incoming and outgoing message names. So when debug is true
+ * no_types must be set to the value of the highest valid message id + 1,
+ * and type_to_string must point to a string array of size no_types for
+ * converting the message ids to their names.
+ */
 struct udscs_connection *udscs_connect(const char *socketname,
 udscs_read_callback read_callback,
 udscs_disconnect_callback disconnect_callback,
 const char * const type_to_string[], int no_types, int debug);
 
-/* The contents of connp will be made NULL. */
+/* Closes the connection, releases the corresponding resources and
+ * sets *connp to NULL.
+ *
+ * Does nothing if *connp is NULL.
+ */
 void udscs_destroy_connection(struct udscs_connection **connp);
 
+/* Given a udscs client fill the fd_sets pointed to by readfds and
+ * writefds for select() usage.
+ * Return value: value of the highest fd + 1 or -1 if conn is NULL
+ */
 int udscs_client_fill_fds(struct udscs_connection *conn, fd_set *readfds,
 fd_set *writefds);
 
-/* Note the connection may be destroyed (when disconnected) by this call
- * in this case the disconnect calllback will get called before the
- * destruction and the contents of connp will be made NULL.
+/* Handle any events flagged by select for the given udscs client.
+ * Note that upon disconnection this will call the disconnect callback
+ * and then destroy the connection which will set *connp to NULL.
+ *
+ * Does nothing if *connp is NULL.
  */
 void udscs_client_handle_fds(struct udscs_connection **connp, fd_set *readfds,
 fd_set *writefds);
@@ -82,8 +99,12 @@ void udscs_client_handle_fds(struct udscs_connection 
**connp, fd_set *readfds,
 int udscs_write(struct udscs_connection *conn, uint32_t type, uint32_t arg1,
 uint32_t arg2, const uint8_t *data, uint32_t size);
 
-/* To associate per connection data with a connection. */
+/* Associates the specified user data with the connection. */
 void udscs_set_user_data(struct udscs_connection *conn, void *data);
+
+/* Returns the connection's associated user data.
+ * Returns NULL if conn is NULL.
+ */
 void *udscs_get_user_data(struct udscs_connection *conn);
 
 
@@ -98,13 +119,26 @@ struct udscs_server;
  */
 typedef void (*udscs_connect_callback)(struct udscs_connection *conn);
 
-/* Create a unix domain socket named name and start listening on it. */
+/* Create the pathname unix domain socket specified by socketname and
+ * start listening on it.
+ *
+ * If debug is true then the events on this socket and related individual
+ * connections will be traced.
+ * This includes the incoming and outgoing message names. So when debug is true
+ * no_types must be set to the value of the highest valid message id + 1,
+ * and type_to_string must point to a string array of size no_types for
+ * converting the message ids to their names.
+ */
 struct udscs_server *udscs_create_server(const char *socketname,
 udscs_connect_callback connect_callback,
 udscs_read_callback read_callback,
 udscs_disconnect_callback disconnect_callback,
 const char * const type_to_string[], int no_types, int debug);
 
+/* Closes all the server's connections and releases the corresponding
+ * resources.
+ * Does nothing if server is NULL.
+ */
 void udscs_destroy_server(struct udscs_server *server);
 
 /* Like udscs_write, but then send the message to all clients connected to
@@ -122,19 +156,21 @@ typedef int (*udscs_for_all_clients_callback)(struct 
udscs_connection **connp,
 
 /* Call func for all clients connected to the server, passing through
  * priv to all func calls. Returns the total of the return values from all
- * calls to func.
+ * calls to func or 0 if server is NULL.
  */
 int udscs_server_for_all_clients(struct udscs_server *server,
 udscs_for_all_clients_callback func, void *priv);
 
-/* Given an udscs server or client fill the fd_sets pointed to by readfds and
+/* Given a udscs server fill the fd_sets pointed to by readfds and
  * writefds for select() usage.
- * Return value: value of the highest fd + 1
+ * Return value: value of the