[Spice-devel] [PATCH v5 1/8] Bump GLib version to 2.34

2017-10-19 Thread Jakub Janků 
This is required for the following GMainLoop integration,
which utilizes some of the new functions/definitions, namely:

[definition][GLib version]
g_clear_pointer()   2.34
G_SOURCE_REMOVE 2.32
G_SOURCE_CONTINUE   2.32
g_unix_signal_add() 2.30

GLib version accross distributions:
- Fedora 26  2.52
- Fedora 25  2.50
- Fedora 24  2.48
- CentOS 7   2.46
- Debian 9   2.50

RHEL 6 is no longer supported,
so remove README.RHEL-5.

Acked-by: Frediano Ziglio 
---
 Makefile.am   |  1 -
 README.RHEL-5 | 34 --
 configure.ac  |  2 +-
 3 files changed, 1 insertion(+), 36 deletions(-)
 delete mode 100644 README.RHEL-5

diff --git a/Makefile.am b/Makefile.am
index 45f7177..4689713 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -121,7 +121,6 @@ manpage_DATA =  \
 
 EXTRA_DIST =   \
NEWS\
-   README.RHEL-5   \
data/70-spice-vdagentd.rules\
data/spice-vdagent.desktop  \
data/spice-vdagentd \
diff --git a/README.RHEL-5 b/README.RHEL-5
deleted file mode 100644
index 9b9ccc7..000
--- a/README.RHEL-5
+++ /dev/null
@@ -1,34 +0,0 @@
-spice-vdagent and RHEL-5 README

-
-spice-vdagent can also be used inside RHEL-5 guests, if, and only if, they
-are hosted on a host which uses virtio-serial as the agent channel. Note
-that older hosts, such as qemu shipped with RHEL-5 for example, use
-a custom pci device (the spicevmc device) for the agent channel and
-there is no Linux support for this device so spice-vdagent does not work
-inside guests hosted on such hosts! Hosts based on RHEL-6 or Fedora 15
-and newer for example, are fine.
-
-spice-vdagent relies on ConsoleKit to determine the active session, since
-RHEL-5 has no ConsoleKit it cannot do this on RHEL-5, therefor only 1
-active X-session is supported with RHEL-5. If you try to start multiple
-sessions by running multiple X-servers the agent will disable itself.
-
-Since the RHEL-5 X-server does not support hotplugging of input devices,
-spice-vdagentd must be started before the X-server, so that it can create
-the uinput tablet device it uses for agent mouse mode.
-
-This also means that you must use a customized xorg.conf to teach the
-X-server about the uinput device. An example xorg.conf is included with
-spice-vdagent. After installing spice-vdagent, first start spice-vdagentd,
-and then copy this file to /etc/X11/xorg.conf and restart your X-server.
-
-Note: the sample xorg.conf assumes that the vm has been configured to *not*
-use a usb tablet device, since there is no need for one with the agent and
-usb emulation takes a significant amount of CPU.
-
-Building from source for RHEL-5

-
-If you're building spice-vdagent from source on RHEL-5, you must pass
---disable-console-kit --enable-static-uinput to ./configure!
diff --git a/configure.ac b/configure.ac
index fbc20a9..d92b527 100644
--- a/configure.ac
+++ b/configure.ac
@@ -85,7 +85,7 @@ AC_ARG_ENABLE([static-uinput],
   [enable_static_uinput="$enableval"],
   [enable_static_uinput="no"])
 
-PKG_CHECK_MODULES([GLIB2], [glib-2.0 >= 2.28])
+PKG_CHECK_MODULES([GLIB2], [glib-2.0 >= 2.34])
 PKG_CHECK_MODULES(X, [xfixes xrandr >= 1.3 xinerama x11])
 PKG_CHECK_MODULES(SPICE, [spice-protocol >= 0.12.13])
 PKG_CHECK_MODULES(ALSA, [alsa >= 1.0.22])
-- 
2.13.6

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

Re: [Spice-devel] [PATCH spice-gtk v2] spicy: Add call of gst_deinit at program exit

2017-10-19 Thread Marc-André Lureau 
Hi

- Original Message -
> From: Christophe de Dinechin 
> 
> This is useful for some instrumentation, e.g. the leaks tracer,
> that perform some of their operations within gst_deinit.
> 
> Without this patch, if you run spicy with
>   GST_DEBUG="GST_TRACER:7" GST_TRACERS="leaks" spicy ...
> the leak tracer does not show any output, because it runs in gst_deinit.
> 
> Signed-off-by: Christophe de Dinechin 

Reviewed-by: Marc-André Lureau 

> ---
>  tools/spicy.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/spicy.c b/tools/spicy.c
> index 088e034..d79e504 100644
> --- a/tools/spicy.c
> +++ b/tools/spicy.c
> @@ -2068,5 +2068,8 @@ int main(int argc, char *argv[])
>  g_free(spicy_title);
>  
>  setup_terminal(true);
> +#if HAVE_GSTAUDIO || HAVE_GSTVIDEO
> +gst_deinit();
> +#endif
>  return 0;
>  }
> --
> 2.13.5 (Apple Git-94)
> 
> ___
> 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

[Spice-devel] [PATCH spice-gtk v2] spicy: Add call of gst_deinit at program exit

2017-10-19 Thread Christophe de Dinechin 
From: Christophe de Dinechin 

This is useful for some instrumentation, e.g. the leaks tracer,
that perform some of their operations within gst_deinit.

Without this patch, if you run spicy with
GST_DEBUG="GST_TRACER:7" GST_TRACERS="leaks" spicy ...
the leak tracer does not show any output, because it runs in gst_deinit.

Signed-off-by: Christophe de Dinechin 
---
 tools/spicy.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/spicy.c b/tools/spicy.c
index 088e034..d79e504 100644
--- a/tools/spicy.c
+++ b/tools/spicy.c
@@ -2068,5 +2068,8 @@ int main(int argc, char *argv[])
 g_free(spicy_title);
 
 setup_terminal(true);
+#if HAVE_GSTAUDIO || HAVE_GSTVIDEO
+gst_deinit();
+#endif
 return 0;
 }
-- 
2.13.5 (Apple Git-94)

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

[Spice-devel] [PATCH spice-server v2] Use standard "Red" namespace

2017-10-19 Thread Jonathon Jongsma 
The objects RedsStream and RedsSASL are currently using the namespace
"Reds" rather than the standard "Red" namespace used throughout the rest
of the project. Change these to be consistent. This also means changing
method names and some related enumeration types.

The files were also renamed to reflect the change:
  reds-stream.[ch] -> red-stream.[ch]

Signed-off-by: Jonathon Jongsma 
---
Changes in v2:
 - removed experimental code changes from botched cherry-pick

 server/Makefile.am |   4 +-
 server/common-graphics-channel.c   |   4 +-
 server/cursor-channel-client.c |   2 +-
 server/cursor-channel-client.h |   4 +-
 server/cursor-channel.c|   2 +-
 server/cursor-channel.h|   2 +-
 server/dcc-send.c  |   2 +-
 server/dcc.c   |   8 +-
 server/dcc.h   |   2 +-
 server/display-channel.h   |   2 +-
 server/inputs-channel-client.c |   2 +-
 server/inputs-channel-client.h |   2 +-
 server/inputs-channel.c|   6 +-
 server/main-channel-client.c   |   2 +-
 server/main-channel-client.h   |   2 +-
 server/main-channel.c  |   2 +-
 server/main-channel.h  |   2 +-
 server/red-channel-client.c|  28 ++--
 server/red-channel-client.h|   4 +-
 server/red-channel.c   |   6 +-
 server/red-channel.h   |   6 +-
 server/red-qxl.c   |   4 +-
 server/{reds-stream.c => red-stream.c} | 284 -
 server/red-stream.h|  93 +++
 server/red-worker.h|   4 +-
 server/reds-private.h  |   2 +-
 server/reds-stream.h   |  93 ---
 server/reds.c  | 158 +-
 server/smartcard-channel-client.c  |   2 +-
 server/smartcard-channel-client.h  |   2 +-
 server/smartcard.c |   2 +-
 server/sound.c |  10 +-
 server/spicevmc.c  |  12 +-
 server/stream-channel.c|   4 +-
 server/tests/test-channel.c|   6 +-
 server/tests/test-stream.c |  24 +--
 36 files changed, 397 insertions(+), 397 deletions(-)
 rename server/{reds-stream.c => red-stream.c} (74%)
 create mode 100644 server/red-stream.h
 delete mode 100644 server/reds-stream.h

diff --git a/server/Makefile.am b/server/Makefile.am
index e2e3ce861..20f0f1925 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -152,8 +152,8 @@ libserver_la_SOURCES =  \
reds.c  \
reds.h  \
reds-private.h  \
-   reds-stream.c   \
-   reds-stream.h   \
+   red-stream.c\
+   red-stream.h\
red-worker.c\
red-worker.h\
sound.c \
diff --git a/server/common-graphics-channel.c b/server/common-graphics-channel.c
index 0cbc2762c..ce6b5e57c 100644
--- a/server/common-graphics-channel.c
+++ b/server/common-graphics-channel.c
@@ -78,7 +78,7 @@ bool common_channel_client_config_socket(RedChannelClient 
*rcc)
 {
 RedClient *client = red_channel_client_get_client(rcc);
 MainChannelClient *mcc = red_client_get_main(client);
-RedsStream *stream = red_channel_client_get_stream(rcc);
+RedStream *stream = red_channel_client_get_stream(rcc);
 gboolean is_low_bandwidth;
 
 // TODO - this should be dynamic, not one time at channel creation
@@ -89,7 +89,7 @@ bool common_channel_client_config_socket(RedChannelClient 
*rcc)
  * the application level.
  * see: http://www.stuartcheshire.org/papers/NagleDelayedAck/
  */
-reds_stream_set_no_delay(stream, !is_low_bandwidth);
+red_stream_set_no_delay(stream, !is_low_bandwidth);
 
 // TODO: move wide/narrow ack setting to red_channel.
 red_channel_client_ack_set_client_window(rcc,
diff --git a/server/cursor-channel-client.c b/server/cursor-channel-client.c
index 42ab5d763..6d39e24ed 100644
--- a/server/cursor-channel-client.c
+++ b/server/cursor-channel-client.c
@@ -97,7 +97,7 @@ void cursor_channel_client_migrate(RedChannelClient *rcc)
 red_channel_client_default_migrate(rcc);
 }
 
-CursorChannelClient* cursor_channel_client_new(CursorChannel *cursor, 
RedClient *client, RedsStream *stream,
+CursorChannelClient* cursor_channel_client_new(CursorChannel *cursor, 
RedClient *client, RedStream *stream,
int mig_target,
RedChannelCapabilities *caps)
 {
diff --git a/server/cursor-channel-client.h

Re: [Spice-devel] [PATCH spice-server 1/2] Use standard "Red" namespace

2017-10-19 Thread Jonathon Jongsma 
On Thu, 2017-10-19 at 02:19 -0400, Frediano Ziglio wrote:
> > 
> > The objects RedsStream and RedsSASL are currently using the
> > namespace
> > "Reds" rather than the standard "Red" namespace used throughout the
> > rest
> > of the project. Change these to be consistent. This also means
> > changing
> > method names and some related enumeration types.
> > 
> > The files were also renamed to reflect the change:
> >   reds-stream.[ch] -> red-stream.[ch]
> > ---
> > 
> > Too much code churn?
> > 
> 
> Not too much worried. I'm more worried about the additional debug
> code,
> streaming and DisplayChannel :-)

hmm. What did I do? I tried to cherry-pick from a different branch and
apparently botched it. Sorry!



> 
> > 
> >  server/Makefile.am |   4 +-
> >  server/common-graphics-channel.c   |   4 +-
> >  server/cursor-channel-client.c |   2 +-
> >  server/cursor-channel-client.h |   4 +-
> >  server/cursor-channel.c|   2 +-
> >  server/cursor-channel.h|   2 +-
> >  server/dcc-send.c  |   4 +-
> >  server/dcc.c   |   8 +-
> >  server/dcc.h   |   2 +-
> >  server/display-channel.c   |  42 -
> >  server/display-channel.h   |   2 +-
> >  server/inputs-channel-client.c |   2 +-
> >  server/inputs-channel-client.h |   2 +-
> >  server/inputs-channel.c|   6 +-
> >  server/main-channel-client.c   |   2 +-
> >  server/main-channel-client.h   |   2 +-
> >  server/main-channel.c  |   2 +-
> >  server/main-channel.h  |   2 +-
> >  server/red-channel-client.c|  28 ++--
> >  server/red-channel-client.h|   4 +-
> >  server/red-channel.c   |   6 +-
> >  server/red-channel.h   |   6 +-
> >  server/red-qxl.c   |   4 +-
> >  server/{reds-stream.c => red-stream.c} | 284
> >  -
> >  server/red-stream.h|  93 +++
> >  server/red-worker.h|   4 +-
> >  server/reds-private.h  |   2 +-
> >  server/reds-stream.h   |  93 ---
> >  server/reds.c  | 162 +--
> >  server/smartcard-channel-client.c  |   2 +-
> >  server/smartcard-channel-client.h  |   2 +-
> >  server/smartcard.c |   2 +-
> >  server/sound.c |  10 +-
> >  server/spicevmc.c  |  12 +-
> >  server/stream-channel.c|   4 +-
> >  server/stream.c|   9 +-
> >  server/stream.h|   6 +-
> >  server/tests/test-channel.c|   6 +-
> >  server/tests/test-stream.c |  24 +--
> >  39 files changed, 449 insertions(+), 408 deletions(-)
> >  rename server/{reds-stream.c => red-stream.c} (74%)
> >  create mode 100644 server/red-stream.h
> >  delete mode 100644 server/reds-stream.h
> > 
> > diff --git a/server/Makefile.am b/server/Makefile.am
> > index e2e3ce861..20f0f1925 100644
> > --- a/server/Makefile.am
> > +++ b/server/Makefile.am
> > @@ -152,8 +152,8 @@ libserver_la_SOURCES =  
> > \
> > reds.c  \
> > reds.h  \
> > reds-private.h  \
> > -   reds-stream.c   \
> > -   reds-stream.h   \
> > +   red-stream.c\
> > +   red-stream.h\
> > red-worker.c\
> > red-worker.h\
> > sound.c \
> > diff --git a/server/common-graphics-channel.c
> > b/server/common-graphics-channel.c
> > index 0cbc2762c..ce6b5e57c 100644
> > --- a/server/common-graphics-channel.c
> > +++ b/server/common-graphics-channel.c
> > @@ -78,7 +78,7 @@ bool
> > common_channel_client_config_socket(RedChannelClient
> > *rcc)
> >  {
> >  RedClient *client = red_channel_client_get_client(rcc);
> >  MainChannelClient *mcc = red_client_get_main(client);
> > -RedsStream *stream = red_channel_client_get_stream(rcc);
> > +RedStream *stream = red_channel_client_get_stream(rcc);
> >  gboolean is_low_bandwidth;
> >  
> >  // TODO - this should be dynamic, not one time at channel
> > creation
> > @@ -89,7 +89,7 @@ bool
> > common_channel_client_config_socket(RedChannelClient
> > *rcc)
> >   * the application level.
> >   * see: http://www.stuartcheshire.org/papers/NagleDelayedAck/
> >   */
> > -reds_stream_set_no_delay(stream, !is_low_bandwidth);
> > +red_stream_set_no_delay(stream, !is_low_bandwidth);
> >  
> >  // TODO: move wide/narrow ack setting to red_channel.
> >  red_channel_client_ack_set_client_window(rcc,
> > diff --git

Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit

2017-10-19 Thread Christophe de Dinechin 

> On 19 Oct 2017, at 15:10, Daniel P. Berrange  wrote:
> 
> On Thu, Oct 19, 2017 at 03:01:17PM +0200, Christophe de Dinechin wrote:
>>> One reason is that you may use a library dynamically, and you may dlclose() 
>>> it, and then atexit() will likely crash. 
>> 
>> Is that a real or theoretical scenario? Who loads this library dynamically
>> currently?
> 
> Anyone using SPICE from a non-C language will have dlopen'd it and potentially
> later dlclose it. 

I’m actually quite curious how this could possibly fly today, given what I 
learned
recently about how we deal with gstreamer objects. Closing the library
seems a bit difficult. There isn’t even a header for that library.

That being said, there is this in spicy.c:

#if HAVE_GSTAUDIO || HAVE_GSTVIDEO
gst_init(, );
#endif

So I guess it makes sense to put the gst_deinit there. Ugly #ifdefs included.
I’ll do a patch with this approach instead.

In that case, do we really need calls to gst_init_check() in the “library"?

> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit

2017-10-19 Thread Christophe de Dinechin 

> On 19 Oct 2017, at 15:10, Daniel P. Berrange  wrote:
> 
> On Thu, Oct 19, 2017 at 03:01:17PM +0200, Christophe de Dinechin wrote:
>>> One reason is that you may use a library dynamically, and you may dlclose() 
>>> it, and then atexit() will likely crash. 
>> 
>> Is that a real or theoretical scenario? Who loads this library dynamically
>> currently?
> 
> Anyone using SPICE from a non-C language will have dlopen'd it and potentially
> later dlclose it. 

Note that here, the symbol passed to atexit is in gstreamer, not in spice.

> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[Spice-devel] MacOS Client Crashing upon Connection to graphic Server is established

2017-10-19 Thread Jannik Adam 

Hello Spice Dev-Team,

in order to connect to a virtual desktop hosted on a university system 
using "Red Head Virtualization", I installed the bundled Spice Version 
for macOS from your Site (https://www.spice-space.org/osx-client.html).


The initial connection to the VM seams to Work, but every time the Text 
on the Black screen


"Connecting to Graphic Server" switches to "Connected to Graphic Server"

the Remote Viewer stops listening to any kind of interaction. I even had 
to kill the process using the Activity Monitor...
Please note, that the connection is Working on a Windows Client, so the 
Server side seams OK.


To describe the situation in a nutshell:

I used the bundled *RemoteViewer* in *Version 0.5.7*
on *MacOS 10.12.6* (Sierra) (16G29)
on Hardware Modell *MacBookPro11,3*

I tried to connect to a *Remote Server* running *Red Head Virtualization
*The *VM's* I tried to connect to are running*XUbuntu 16.04*, *CentOS 7* 
and *Windows 10*


*After connection to the Graphic Server* is established,**the app *stops 
reacting to any kind of Input*.*

*There is *no problems* connecting to those VM's with the *Windows Client*.

Since i didn't found any obvious File that screamed "Log File", and in 
lack of anything else useful, I used the Activity Monitor to sample the 
process (that by the way didn't got any additional processing time since 
the hangup) and attached the Result onto this Mail. It contains a 
StackTrace that is hopefully helpful in some way.


I would appreciate any Help you can offer me.
If you need additional Informations or retests, feel free to contact me.

Thank you in advanced.

Regards,

Jannik Adam


**


Sampling process 2733 for 3 seconds with 1 millisecond of run time between 
samples
Sampling completed, processing symbols...
Analysis of sampling RemoteViewer-bin (pid 2733) every 1 millisecond
Process: RemoteViewer-bin [2733]
Path:/Applications/RemoteViewer.app/Contents/MacOS/RemoteViewer-bin
Load Address:0x10a53d000
Identifier:  org.virt-manager.remote-viewer
Version: 0.5.7 (0.5.7)
Code Type:   X86-64
Parent Process:  ??? [1]

Date/Time:   2017-10-18 19:31:24.273 +0200
Launch Time: 2017-10-18 19:13:38.275 +0200
OS Version:  Mac OS X 10.12.6 (16G29)
Report Version:  7
Analysis Tool:   /usr/bin/sample


Call graph:
2551 Thread_264357   DispatchQueue_1: com.apple.main-thread  (serial)
+ 2551 start  (in RemoteViewer-bin) + 52  [0x10a548454]
+   2551 main  (in RemoteViewer-bin) + 1588  [0x10a55c494]
+ 2551 gtk_main  (in libgtk-quartz-2.0.0.dylib) + 176  [0x10a68ec10]
+   2551 g_main_loop_run  (in libglib-2.0.0.dylib) + 287  [0x10b872fbf]
+ 2551 g_main_context_iterate  (in libglib-2.0.0.dylib) + 421  
[0x10b8719c5]
+   2551 poll_func  (in libgdk-quartz-2.0.0.dylib) + 181  
[0x10b121d75]
+ 2551 -[NSApplication(NSEvent) 
_nextEventMatchingEventMask:untilDate:inMode:dequeue:]  (in AppKit) + 2796  
[0x7fff9e0857ee]
+   2551 _DPSNextEvent  (in AppKit) + 1120  [0x7fff9d909a54]
+ 2551 _BlockUntilNextEventMatchingListInModeWithFilter  
(in HIToolbox) + 71  [0x7fff9f370b26]
+   2551 ReceiveNextEventCommon  (in HIToolbox) + 432  
[0x7fff9f370cf1]
+ 2551 RunCurrentEventLoopInMode  (in HIToolbox) + 240  
[0x7fff9f370ebc]
+   2551 CFRunLoopRunSpecific  (in CoreFoundation) + 
420  [0x7fff9fe10114]
+ 2551 __CFRunLoopRun  (in CoreFoundation) + 1361  
[0x7fff9fe108c1]
+   2551 __CFRunLoopServiceMachPort  (in 
CoreFoundation) + 212  [0x7fff9fe11434]
+ 2551 mach_msg  (in libsystem_kernel.dylib) + 
55  [0x7fffb56be797]
+   2551 mach_msg_trap  (in 
libsystem_kernel.dylib) + 10  [0x7fffb56bf34a]
2551 Thread_264392
+ 2551 thread_start  (in libsystem_pthread.dylib) + 13  [0x7fffb57b108d]
+   2551 _pthread_start  (in libsystem_pthread.dylib) + 286  
[0x7fffb57b1887]
+ 2551 _pthread_body  (in libsystem_pthread.dylib) + 180  
[0x7fffb57b193b]
+   2551 g_thread_proxy  (in libglib-2.0.0.dylib) + 90  [0x10b89490a]
+ 2551 coroutine_thread  (in libspice-client-glib-2.0.8.dylib) + 71 
 [0x10ab9bef7]
+   2551 spice_channel_coroutine  (in 
libspice-client-glib-2.0.8.dylib) + 9645  [0x10ab7a00d]
+ 2551 g_coroutine_socket_wait  (in 
libspice-client-glib-2.0.8.dylib) + 169  [0x10ab7c599]
+   2551 coroutine_yield  (in libspice-client-glib-2.0.8.dylib) 
+ 131  [0x10ab9bdc3]
+ 2551 g_cond_wait  (in libglib-2.0.0.dylib) + 131  
[0x10b8afae3]
+   2551 _pthread_cond_wait  (in libsystem_pthread.dylib) + 
847  [0x7fffb57b2881]
+ 2551 _pthread_mutex_lock_wait  (in 
libsystem_pthread.dylib) + 100  [0x7fffb57b1dfa]
+

Re: [Spice-devel] [PATCH] drm/qxl: workaround broken qxl hw primary setting.

2017-10-19 Thread Gerd Hoffmann 
On Thu, 2017-10-19 at 07:28 -0400, Frediano Ziglio wrote:

> > This tries to work out when the commit is likely just a pageflip
> > and avoid touching the primary surface, this might go wrong at
> > some point but I believe it's the same level as wrong as the old
> > code
> > base.
> > 
> > Signed-off-by: Dave Airlie 
> > ---
> >  drivers/gpu/drm/qxl/qxl_display.c | 21 +++--
> >  1 file changed, 19 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/qxl/qxl_display.c
> > b/drivers/gpu/drm/qxl/qxl_display.c
> > index afbf50d..3b702d1 100644
> > --- a/drivers/gpu/drm/qxl/qxl_display.c
> > +++ b/drivers/gpu/drm/qxl/qxl_display.c
> > @@ -502,6 +502,7 @@ static void qxl_primary_atomic_update(struct
> > drm_plane
> > *plane,
> >     struct qxl_framebuffer *qfb_old;
> >     struct qxl_bo *bo = gem_to_qxl_bo(qfb->obj);
> >     struct qxl_bo *bo_old;
> > +   bool update_primary = true;
> >     struct drm_clip_rect norect = {
> >     .x1 = 0,
> >     .y1 = 0,
> > @@ -519,15 +520,31 @@ static void qxl_primary_atomic_update(struct
> > drm_plane
> > *plane,
> >     if (bo == bo_old)
> >     return;
> >  
> > +   if (bo && bo_old &&
> > +   plane->state->crtc == old_state->crtc &&
> > +   plane->state->crtc_w == old_state->crtc_w &&
> > +   plane->state->crtc_h == old_state->crtc_h &&
> > +   plane->state->src_x == old_state->src_x &&
> > +   plane->state->src_y == old_state->src_y &&
> > +   plane->state->src_w == old_state->src_w &&
> > +   plane->state->src_h == old_state->src_h &&
> > +   plane->state->rotation == old_state->rotation &&
> > +   plane->state->zpos == old_state->zpos)
> > +   /* this is likely a pageflip */
> > +   update_primary = false;
> > +
> >     if (bo_old && bo_old->is_primary) {
> > -   qxl_io_destroy_primary(qdev);
> > +   if (update_primary)
> > +   qxl_io_destroy_primary(qdev);
> >     bo_old->is_primary = false;
> >     }
> >  
> >     if (!bo->is_primary) {
> > -   qxl_io_create_primary(qdev, 0, bo);
> > +   if (update_primary)
> > +   qxl_io_create_primary(qdev, 0, bo);
> >     bo->is_primary = true;
> 
> Here the primary is still the old object but you are setting the
> new.
> Looking around the old is unpinned and the new one pinned which
> is now wrong as QXL device suppose the memory pointer by the
> primary surface (after your patch bo_old object) remains
> available.

Yes.

So it is bug compatible with commit 058e9f5c82, as the commit message
claims ;)

> If we are not able to avoid the copy and we need to keep the old
> surface in place maybe instead of creating the new object as SURFACE
> we could just use for source for the Drawable for the DRAW_COPY
> operation.
> In this way when release is received the object can be unpinned being
> free to be moved.

https://lists.freedesktop.org/archives/dri-devel/2017-October/155541.ht
ml

cheers,
  Gerd

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

Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit

2017-10-19 Thread Daniel P. Berrange 
On Thu, Oct 19, 2017 at 03:01:17PM +0200, Christophe de Dinechin wrote:
> > One reason is that you may use a library dynamically, and you may dlclose() 
> > it, and then atexit() will likely crash. 
> 
> Is that a real or theoretical scenario? Who loads this library dynamically
> currently?

Anyone using SPICE from a non-C language will have dlopen'd it and potentially
later dlclose it. 

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit

2017-10-19 Thread Frediano Ziglio 
> > On 19 Oct 2017, at 13:49, Marc-André Lureau < marcandre.lur...@redhat.com >
> > wrote:
> 

> > - Original Message -
> 

> > > > On 19 Oct 2017, at 13:15, Marc-André Lureau <
> > > > marcandre.lur...@redhat.com
> > > > >
> > > 
> > 
> 
> > > > wrote:
> > > 
> > 
> 

> > > > Hi
> > > 
> > 
> 

> > > > - Original Message -
> > > 
> > 
> 

> > > > > > From: Christophe de Dinechin < dinec...@redhat.com >
> > > > > 
> > > > 
> > > 
> > 
> 

> > > > > > This is useful for some instrumentation, e.g. the leaks tracer,
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > that perform some of their operations within gst_deinit.
> > > > > 
> > > > 
> > > 
> > 
> 

> > > > > > Without this patch, if you run spicy with
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > GST_DEBUG="GST_TRACER:7" GST_TRACERS="leaks" spicy ...
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > the leak tracer does not run at exit, because it runs in
> > > > > > gst_deinit.
> > > > > 
> > > > 
> > > 
> > 
> 

> > > > > > Signed-off-by: Christophe de Dinechin < dinec...@redhat.com >
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > ---
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > spice-common | 2 +-
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > src/channel-display-gst.c | 1 +
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > 2 files changed, 2 insertions(+), 1 deletion(-)
> > > > > 
> > > > 
> > > 
> > 
> 

> > > > > > diff --git a/spice-common b/spice-common
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > index 429ad96..ba11de3 16
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > --- a/spice-common
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > +++ b/spice-common
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > @@ -1 +1 @@
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > -Subproject commit 429ad965371ceaaf60b81ccbed7da660ef9e0a94
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > +Subproject commit ba11de3f3fd58d1b1a99bb62dd9e409e9961a78e
> > > > > 
> > > > 
> > > 
> > 
> 

> > probably a mistake
> 

> Yes.

> > > > > > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > index f978602..c9ab9bf 100644
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > --- a/src/channel-display-gst.c
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > +++ b/src/channel-display-gst.c
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > @@ -578,6 +578,7 @@ static gboolean gstvideo_init(void)
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > GError *err = NULL;
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > if (gst_init_check(NULL, NULL, )) {
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > success = 1;
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > + atexit(gst_deinit);
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > } else {
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > spice_warning("Disabling GStreamer video support: %s",
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > err->message);
> > > > > 
> > > > 
> > > 
> > 
> 
> > > > > > g_clear_error();
> > > > > 
> > > > 
> > > 
> > 
> 

> > > > > Calling atexit from a library is a bad idea.
> > > > 
> > > 
> > 
> 

> > > > And calling gst_deinit() from a library is also wrong.
> > > 
> > 
> 

> > > Why? What would be a better way?
> > 
> 

> > Not calling it:
> 

> But as pointed in the commit log, this means that some gstreamer tools
> that rely on gst_deinit being called are broken for all spice clients.

> > /**
> 
> > * gst_deinit:
> 
> > *
> 
> > * Clean up any resources created by GStreamer in gst_init().
> 
> > *
> 
> > * It is normally not needed to call this function in a normal application
> 
> > * as the resources will automatically be freed when the program terminates.
> 
> > * This function is therefore mostly used by testsuites and other memory
> 
> > * profiling tools.
> 
> > *
> 
> > * After this call GStreamer (including this method) should not be used
> > anymore.
> 
> > */
> 

> > spice-gtk is a library, that happen to use gstreamer.
> 

> If this library did init gstreamer, it is responsible to deinit it.
> Just like if it opens a file or allocates memory, it’s responsible to close
> it or deallocate it.

> > If something else in the client app is also using gstreamer this will cause
> > troubles.
> 

> And if someone unloads our library presently, it leaks.

> > Well calling atexit() should be fine, even if called multiple times, Why
> > it's
> > not done in gstreamer in this case? Why would every application have to add
> > gst_deinit() themself?
> 

> Well, the gstreamer init / deinit design reeks of big globals lurking
> somewhere anyway.
> So if you ask me, that interface should have been more like:

> gst_id id = gst_init();
> gst_deinit(id);

> That would have made it very clear that you could init multiple times and
> that each caller
> was responsible for calling deinit. But of course, that would have also meant
> passing the
> id around all the time. Much less convenient.

Or you could use a static counter incremented by gst_init and decremented by

Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit

2017-10-19 Thread Christophe de Dinechin 

> On 19 Oct 2017, at 13:49, Marc-André Lureau  
> wrote:
> 
> 
> 
> - Original Message -
>> 
>>> On 19 Oct 2017, at 13:15, Marc-André Lureau 
>>> wrote:
>>> 
>>> Hi
>>> 
>>> - Original Message -
> 
> From: Christophe de Dinechin 
> 
> This is useful for some instrumentation, e.g. the leaks tracer,
> that perform some of their operations within gst_deinit.
> 
> Without this patch, if you run spicy with
>   GST_DEBUG="GST_TRACER:7" GST_TRACERS="leaks" spicy ...
> the leak tracer does not run at exit, because it runs in gst_deinit.
> 
> Signed-off-by: Christophe de Dinechin 
> ---
> spice-common  | 2 +-
> src/channel-display-gst.c | 1 +
> 2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/spice-common b/spice-common
> index 429ad96..ba11de3 16
> --- a/spice-common
> +++ b/spice-common
> @@ -1 +1 @@
> -Subproject commit 429ad965371ceaaf60b81ccbed7da660ef9e0a94
> +Subproject commit ba11de3f3fd58d1b1a99bb62dd9e409e9961a78e
> 
> probably a mistake

Yes.

> 
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index f978602..c9ab9bf 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -578,6 +578,7 @@ static gboolean gstvideo_init(void)
>GError *err = NULL;
>if (gst_init_check(NULL, NULL, )) {
>success = 1;
> +atexit(gst_deinit);
>} else {
>spice_warning("Disabling GStreamer video support: %s",
>err->message);
>g_clear_error();
 
 Calling atexit from a library is a bad idea.
>>> 
>>> And calling gst_deinit() from a library is also wrong.
>> 
>> Why? What would be a better way?
> 
> Not calling it:

But as pointed in the commit log, this means that some gstreamer tools
that rely on gst_deinit being called are broken for all spice clients.


> /**
> * gst_deinit:
> *
> * Clean up any resources created by GStreamer in gst_init().
> *
> * It is normally not needed to call this function in a normal application
> * as the resources will automatically be freed when the program terminates.
> * This function is therefore mostly used by testsuites and other memory
> * profiling tools.
> *
> * After this call GStreamer (including this method) should not be used 
> anymore. 
> */
> 
> spice-gtk is a library, that happen to use gstreamer.

If this library did init gstreamer, it is responsible to deinit it.
Just like if it opens a file or allocates memory, it’s responsible to close it 
or deallocate it.


> 
> If something else in the client app is also using gstreamer this will cause 
> troubles.

And if someone unloads our library presently, it leaks.


> 
> Well calling atexit() should be fine, even if called multiple times, Why it's 
> not done in gstreamer in this case? Why would every application have to add 
> gst_deinit() themself?

Well, the gstreamer init / deinit design reeks of big globals lurking somewhere 
anyway.
So if you ask me, that interface should have been more like:

gst_id id = gst_init();
gst_deinit(id);

That would have made it very clear that you could init multiple times and that 
each caller
was responsible for calling deinit. But of course, that would have also meant 
passing the
id around all the time. Much less convenient.

Since we don’t have that kind of interface, we can only second-guess wha the 
intent of the
orignal developers was.

Globals are evil. Pthread globals are worse. That’s not a reason to not call 
deinit.

> 
> One reason is that you may use a library dynamically, and you may dlclose() 
> it, and then atexit() will likely crash. 

Is that a real or theoretical scenario? Who loads this library dynamically 
currently?

> 
>> 
>> The scenario where we call gst_init is a bit complicated,
>> and there are multiple clients that may call it. I see
>> no case where, when we have called gst_init, we are not
>> responsible for also calling gst_deinit.
>> 
>> In any case, it’s even more wrong to not call gst_deinit at all,
>> and I don’t see a simple way to do that from main with the
>> existing code without elaborate tests that have somewhat
>> deep knowledge of the library internals.
>> 
>> How would you do it?

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

Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit

2017-10-19 Thread Daniel P. Berrange 
On Thu, Oct 19, 2017 at 08:53:08AM -0400, Frediano Ziglio wrote:
> > 
> > On Thu, Oct 19, 2017 at 08:03:00AM -0400, Frediano Ziglio wrote:
> > > > 
> > > > 
> > > > In fact non-trivial shared libraries should generally never be unloaded,
> > > > even
> > > > if they were originally dlopend.  If the library has used a pthread 
> > > > local
> > > > with
> > > > a destructor function, then unloading the library will remove the code
> > > > that
> > > > contains the impl of the destructor. When the thread later exits and its
> > > > thread
> > > > locals are cleaned up, the application will crash & burn.
> > > > 
> > > > Many libraries, including libvirt, will link with '-z nodelete' to
> > > > prevent
> > > > unloading of the library even if dlclose() is called, to avoid these 
> > > > kind
> > > > of crashes.
> > > > 
> > > > IOW getting perfect "cleanup" is just a fools errand and will likely
> > > > create
> > > > obscure problems down the road that are worse than the problems the
> > > > cleanup
> > > > is trying to solve. Just accept normal process resource cleanup when the
> > > > application exits.
> > > 
> > > This is a point for Windows... they manage to have unloading working.
> > > Also you can unload Linux kernel modules.
> > > I honestly find these reasoning a lazy excuse to bad programming and
> > > design.
> > 
> > Calling it laziness is completely missing the point. There is *nothing*
> > in the pthreads API that lets you avoid the problem with thread local
> > destructors described above, no matter how much we want to fix it. The
> > only "solution" is to not use pthread locals at all, which is not practical.
> > 
> 
> If not lazy as I said is bad design, not? Somebody designed pthread too.

POSIX designed pthreads, but dlopen comes from Solaris, only after that
adopted as part of POSIX. The situation is just a result of the way the
standard has organically grown over time, not been designed top down to
deal with all possible interactions

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit

2017-10-19 Thread Frediano Ziglio 
> 
> On Thu, Oct 19, 2017 at 08:03:00AM -0400, Frediano Ziglio wrote:
> > > 
> > > 
> > > In fact non-trivial shared libraries should generally never be unloaded,
> > > even
> > > if they were originally dlopend.  If the library has used a pthread local
> > > with
> > > a destructor function, then unloading the library will remove the code
> > > that
> > > contains the impl of the destructor. When the thread later exits and its
> > > thread
> > > locals are cleaned up, the application will crash & burn.
> > > 
> > > Many libraries, including libvirt, will link with '-z nodelete' to
> > > prevent
> > > unloading of the library even if dlclose() is called, to avoid these kind
> > > of crashes.
> > > 
> > > IOW getting perfect "cleanup" is just a fools errand and will likely
> > > create
> > > obscure problems down the road that are worse than the problems the
> > > cleanup
> > > is trying to solve. Just accept normal process resource cleanup when the
> > > application exits.
> > 
> > This is a point for Windows... they manage to have unloading working.
> > Also you can unload Linux kernel modules.
> > I honestly find these reasoning a lazy excuse to bad programming and
> > design.
> 
> Calling it laziness is completely missing the point. There is *nothing*
> in the pthreads API that lets you avoid the problem with thread local
> destructors described above, no matter how much we want to fix it. The
> only "solution" is to not use pthread locals at all, which is not practical.
> 
> 
> Regards,
> Daniel

If not lazy as I said is bad design, not? Somebody designed pthread too.

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

Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit

2017-10-19 Thread Daniel P. Berrange 
On Thu, Oct 19, 2017 at 08:03:00AM -0400, Frediano Ziglio wrote:
> > 
> > 
> > In fact non-trivial shared libraries should generally never be unloaded, 
> > even
> > if they were originally dlopend.  If the library has used a pthread local
> > with
> > a destructor function, then unloading the library will remove the code that
> > contains the impl of the destructor. When the thread later exits and its
> > thread
> > locals are cleaned up, the application will crash & burn.
> > 
> > Many libraries, including libvirt, will link with '-z nodelete' to prevent
> > unloading of the library even if dlclose() is called, to avoid these kind
> > of crashes.
> > 
> > IOW getting perfect "cleanup" is just a fools errand and will likely create
> > obscure problems down the road that are worse than the problems the cleanup
> > is trying to solve. Just accept normal process resource cleanup when the
> > application exits.
> 
> This is a point for Windows... they manage to have unloading working.
> Also you can unload Linux kernel modules.
> I honestly find these reasoning a lazy excuse to bad programming and design.

Calling it laziness is completely missing the point. There is *nothing*
in the pthreads API that lets you avoid the problem with thread local
destructors described above, no matter how much we want to fix it. The
only "solution" is to not use pthread locals at all, which is not practical.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit

2017-10-19 Thread Frediano Ziglio 
> 
> On Thu, Oct 19, 2017 at 07:47:24AM -0400, Frediano Ziglio wrote:
> > > > On 19 Oct 2017, at 12:32, Frediano Ziglio < fzig...@redhat.com > wrote:
> > > 
> > 
> > > > > From: Christophe de Dinechin < dinec...@redhat.com >
> > > > 
> > > 
> > 
> > > > > This is useful for some instrumentation, e.g. the leaks tracer,
> > > > 
> > > 
> > > > > that perform some of their operations within gst_deinit.
> > > > 
> > > 
> > 
> > > > > Without this patch, if you run spicy with
> > > > 
> > > 
> > > > > GST_DEBUG="GST_TRACER:7" GST_TRACERS="leaks" spicy ...
> > > > 
> > > 
> > > > > the leak tracer does not run at exit, because it runs in gst_deinit.
> > > > 
> > > 
> > 
> > > > > Signed-off-by: Christophe de Dinechin < dinec...@redhat.com >
> > > > 
> > > 
> > > > > ---
> > > > 
> > > 
> > > > > spice-common | 2 +-
> > > > 
> > > 
> > > > > src/channel-display-gst.c | 1 +
> > > > 
> > > 
> > > > > 2 files changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > 
> > 
> > > > > diff --git a/spice-common b/spice-common
> > > > 
> > > 
> > > > > index 429ad96..ba11de3 16
> > > > 
> > > 
> > > > > --- a/spice-common
> > > > 
> > > 
> > > > > +++ b/spice-common
> > > > 
> > > 
> > > > > @@ -1 +1 @@
> > > > 
> > > 
> > > > > -Subproject commit 429ad965371ceaaf60b81ccbed7da660ef9e0a94
> > > > 
> > > 
> > > > > +Subproject commit ba11de3f3fd58d1b1a99bb62dd9e409e9961a78e
> > > > 
> > > 
> > > > > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > > > 
> > > 
> > > > > index f978602..c9ab9bf 100644
> > > > 
> > > 
> > > > > --- a/src/channel-display-gst.c
> > > > 
> > > 
> > > > > +++ b/src/channel-display-gst.c
> > > > 
> > > 
> > > > > @@ -578,6 +578,7 @@ static gboolean gstvideo_init(void)
> > > > 
> > > 
> > > > > GError *err = NULL;
> > > > 
> > > 
> > > > > if (gst_init_check(NULL, NULL, )) {
> > > > 
> > > 
> > > > > success = 1;
> > > > 
> > > 
> > > > > + atexit(gst_deinit);
> > > > 
> > > 
> > > > > } else {
> > > > 
> > > 
> > > > > spice_warning("Disabling GStreamer video support: %s",
> > > > 
> > > 
> > > > > err->message);
> > > > 
> > > 
> > > > > g_clear_error();
> > > > 
> > > 
> > 
> > > > Calling atexit from a library is a bad idea.
> > > 
> > 
> > > Could you elaborate?
> > 
> > > I do not really agree with this statement. I’d actually go as far as
> > > saying
> > > that libraries are the reason atexit exists to start with.
> > > Apparently, I’m not alone, see first three responses in
> > > https://stackoverflow.com/questions/25115612/whats-the-scenario-to-use-atexit-function
> > > that all mention libraries.
> > 
> > > Christophe
> > 
> > Shared libraries in theory can be unloaded before the atexit function is
> > called for instance.
> > 
> > Calling gst_deinit from a library is a bad idea because other part of the
> > program could use
> > gstreamer too and call other gstreamer functions. Even after your atexit
> > function!
> > 
> > The gcc way to catch shared library unload is to use the destructor
> > attribute
> > which in Unix usually chain the .deinit/deinit function. Also has the
> > advantage of not using space (atexit function calls are usually limited as
> > in
> > many systems the buffer used to register  them is static).
> 
> In fact non-trivial shared libraries should generally never be unloaded, even
> if they were originally dlopend.  If the library has used a pthread local
> with
> a destructor function, then unloading the library will remove the code that
> contains the impl of the destructor. When the thread later exits and its
> thread
> locals are cleaned up, the application will crash & burn.
> 
> Many libraries, including libvirt, will link with '-z nodelete' to prevent
> unloading of the library even if dlclose() is called, to avoid these kind
> of crashes.
> 
> IOW getting perfect "cleanup" is just a fools errand and will likely create
> obscure problems down the road that are worse than the problems the cleanup
> is trying to solve. Just accept normal process resource cleanup when the
> application exits.
> 
> Regards,
> Daniel

This is a point for Windows... they manage to have unloading working.
Also you can unload Linux kernel modules.
I honestly find these reasoning a lazy excuse to bad programming and design.

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

Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit

2017-10-19 Thread Daniel P. Berrange 
On Thu, Oct 19, 2017 at 07:47:24AM -0400, Frediano Ziglio wrote:
> > > On 19 Oct 2017, at 12:32, Frediano Ziglio < fzig...@redhat.com > wrote:
> > 
> 
> > > > From: Christophe de Dinechin < dinec...@redhat.com >
> > > 
> > 
> 
> > > > This is useful for some instrumentation, e.g. the leaks tracer,
> > > 
> > 
> > > > that perform some of their operations within gst_deinit.
> > > 
> > 
> 
> > > > Without this patch, if you run spicy with
> > > 
> > 
> > > > GST_DEBUG="GST_TRACER:7" GST_TRACERS="leaks" spicy ...
> > > 
> > 
> > > > the leak tracer does not run at exit, because it runs in gst_deinit.
> > > 
> > 
> 
> > > > Signed-off-by: Christophe de Dinechin < dinec...@redhat.com >
> > > 
> > 
> > > > ---
> > > 
> > 
> > > > spice-common | 2 +-
> > > 
> > 
> > > > src/channel-display-gst.c | 1 +
> > > 
> > 
> > > > 2 files changed, 2 insertions(+), 1 deletion(-)
> > > 
> > 
> 
> > > > diff --git a/spice-common b/spice-common
> > > 
> > 
> > > > index 429ad96..ba11de3 16
> > > 
> > 
> > > > --- a/spice-common
> > > 
> > 
> > > > +++ b/spice-common
> > > 
> > 
> > > > @@ -1 +1 @@
> > > 
> > 
> > > > -Subproject commit 429ad965371ceaaf60b81ccbed7da660ef9e0a94
> > > 
> > 
> > > > +Subproject commit ba11de3f3fd58d1b1a99bb62dd9e409e9961a78e
> > > 
> > 
> > > > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > > 
> > 
> > > > index f978602..c9ab9bf 100644
> > > 
> > 
> > > > --- a/src/channel-display-gst.c
> > > 
> > 
> > > > +++ b/src/channel-display-gst.c
> > > 
> > 
> > > > @@ -578,6 +578,7 @@ static gboolean gstvideo_init(void)
> > > 
> > 
> > > > GError *err = NULL;
> > > 
> > 
> > > > if (gst_init_check(NULL, NULL, )) {
> > > 
> > 
> > > > success = 1;
> > > 
> > 
> > > > + atexit(gst_deinit);
> > > 
> > 
> > > > } else {
> > > 
> > 
> > > > spice_warning("Disabling GStreamer video support: %s",
> > > 
> > 
> > > > err->message);
> > > 
> > 
> > > > g_clear_error();
> > > 
> > 
> 
> > > Calling atexit from a library is a bad idea.
> > 
> 
> > Could you elaborate?
> 
> > I do not really agree with this statement. I’d actually go as far as saying
> > that libraries are the reason atexit exists to start with.
> > Apparently, I’m not alone, see first three responses in
> > https://stackoverflow.com/questions/25115612/whats-the-scenario-to-use-atexit-function
> > that all mention libraries.
> 
> > Christophe
> 
> Shared libraries in theory can be unloaded before the atexit function is 
> called for instance. 
> 
> Calling gst_deinit from a library is a bad idea because other part of the 
> program could use 
> gstreamer too and call other gstreamer functions. Even after your atexit 
> function! 
> 
> The gcc way to catch shared library unload is to use the destructor attribute
> which in Unix usually chain the .deinit/deinit function. Also has the
> advantage of not using space (atexit function calls are usually limited as in
> many systems the buffer used to register  them is static).

In fact non-trivial shared libraries should generally never be unloaded, even
if they were originally dlopend.  If the library has used a pthread local with
a destructor function, then unloading the library will remove the code that
contains the impl of the destructor. When the thread later exits and its thread
locals are cleaned up, the application will crash & burn.

Many libraries, including libvirt, will link with '-z nodelete' to prevent
unloading of the library even if dlclose() is called, to avoid these kind
of crashes.

IOW getting perfect "cleanup" is just a fools errand and will likely create
obscure problems down the road that are worse than the problems the cleanup
is trying to solve. Just accept normal process resource cleanup when the
application exits.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit

2017-10-19 Thread Marc-André Lureau 


- Original Message -
> 
> > On 19 Oct 2017, at 13:15, Marc-André Lureau 
> > wrote:
> > 
> > Hi
> > 
> > - Original Message -
> >>> 
> >>> From: Christophe de Dinechin 
> >>> 
> >>> This is useful for some instrumentation, e.g. the leaks tracer,
> >>> that perform some of their operations within gst_deinit.
> >>> 
> >>> Without this patch, if you run spicy with
> >>>   GST_DEBUG="GST_TRACER:7" GST_TRACERS="leaks" spicy ...
> >>> the leak tracer does not run at exit, because it runs in gst_deinit.
> >>> 
> >>> Signed-off-by: Christophe de Dinechin 
> >>> ---
> >>> spice-common  | 2 +-
> >>> src/channel-display-gst.c | 1 +
> >>> 2 files changed, 2 insertions(+), 1 deletion(-)
> >>> 
> >>> diff --git a/spice-common b/spice-common
> >>> index 429ad96..ba11de3 16
> >>> --- a/spice-common
> >>> +++ b/spice-common
> >>> @@ -1 +1 @@
> >>> -Subproject commit 429ad965371ceaaf60b81ccbed7da660ef9e0a94
> >>> +Subproject commit ba11de3f3fd58d1b1a99bb62dd9e409e9961a78e

probably a mistake

> >>> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> >>> index f978602..c9ab9bf 100644
> >>> --- a/src/channel-display-gst.c
> >>> +++ b/src/channel-display-gst.c
> >>> @@ -578,6 +578,7 @@ static gboolean gstvideo_init(void)
> >>> GError *err = NULL;
> >>> if (gst_init_check(NULL, NULL, )) {
> >>> success = 1;
> >>> +atexit(gst_deinit);
> >>> } else {
> >>> spice_warning("Disabling GStreamer video support: %s",
> >>> err->message);
> >>> g_clear_error();
> >> 
> >> Calling atexit from a library is a bad idea.
> > 
> > And calling gst_deinit() from a library is also wrong.
> 
> Why? What would be a better way?

Not calling it:
/**
 * gst_deinit:
 *
 * Clean up any resources created by GStreamer in gst_init().
 *
 * It is normally not needed to call this function in a normal application
 * as the resources will automatically be freed when the program terminates.
 * This function is therefore mostly used by testsuites and other memory
 * profiling tools.
 *
 * After this call GStreamer (including this method) should not be used 
anymore. 
 */

spice-gtk is a library, that happen to use gstreamer.

If something else in the client app is also using gstreamer this will cause 
troubles.

Well calling atexit() should be fine, even if called multiple times, Why it's 
not done in gstreamer in this case? Why would every application have to add 
gst_deinit() themself?

One reason is that you may use a library dynamically, and you may dlclose() it, 
and then atexit() will likely crash. 

> 
> The scenario where we call gst_init is a bit complicated,
> and there are multiple clients that may call it. I see
> no case where, when we have called gst_init, we are not
> responsible for also calling gst_deinit.
> 
> In any case, it’s even more wrong to not call gst_deinit at all,
> and I don’t see a simple way to do that from main with the
> existing code without elaborate tests that have somewhat
> deep knowledge of the library internals.
> 
> How would you do it?
> 
> > 
> 
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit

2017-10-19 Thread Frediano Ziglio 
> > On 19 Oct 2017, at 12:32, Frediano Ziglio < fzig...@redhat.com > wrote:
> 

> > > From: Christophe de Dinechin < dinec...@redhat.com >
> > 
> 

> > > This is useful for some instrumentation, e.g. the leaks tracer,
> > 
> 
> > > that perform some of their operations within gst_deinit.
> > 
> 

> > > Without this patch, if you run spicy with
> > 
> 
> > > GST_DEBUG="GST_TRACER:7" GST_TRACERS="leaks" spicy ...
> > 
> 
> > > the leak tracer does not run at exit, because it runs in gst_deinit.
> > 
> 

> > > Signed-off-by: Christophe de Dinechin < dinec...@redhat.com >
> > 
> 
> > > ---
> > 
> 
> > > spice-common | 2 +-
> > 
> 
> > > src/channel-display-gst.c | 1 +
> > 
> 
> > > 2 files changed, 2 insertions(+), 1 deletion(-)
> > 
> 

> > > diff --git a/spice-common b/spice-common
> > 
> 
> > > index 429ad96..ba11de3 16
> > 
> 
> > > --- a/spice-common
> > 
> 
> > > +++ b/spice-common
> > 
> 
> > > @@ -1 +1 @@
> > 
> 
> > > -Subproject commit 429ad965371ceaaf60b81ccbed7da660ef9e0a94
> > 
> 
> > > +Subproject commit ba11de3f3fd58d1b1a99bb62dd9e409e9961a78e
> > 
> 
> > > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > 
> 
> > > index f978602..c9ab9bf 100644
> > 
> 
> > > --- a/src/channel-display-gst.c
> > 
> 
> > > +++ b/src/channel-display-gst.c
> > 
> 
> > > @@ -578,6 +578,7 @@ static gboolean gstvideo_init(void)
> > 
> 
> > > GError *err = NULL;
> > 
> 
> > > if (gst_init_check(NULL, NULL, )) {
> > 
> 
> > > success = 1;
> > 
> 
> > > + atexit(gst_deinit);
> > 
> 
> > > } else {
> > 
> 
> > > spice_warning("Disabling GStreamer video support: %s",
> > 
> 
> > > err->message);
> > 
> 
> > > g_clear_error();
> > 
> 

> > Calling atexit from a library is a bad idea.
> 

> Could you elaborate?

> I do not really agree with this statement. I’d actually go as far as saying
> that libraries are the reason atexit exists to start with.
> Apparently, I’m not alone, see first three responses in
> https://stackoverflow.com/questions/25115612/whats-the-scenario-to-use-atexit-function
> that all mention libraries.

> Christophe

Shared libraries in theory can be unloaded before the atexit function is called 
for instance. 

Calling gst_deinit from a library is a bad idea because other part of the 
program could use 
gstreamer too and call other gstreamer functions. Even after your atexit 
function! 

The gcc way to catch shared library unload is to use the destructor attribute 
which in 
Unix usually chain the .deinit/deinit function. Also has the advantage of not 
using space 
(atexit function calls are usually limited as in many systems the buffer used 
to register 
them is static). 

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

Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit

2017-10-19 Thread Christophe de Dinechin 

> On 19 Oct 2017, at 13:19, Frediano Ziglio  wrote:
> 
>> 
>> Hi
>> 
>> - Original Message -
 
 From: Christophe de Dinechin 
 
 This is useful for some instrumentation, e.g. the leaks tracer,
 that perform some of their operations within gst_deinit.
 
 Without this patch, if you run spicy with
GST_DEBUG="GST_TRACER:7" GST_TRACERS="leaks" spicy ...
 the leak tracer does not run at exit, because it runs in gst_deinit.
 
 Signed-off-by: Christophe de Dinechin 
 ---
 spice-common  | 2 +-
 src/channel-display-gst.c | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)
 
 diff --git a/spice-common b/spice-common
 index 429ad96..ba11de3 16
 --- a/spice-common
 +++ b/spice-common
 @@ -1 +1 @@
 -Subproject commit 429ad965371ceaaf60b81ccbed7da660ef9e0a94
 +Subproject commit ba11de3f3fd58d1b1a99bb62dd9e409e9961a78e
 diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
 index f978602..c9ab9bf 100644
 --- a/src/channel-display-gst.c
 +++ b/src/channel-display-gst.c
 @@ -578,6 +578,7 @@ static gboolean gstvideo_init(void)
 GError *err = NULL;
 if (gst_init_check(NULL, NULL, )) {
 success = 1;
 +atexit(gst_deinit);
 } else {
 spice_warning("Disabling GStreamer video support: %s",
 err->message);
 g_clear_error();
>>> 
>>> Calling atexit from a library is a bad idea.
>> 
>> And calling gst_deinit() from a library is also wrong.
>> 
>> 
> 
> If the main target is just spicy and knowing spicy is mainly a developer
> tool you can add a call to gst_deinit in spicy.

Why would only spicy be concerned? That’s the case where I saw
the issue, but I guess any user of the library has the same issue.

> Frediano

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

Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit

2017-10-19 Thread Christophe de Dinechin 

> On 19 Oct 2017, at 13:15, Marc-André Lureau  
> wrote:
> 
> Hi
> 
> - Original Message -
>>> 
>>> From: Christophe de Dinechin 
>>> 
>>> This is useful for some instrumentation, e.g. the leaks tracer,
>>> that perform some of their operations within gst_deinit.
>>> 
>>> Without this patch, if you run spicy with
>>> GST_DEBUG="GST_TRACER:7" GST_TRACERS="leaks" spicy ...
>>> the leak tracer does not run at exit, because it runs in gst_deinit.
>>> 
>>> Signed-off-by: Christophe de Dinechin 
>>> ---
>>> spice-common  | 2 +-
>>> src/channel-display-gst.c | 1 +
>>> 2 files changed, 2 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/spice-common b/spice-common
>>> index 429ad96..ba11de3 16
>>> --- a/spice-common
>>> +++ b/spice-common
>>> @@ -1 +1 @@
>>> -Subproject commit 429ad965371ceaaf60b81ccbed7da660ef9e0a94
>>> +Subproject commit ba11de3f3fd58d1b1a99bb62dd9e409e9961a78e
>>> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
>>> index f978602..c9ab9bf 100644
>>> --- a/src/channel-display-gst.c
>>> +++ b/src/channel-display-gst.c
>>> @@ -578,6 +578,7 @@ static gboolean gstvideo_init(void)
>>> GError *err = NULL;
>>> if (gst_init_check(NULL, NULL, )) {
>>> success = 1;
>>> +atexit(gst_deinit);
>>> } else {
>>> spice_warning("Disabling GStreamer video support: %s",
>>> err->message);
>>> g_clear_error();
>> 
>> Calling atexit from a library is a bad idea.
> 
> And calling gst_deinit() from a library is also wrong.

Why? What would be a better way?

The scenario where we call gst_init is a bit complicated,
and there are multiple clients that may call it. I see
no case where, when we have called gst_init, we are not
responsible for also calling gst_deinit.

In any case, it’s even more wrong to not call gst_deinit at all,
and I don’t see a simple way to do that from main with the
existing code without elaborate tests that have somewhat
deep knowledge of the library internals.

How would you do it?

> 

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

Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit

2017-10-19 Thread Christophe de Dinechin 

> On 19 Oct 2017, at 12:32, Frediano Ziglio  wrote:
> 
>> 
>> From: Christophe de Dinechin 
>> 
>> This is useful for some instrumentation, e.g. the leaks tracer,
>> that perform some of their operations within gst_deinit.
>> 
>> Without this patch, if you run spicy with
>>  GST_DEBUG="GST_TRACER:7" GST_TRACERS="leaks" spicy ...
>> the leak tracer does not run at exit, because it runs in gst_deinit.
>> 
>> Signed-off-by: Christophe de Dinechin 
>> ---
>> spice-common  | 2 +-
>> src/channel-display-gst.c | 1 +
>> 2 files changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/spice-common b/spice-common
>> index 429ad96..ba11de3 16
>> --- a/spice-common
>> +++ b/spice-common
>> @@ -1 +1 @@
>> -Subproject commit 429ad965371ceaaf60b81ccbed7da660ef9e0a94
>> +Subproject commit ba11de3f3fd58d1b1a99bb62dd9e409e9961a78e
>> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
>> index f978602..c9ab9bf 100644
>> --- a/src/channel-display-gst.c
>> +++ b/src/channel-display-gst.c
>> @@ -578,6 +578,7 @@ static gboolean gstvideo_init(void)
>> GError *err = NULL;
>> if (gst_init_check(NULL, NULL, )) {
>> success = 1;
>> +atexit(gst_deinit);
>> } else {
>> spice_warning("Disabling GStreamer video support: %s",
>> err->message);
>> g_clear_error();
> 
> Calling atexit from a library is a bad idea.

Could you elaborate?

I do not really agree with this statement. I’d actually go as far as saying
that libraries are the reason atexit exists to start with.
Apparently, I’m not alone, see first three responses in
https://stackoverflow.com/questions/25115612/whats-the-scenario-to-use-atexit-function
that all mention libraries.


Christophe

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

Re: [Spice-devel] [PATCH] drm/qxl: workaround broken qxl hw primary setting.

2017-10-19 Thread Frediano Ziglio 
> 
> From: Dave Airlie 
> 
> QXL hw can't change primary surfaces easily, the server sends a msg
> and the client flickers a lo when it does. The old pre-atomic
> page flip code endeavoured to workaround this with a copy operation.
> 
> This worked by another accident of how the qxl virtual gpu is designed,
> it does lazy operation evaluation on the host, so this operation
> wouldn't generally trash the memory, and result in correct display.
> 
> This tries to work out when the commit is likely just a pageflip
> and avoid touching the primary surface, this might go wrong at
> some point but I believe it's the same level as wrong as the old code
> base.
> 
> Signed-off-by: Dave Airlie 
> ---
>  drivers/gpu/drm/qxl/qxl_display.c | 21 +++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c
> b/drivers/gpu/drm/qxl/qxl_display.c
> index afbf50d..3b702d1 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -502,6 +502,7 @@ static void qxl_primary_atomic_update(struct drm_plane
> *plane,
>   struct qxl_framebuffer *qfb_old;
>   struct qxl_bo *bo = gem_to_qxl_bo(qfb->obj);
>   struct qxl_bo *bo_old;
> + bool update_primary = true;
>   struct drm_clip_rect norect = {
>   .x1 = 0,
>   .y1 = 0,
> @@ -519,15 +520,31 @@ static void qxl_primary_atomic_update(struct drm_plane
> *plane,
>   if (bo == bo_old)
>   return;
>  
> + if (bo && bo_old &&
> + plane->state->crtc == old_state->crtc &&
> + plane->state->crtc_w == old_state->crtc_w &&
> + plane->state->crtc_h == old_state->crtc_h &&
> + plane->state->src_x == old_state->src_x &&
> + plane->state->src_y == old_state->src_y &&
> + plane->state->src_w == old_state->src_w &&
> + plane->state->src_h == old_state->src_h &&
> + plane->state->rotation == old_state->rotation &&
> + plane->state->zpos == old_state->zpos)
> + /* this is likely a pageflip */
> + update_primary = false;
> +
>   if (bo_old && bo_old->is_primary) {
> - qxl_io_destroy_primary(qdev);
> + if (update_primary)
> + qxl_io_destroy_primary(qdev);
>   bo_old->is_primary = false;
>   }
>  
>   if (!bo->is_primary) {
> - qxl_io_create_primary(qdev, 0, bo);
> + if (update_primary)
> + qxl_io_create_primary(qdev, 0, bo);
>   bo->is_primary = true;

Here the primary is still the old object but you are setting the
new.
Looking around the old is unpinned and the new one pinned which
is now wrong as QXL device suppose the memory pointer by the
primary surface (after your patch bo_old object) remains
available.

>   }
> +
>   qxl_draw_dirty_fb(qdev, qfb, bo, 0, 0, , 1, 1);

I had a look at this function. It send a DRAW_COPY operation
passing the bo object as a Drawable. However does nothing to
keep the object allocated. The Drawable and its buffer (so
bo object) should be available to QXL and not touched (changed)
till a release is received.

>  }
>  

If we are not able to avoid the copy and we need to keep the old
surface in place maybe instead of creating the new object as SURFACE
we could just use for source for the Drawable for the DRAW_COPY operation.
In this way when release is received the object can be unpinned being
free to be moved.

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

Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit

2017-10-19 Thread Frediano Ziglio 
> 
> Hi
> 
> - Original Message -
> > > 
> > > From: Christophe de Dinechin 
> > > 
> > > This is useful for some instrumentation, e.g. the leaks tracer,
> > > that perform some of their operations within gst_deinit.
> > > 
> > > Without this patch, if you run spicy with
> > >   GST_DEBUG="GST_TRACER:7" GST_TRACERS="leaks" spicy ...
> > > the leak tracer does not run at exit, because it runs in gst_deinit.
> > > 
> > > Signed-off-by: Christophe de Dinechin 
> > > ---
> > >  spice-common  | 2 +-
> > >  src/channel-display-gst.c | 1 +
> > >  2 files changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/spice-common b/spice-common
> > > index 429ad96..ba11de3 16
> > > --- a/spice-common
> > > +++ b/spice-common
> > > @@ -1 +1 @@
> > > -Subproject commit 429ad965371ceaaf60b81ccbed7da660ef9e0a94
> > > +Subproject commit ba11de3f3fd58d1b1a99bb62dd9e409e9961a78e
> > > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > > index f978602..c9ab9bf 100644
> > > --- a/src/channel-display-gst.c
> > > +++ b/src/channel-display-gst.c
> > > @@ -578,6 +578,7 @@ static gboolean gstvideo_init(void)
> > >  GError *err = NULL;
> > >  if (gst_init_check(NULL, NULL, )) {
> > >  success = 1;
> > > +atexit(gst_deinit);
> > >  } else {
> > >  spice_warning("Disabling GStreamer video support: %s",
> > >  err->message);
> > >  g_clear_error();
> > 
> > Calling atexit from a library is a bad idea.
> 
> And calling gst_deinit() from a library is also wrong.
> 
> 

If the main target is just spicy and knowing spicy is mainly a developer
tool you can add a call to gst_deinit in spicy.

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

Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit

2017-10-19 Thread Marc-André Lureau 
Hi

- Original Message -
> > 
> > From: Christophe de Dinechin 
> > 
> > This is useful for some instrumentation, e.g. the leaks tracer,
> > that perform some of their operations within gst_deinit.
> > 
> > Without this patch, if you run spicy with
> > GST_DEBUG="GST_TRACER:7" GST_TRACERS="leaks" spicy ...
> > the leak tracer does not run at exit, because it runs in gst_deinit.
> > 
> > Signed-off-by: Christophe de Dinechin 
> > ---
> >  spice-common  | 2 +-
> >  src/channel-display-gst.c | 1 +
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/spice-common b/spice-common
> > index 429ad96..ba11de3 16
> > --- a/spice-common
> > +++ b/spice-common
> > @@ -1 +1 @@
> > -Subproject commit 429ad965371ceaaf60b81ccbed7da660ef9e0a94
> > +Subproject commit ba11de3f3fd58d1b1a99bb62dd9e409e9961a78e
> > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > index f978602..c9ab9bf 100644
> > --- a/src/channel-display-gst.c
> > +++ b/src/channel-display-gst.c
> > @@ -578,6 +578,7 @@ static gboolean gstvideo_init(void)
> >  GError *err = NULL;
> >  if (gst_init_check(NULL, NULL, )) {
> >  success = 1;
> > +atexit(gst_deinit);
> >  } else {
> >  spice_warning("Disabling GStreamer video support: %s",
> >  err->message);
> >  g_clear_error();
> 
> Calling atexit from a library is a bad idea.

And calling gst_deinit() from a library is also wrong.

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

Re: [Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit

2017-10-19 Thread Frediano Ziglio 
> 
> From: Christophe de Dinechin 
> 
> This is useful for some instrumentation, e.g. the leaks tracer,
> that perform some of their operations within gst_deinit.
> 
> Without this patch, if you run spicy with
>   GST_DEBUG="GST_TRACER:7" GST_TRACERS="leaks" spicy ...
> the leak tracer does not run at exit, because it runs in gst_deinit.
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  spice-common  | 2 +-
>  src/channel-display-gst.c | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/spice-common b/spice-common
> index 429ad96..ba11de3 16
> --- a/spice-common
> +++ b/spice-common
> @@ -1 +1 @@
> -Subproject commit 429ad965371ceaaf60b81ccbed7da660ef9e0a94
> +Subproject commit ba11de3f3fd58d1b1a99bb62dd9e409e9961a78e
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index f978602..c9ab9bf 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -578,6 +578,7 @@ static gboolean gstvideo_init(void)
>  GError *err = NULL;
>  if (gst_init_check(NULL, NULL, )) {
>  success = 1;
> +atexit(gst_deinit);
>  } else {
>  spice_warning("Disabling GStreamer video support: %s",
>  err->message);
>  g_clear_error();

Calling atexit from a library is a bad idea.

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

[Spice-devel] [PATCH spice-gtk] Add call of gst_deinit at program exit

2017-10-19 Thread Christophe de Dinechin 
From: Christophe de Dinechin 

This is useful for some instrumentation, e.g. the leaks tracer,
that perform some of their operations within gst_deinit.

Without this patch, if you run spicy with
GST_DEBUG="GST_TRACER:7" GST_TRACERS="leaks" spicy ...
the leak tracer does not run at exit, because it runs in gst_deinit.

Signed-off-by: Christophe de Dinechin 
---
 spice-common  | 2 +-
 src/channel-display-gst.c | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/spice-common b/spice-common
index 429ad96..ba11de3 16
--- a/spice-common
+++ b/spice-common
@@ -1 +1 @@
-Subproject commit 429ad965371ceaaf60b81ccbed7da660ef9e0a94
+Subproject commit ba11de3f3fd58d1b1a99bb62dd9e409e9961a78e
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index f978602..c9ab9bf 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -578,6 +578,7 @@ static gboolean gstvideo_init(void)
 GError *err = NULL;
 if (gst_init_check(NULL, NULL, )) {
 success = 1;
+atexit(gst_deinit);
 } else {
 spice_warning("Disabling GStreamer video support: %s", 
err->message);
 g_clear_error();
-- 
2.13.5 (Apple Git-94)

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

Re: [Spice-devel] [PATCH] drm/qxl: workaround broken qxl hw primary setting.

2017-10-19 Thread Frediano Ziglio 
> 
> From: Dave Airlie 
> 
> QXL hw can't change primary surfaces easily, the server sends a msg
> and the client flickers a lo when it does. The old pre-atomic

typo: lo -> lot

> page flip code endeavoured to workaround this with a copy operation.
> 

not clear. Do you mean a memcpy like operation or a DRAW_COPY ?

> This worked by another accident of how the qxl virtual gpu is designed,
> it does lazy operation evaluation on the host, so this operation
> wouldn't generally trash the memory, and result in correct display.
> 
> This tries to work out when the commit is likely just a pageflip
> and avoid touching the primary surface, this might go wrong at
> some point but I believe it's the same level as wrong as the old code
> base.
> 

This sentence contains lot of hopes :-)

> Signed-off-by: Dave Airlie 
> ---
>  drivers/gpu/drm/qxl/qxl_display.c | 21 +++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c
> b/drivers/gpu/drm/qxl/qxl_display.c
> index afbf50d..3b702d1 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -502,6 +502,7 @@ static void qxl_primary_atomic_update(struct drm_plane
> *plane,
>   struct qxl_framebuffer *qfb_old;
>   struct qxl_bo *bo = gem_to_qxl_bo(qfb->obj);
>   struct qxl_bo *bo_old;
> + bool update_primary = true;
>   struct drm_clip_rect norect = {
>   .x1 = 0,
>   .y1 = 0,
> @@ -519,15 +520,31 @@ static void qxl_primary_atomic_update(struct drm_plane
> *plane,
>   if (bo == bo_old)
>   return;
>  
> + if (bo && bo_old &&
> + plane->state->crtc == old_state->crtc &&
> + plane->state->crtc_w == old_state->crtc_w &&
> + plane->state->crtc_h == old_state->crtc_h &&
> + plane->state->src_x == old_state->src_x &&
> + plane->state->src_y == old_state->src_y &&
> + plane->state->src_w == old_state->src_w &&
> + plane->state->src_h == old_state->src_h &&
> + plane->state->rotation == old_state->rotation &&
> + plane->state->zpos == old_state->zpos)
> + /* this is likely a pageflip */
> + update_primary = false;
> +
>   if (bo_old && bo_old->is_primary) {
> - qxl_io_destroy_primary(qdev);
> + if (update_primary)
> + qxl_io_destroy_primary(qdev);
>   bo_old->is_primary = false;
>   }
>  
>   if (!bo->is_primary) {
> - qxl_io_create_primary(qdev, 0, bo);
> + if (update_primary)
> + qxl_io_create_primary(qdev, 0, bo);
>   bo->is_primary = true;
>   }
> +
>   qxl_draw_dirty_fb(qdev, qfb, bo, 0, 0, , 1, 1);
>  }
>  

What is actually supposed to do this function?
It looks weird. Looks like it's mixing drawing and surfaces together.
I don't see the reason to issue a DRAW_COPY (qxl_draw_dirty_fb) if
you just created the primary surface with data.
Also there's a big difference with and without this patch.
You are not changing the primary surface so the frame buffer (exactly
where spice-server draws the commands in QXL memory and where a screen
capture is supposed to find updated pixel after an update command)
still refers to bo_old object. However I don't see a related change in
the life management of bo_old. So or is going to be broken or was broken
before.

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

Re: [Spice-devel] [PATCH spice-server 2/2] RedStream: make some functions static

2017-10-19 Thread Frediano Ziglio 
> 
> These were not used outside of red-stream.c, so make them static and
> remove them from the header.

Acked-by: Frediano Ziglio 

> ---
>  server/red-stream.c | 6 +++---
>  server/red-stream.h | 3 ---
>  2 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/server/red-stream.c b/server/red-stream.c
> index 6fdf1779f..6ce1642b4 100644
> --- a/server/red-stream.c
> +++ b/server/red-stream.c
> @@ -438,7 +438,7 @@ bool red_stream_is_ssl(RedStream *stream)
>  return (stream->priv->ssl != NULL);
>  }
>  
> -void red_stream_disable_writev(RedStream *stream)
> +static void red_stream_disable_writev(RedStream *stream)
>  {
>  stream->priv->writev = NULL;
>  }
> @@ -572,12 +572,12 @@ void red_stream_async_read(RedStream *stream,
>  }
>  
>  #if HAVE_SASL
> -bool red_stream_write_u8(RedStream *s, uint8_t n)
> +static bool red_stream_write_u8(RedStream *s, uint8_t n)
>  {
>  return red_stream_write_all(s, , sizeof(uint8_t));
>  }
>  
> -bool red_stream_write_u32(RedStream *s, uint32_t n)
> +static bool red_stream_write_u32(RedStream *s, uint32_t n)
>  {
>  return red_stream_write_all(s, , sizeof(uint32_t));
>  }
> diff --git a/server/red-stream.h b/server/red-stream.h
> index e6e37e3e7..05179d0a8 100644
> --- a/server/red-stream.h
> +++ b/server/red-stream.h
> @@ -53,9 +53,6 @@ void red_stream_set_async_error_handler(RedStream *stream,
>  ssize_t red_stream_write(RedStream *s, const void *buf, size_t nbyte);
>  ssize_t red_stream_writev(RedStream *s, const struct iovec *iov, int
>  iovcnt);
>  bool red_stream_write_all(RedStream *stream, const void *in_buf, size_t n);
> -bool red_stream_write_u8(RedStream *s, uint8_t n);
> -bool red_stream_write_u32(RedStream *s, uint32_t n);
> -void red_stream_disable_writev(RedStream *stream);
>  void red_stream_free(RedStream *s);
>  
>  void red_stream_push_channel_event(RedStream *s, int event);

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

Re: [Spice-devel] [PATCH spice-server 1/2] Use standard "Red" namespace

2017-10-19 Thread Frediano Ziglio 
> 
> The objects RedsStream and RedsSASL are currently using the namespace
> "Reds" rather than the standard "Red" namespace used throughout the rest
> of the project. Change these to be consistent. This also means changing
> method names and some related enumeration types.
> 
> The files were also renamed to reflect the change:
>   reds-stream.[ch] -> red-stream.[ch]
> ---
> 
> Too much code churn?
> 

Not too much worried. I'm more worried about the additional debug code,
streaming and DisplayChannel :-)

> 
>  server/Makefile.am |   4 +-
>  server/common-graphics-channel.c   |   4 +-
>  server/cursor-channel-client.c |   2 +-
>  server/cursor-channel-client.h |   4 +-
>  server/cursor-channel.c|   2 +-
>  server/cursor-channel.h|   2 +-
>  server/dcc-send.c  |   4 +-
>  server/dcc.c   |   8 +-
>  server/dcc.h   |   2 +-
>  server/display-channel.c   |  42 -
>  server/display-channel.h   |   2 +-
>  server/inputs-channel-client.c |   2 +-
>  server/inputs-channel-client.h |   2 +-
>  server/inputs-channel.c|   6 +-
>  server/main-channel-client.c   |   2 +-
>  server/main-channel-client.h   |   2 +-
>  server/main-channel.c  |   2 +-
>  server/main-channel.h  |   2 +-
>  server/red-channel-client.c|  28 ++--
>  server/red-channel-client.h|   4 +-
>  server/red-channel.c   |   6 +-
>  server/red-channel.h   |   6 +-
>  server/red-qxl.c   |   4 +-
>  server/{reds-stream.c => red-stream.c} | 284
>  -
>  server/red-stream.h|  93 +++
>  server/red-worker.h|   4 +-
>  server/reds-private.h  |   2 +-
>  server/reds-stream.h   |  93 ---
>  server/reds.c  | 162 +--
>  server/smartcard-channel-client.c  |   2 +-
>  server/smartcard-channel-client.h  |   2 +-
>  server/smartcard.c |   2 +-
>  server/sound.c |  10 +-
>  server/spicevmc.c  |  12 +-
>  server/stream-channel.c|   4 +-
>  server/stream.c|   9 +-
>  server/stream.h|   6 +-
>  server/tests/test-channel.c|   6 +-
>  server/tests/test-stream.c |  24 +--
>  39 files changed, 449 insertions(+), 408 deletions(-)
>  rename server/{reds-stream.c => red-stream.c} (74%)
>  create mode 100644 server/red-stream.h
>  delete mode 100644 server/reds-stream.h
> 
> diff --git a/server/Makefile.am b/server/Makefile.am
> index e2e3ce861..20f0f1925 100644
> --- a/server/Makefile.am
> +++ b/server/Makefile.am
> @@ -152,8 +152,8 @@ libserver_la_SOURCES =\
>   reds.c  \
>   reds.h  \
>   reds-private.h  \
> - reds-stream.c   \
> - reds-stream.h   \
> + red-stream.c\
> + red-stream.h\
>   red-worker.c\
>   red-worker.h\
>   sound.c \
> diff --git a/server/common-graphics-channel.c
> b/server/common-graphics-channel.c
> index 0cbc2762c..ce6b5e57c 100644
> --- a/server/common-graphics-channel.c
> +++ b/server/common-graphics-channel.c
> @@ -78,7 +78,7 @@ bool common_channel_client_config_socket(RedChannelClient
> *rcc)
>  {
>  RedClient *client = red_channel_client_get_client(rcc);
>  MainChannelClient *mcc = red_client_get_main(client);
> -RedsStream *stream = red_channel_client_get_stream(rcc);
> +RedStream *stream = red_channel_client_get_stream(rcc);
>  gboolean is_low_bandwidth;
>  
>  // TODO - this should be dynamic, not one time at channel creation
> @@ -89,7 +89,7 @@ bool common_channel_client_config_socket(RedChannelClient
> *rcc)
>   * the application level.
>   * see: http://www.stuartcheshire.org/papers/NagleDelayedAck/
>   */
> -reds_stream_set_no_delay(stream, !is_low_bandwidth);
> +red_stream_set_no_delay(stream, !is_low_bandwidth);
>  
>  // TODO: move wide/narrow ack setting to red_channel.
>  red_channel_client_ack_set_client_window(rcc,
> diff --git a/server/cursor-channel-client.c b/server/cursor-channel-client.c
> index 42ab5d763..6d39e24ed 100644
> --- a/server/cursor-channel-client.c
> +++ b/server/cursor-channel-client.c
> @@ -97,7 +97,7 @@ void cursor_channel_client_migrate(RedChannelClient *rcc)
>  red_channel_client_default_migrate(rcc);
>  }
>  
> -CursorChannelClient* cursor_channel_client_new(CursorChannel *cursor,
>