Re: [Spice-devel] Fedora 19 and 20 spice guest Xorg crashes

2013-11-19 Thread Nahum Shalman 

On 11/18/2013 05:32 PM, Dave Airlie wrote:

On Tue, Nov 19, 2013 at 7:04 AM, Nahum Shalman  wrote:

Context:
Host is running qemu-kvm 1.1.2 and spice 0.12.2.
Fedora 16 VMs ran rock solid on these same virtualization hosts.
The Fedora 19 and 20(testing) VMs are running xf86-video-qxl compiled from
the master branch of the git repo.

We've been seeing a lot of X server crashes in Fedora 19 and 20, generally
after the VM has been running for at least 2-3 days.
The last gasp in the Xorg logs from these crashes generally look something
like:

[1024592.839] Out of memory allocating 261140 bytes
[1024592.839] Out of mem - stats

[1024592.850] max system bytes =  243257344
[1024592.850] system bytes =  243257344
[1024592.850] in use bytes =  133245384

Someone here managed to get a stack trace out of one such crash:

(EE) [mi] EQ overflowing. Additional events will be discarded until existing
events are processed.
(EE)
(EE) Backtrace:
(EE) 0: /usr/bin/X (mieqEnqueue+0x22b) [0x57691b]
(EE) 1: /usr/bin/X (QueuePointerEvents+0x52) [0x44d862]
(EE) 2: /usr/lib64/xorg/modules/input/evdev_drv.so (_init+0x2913)
[0x7ff0faeb17e3]
(EE) 3: /usr/bin/X (DPMSSupported+0xe8) [0x4861f8]
(EE) 4: /usr/bin/X (xf86SerialModemClearBits+0x230) [0x4ae7b0]
(EE) 5: /lib64/libpthread.so.0 (__restore_rt+0x0) [0x3b7de0ef9f]
(EE) 6: /lib64/libpthread.so.0 (__nanosleep_nocancel+0x24) [0x3b7de0e804]
(EE) 7: /usr/lib64/xorg/modules/drivers/qxl_drv.so (qxl_handle_oom+0x69)
[0x7ff10fceccb9]
(EE) 8: /usr/lib64/xorg/modules/drivers/qxl_drv.so (qxl_allocnf+0x48)
[0x7ff10fcecd08]
(EE) 9: /usr/lib64/xorg/modules/drivers/qxl_drv.so
(qxl_bo_alloc_internal+0x76) [0x7ff10fcece06]
(EE) 10: /usr/lib64/xorg/modules/drivers/qxl_drv.so (qxl_image_create+0xf2)
[0x7ff10fce9782]
(EE) 11: /usr/lib64/xorg/modules/drivers/qxl_drv.so
(qxl_surface_put_image+0xf5) [0x7ff10fceb045]
(EE) 12: /usr/lib64/xorg/modules/drivers/qxl_drv.so (uxa_copy_n_to_n+0x5e7)
[0x7ff10fcf7127]
(EE) 13: /usr/bin/X (miCopyRegion+0x1ad) [0x574d2d]
(EE) 14: /usr/bin/X (miDoCopy+0x456) [0x5752b6]
(EE) 15: /usr/lib64/xorg/modules/drivers/qxl_drv.so (uxa_copy_area+0xae)
[0x7ff10fcf5efe]
(EE) 16: /usr/bin/X (dixDestroyPixmap+0x711) [0x433a31]
(EE) 17: /usr/bin/X (SendErrorToClient+0x3f7) [0x436fa7]
(EE) 18: /usr/bin/X (_init+0x3aaa) [0x429b4a]
(EE) 19: /lib64/libc.so.6 (__libc_start_main+0xf5) [0x3b7d221b75]
(EE) 20: /usr/bin/X (_start+0x29) [0x4267b1]
(EE) 21: ? (?+0x29) [0x29]
(EE)
(EE) [mi] These backtraces from mieqEnqueue may point to a culprit higher up
the stack.
(EE) [mi] mieq is NOT the cause. It is a victim.
(EE) [mi] EQ overflow continuing. 100 events have been dropped.

His comment was:

Examining the stack trace more closely, the functions identified are
misleading. The offsets are sometimes larger than the named functions, and
point to different functions not listed in the stripped symbol table.
Looking at the source, it seems that:

(EE) 16: /usr/bin/X (dixDestroyPixmap+0x711) [0x433a31]

This is probably ProcCreatePixmap()

(EE) 17: /usr/bin/X (SendErrorToClient+0x3f7) [0x436fa7]

This is possibly init_screen() or AddScreen()

So, it appears the memory allocation fails while setting up a new screen
structure. This makes more sense, but still leaves open the question why
it's trying to create new screens long after startup.

It's hard to recreate the crashes other than by simply booting and using a
VM for a few days. One theory we're tossing around is that the memory buffer
xf86-video-qxl has to work with is getting fragmented and when the
fragmentation gets bad enough an allocation can fail.
Our best guess is that this is a bug in the xf86-video-qxl driver. Has
anyone else seen similar Xorg crashes?

Guidance on how to fix or at least troubleshoot this further would be
greatly appreciated.


Why aren't you running the Fedora packages?


When we were using the Fedora packages under F19 we were able to trigger 
a crash much more easily:
Specifically we had a script that would repeated launch Firefox, 
Thunderbird, Google Chrome, and Gedit, wait 5 seconds, then kill all 4 
applications.

That script could trigger an X server crash within a couple of hours.

When we switched to compiling the master branch, that script didn't 
crash the X server at all, but normal use still causes the crashes 
described in my previous email.


Thanks!
-Nahum
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] Fedora 19 and 20 spice guest Xorg crashes

2013-11-18 Thread Nahum Shalman 

Context:
Host is running qemu-kvm 1.1.2 and spice 0.12.2.
Fedora 16 VMs ran rock solid on these same virtualization hosts.
The Fedora 19 and 20(testing) VMs are running xf86-video-qxl compiled 
from the master branch of the git repo.


We've been seeing a lot of X server crashes in Fedora 19 and 20, 
generally after the VM has been running for at least 2-3 days.
The last gasp in the Xorg logs from these crashes generally look 
something like:


[1024592.839] Out of memory allocating 261140 bytes
[1024592.839] Out of mem - stats

[1024592.850] max system bytes =  243257344
[1024592.850] system bytes =  243257344
[1024592.850] in use bytes =  133245384

Someone here managed to get a stack trace out of one such crash:

(EE) [mi] EQ overflowing. Additional events will be discarded until 
existing events are processed.

(EE)
(EE) Backtrace:
(EE) 0: /usr/bin/X (mieqEnqueue+0x22b) [0x57691b]
(EE) 1: /usr/bin/X (QueuePointerEvents+0x52) [0x44d862]
(EE) 2: /usr/lib64/xorg/modules/input/evdev_drv.so (_init+0x2913) 
[0x7ff0faeb17e3]

(EE) 3: /usr/bin/X (DPMSSupported+0xe8) [0x4861f8]
(EE) 4: /usr/bin/X (xf86SerialModemClearBits+0x230) [0x4ae7b0]
(EE) 5: /lib64/libpthread.so.0 (__restore_rt+0x0) [0x3b7de0ef9f]
(EE) 6: /lib64/libpthread.so.0 (__nanosleep_nocancel+0x24) [0x3b7de0e804]
(EE) 7: /usr/lib64/xorg/modules/drivers/qxl_drv.so (qxl_handle_oom+0x69) 
[0x7ff10fceccb9]
(EE) 8: /usr/lib64/xorg/modules/drivers/qxl_drv.so (qxl_allocnf+0x48) 
[0x7ff10fcecd08]
(EE) 9: /usr/lib64/xorg/modules/drivers/qxl_drv.so 
(qxl_bo_alloc_internal+0x76) [0x7ff10fcece06]
(EE) 10: /usr/lib64/xorg/modules/drivers/qxl_drv.so 
(qxl_image_create+0xf2) [0x7ff10fce9782]
(EE) 11: /usr/lib64/xorg/modules/drivers/qxl_drv.so 
(qxl_surface_put_image+0xf5) [0x7ff10fceb045]
(EE) 12: /usr/lib64/xorg/modules/drivers/qxl_drv.so 
(uxa_copy_n_to_n+0x5e7) [0x7ff10fcf7127]

(EE) 13: /usr/bin/X (miCopyRegion+0x1ad) [0x574d2d]
(EE) 14: /usr/bin/X (miDoCopy+0x456) [0x5752b6]
(EE) 15: /usr/lib64/xorg/modules/drivers/qxl_drv.so (uxa_copy_area+0xae) 
[0x7ff10fcf5efe]

(EE) 16: /usr/bin/X (dixDestroyPixmap+0x711) [0x433a31]
(EE) 17: /usr/bin/X (SendErrorToClient+0x3f7) [0x436fa7]
(EE) 18: /usr/bin/X (_init+0x3aaa) [0x429b4a]
(EE) 19: /lib64/libc.so.6 (__libc_start_main+0xf5) [0x3b7d221b75]
(EE) 20: /usr/bin/X (_start+0x29) [0x4267b1]
(EE) 21: ? (?+0x29) [0x29]
(EE)
(EE) [mi] These backtraces from mieqEnqueue may point to a culprit 
higher up the stack.

(EE) [mi] mieq is *NOT* the cause. It is a victim.
(EE) [mi] EQ overflow continuing. 100 events have been dropped.

His comment was:

Examining the stack trace more closely, the functions identified are 
misleading. The offsets are sometimes larger than the named functions, 
and point to different functions not listed in the stripped symbol 
table. Looking at the source, it seems that:


(EE) 16: /usr/bin/X (dixDestroyPixmap+0x711) [0x433a31]

 * This is probably ProcCreatePixmap()

(EE) 17: /usr/bin/X (SendErrorToClient+0x3f7) [0x436fa7]

 * This is possibly init_screen() or AddScreen()

So, it appears the memory allocation fails while setting up a new screen 
structure. This makes more sense, but still leaves open the question why 
it's trying to create new screens long after startup.


It's hard to recreate the crashes other than by simply booting and using 
a VM for a few days. One theory we're tossing around is that the memory 
buffer xf86-video-qxl has to work with is getting fragmented and when 
the fragmentation gets bad enough an allocation can fail.
Our best guess is that this is a bug in the xf86-video-qxl driver. Has 
anyone else seen similar Xorg crashes?


Guidance on how to fix or at least troubleshoot this further would be 
greatly appreciated.


Thanks!
-Nahum

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

Re: [Spice-devel] [PATCH] server/red_channel: fix unused variable

2013-07-29 Thread Nahum Shalman 

On 07/28/2013 03:14 PM, Alon Levy wrote:

unused variable 'so_unsent_size' [-Werror=unused-variable]
---
Nahum, could you check that this works for you?

Works for me*. Sorry for introducing a compiler error!
-Nahum

* As much as the other patch works. It compiles and runs, but I get a 
freeze the first time video detection is triggered...




  server/red_channel.c | 23 +--
  1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/server/red_channel.c b/server/red_channel.c
index 31c991b..33af388 100644
--- a/server/red_channel.c
+++ b/server/red_channel.c
@@ -720,22 +720,25 @@ static void red_channel_client_push_ping(RedChannelClient 
*rcc)

  static void red_channel_client_ping_timer(void *opaque)
  {
-int so_unsent_size = 0;
  RedChannelClient *rcc = opaque;

  spice_assert(rcc->latency_monitor.state == PING_STATE_TIMER);
  red_channel_client_cancel_ping_timer(rcc);

  #ifdef HAVE_LINUX_SOCKIOS_H /* SIOCOUTQ is a Linux only ioctl on sockets. */
-/* retrieving the occupied size of the socket's tcp snd buffer (unacked + 
unsent) */
-if (ioctl(rcc->stream->socket, SIOCOUTQ,&so_unsent_size) == -1) {
-spice_printerr("ioctl(SIOCOUTQ) failed, %s", strerror(errno));
-}
-if (so_unsent_size>  0) {
-/* tcp snd buffer is still occupied. rescheduling ping */
-red_channel_client_start_ping_timer(rcc, 
PING_TEST_IDLE_NET_TIMEOUT_MS);
-} else {
-red_channel_client_push_ping(rcc);
+{
+int so_unsent_size = 0;
+
+/* retrieving the occupied size of the socket's tcp snd buffer 
(unacked + unsent) */
+if (ioctl(rcc->stream->socket, SIOCOUTQ,&so_unsent_size) == -1) {
+spice_printerr("ioctl(SIOCOUTQ) failed, %s", strerror(errno));
+}
+if (so_unsent_size>  0) {
+/* tcp snd buffer is still occupied. rescheduling ping */
+red_channel_client_start_ping_timer(rcc, 
PING_TEST_IDLE_NET_TIMEOUT_MS);
+} else {
+red_channel_client_push_ping(rcc);
+}
  }
  #else /* ifdef HAVE_LINUX_SOCKIOS_H */
  /* More portable alternative code path (less accurate but avoids bogus 
ioctls)*/


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

[Spice-devel] [(spice) PATCHv2 RFC 2/2] TIOCOUTQ -> SIOCOUTQ and portability ifdefs

2013-07-18 Thread Nahum Shalman 
The ioctl on sockets is actually named SIOCOUTQ though its value
is identical to TIOCOUTQ which is for terminals.
SIOCOUTQ is linux specific so we add a header check and ifdef based
on the presence of the header
This prevents bogus ioctls on non-Linux platforms
---
 configure.ac |1 +
 server/red_channel.c |   13 +++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index a549ed9..fa1ba31 100644
--- a/configure.ac
+++ b/configure.ac
@@ -51,6 +51,7 @@ PKG_PROG_PKG_CONFIG
 
 AC_CHECK_HEADERS([sys/time.h])
 AC_CHECK_HEADERS([execinfo.h])
+AC_CHECK_HEADERS([linux/sockios.h])
 AC_FUNC_ALLOCA
 
 AC_DEFINE([__STDC_FORMAT_MACROS],[],[Force definition of format macros for 
C++])
diff --git a/server/red_channel.c b/server/red_channel.c
index 85d7ebc..31c991b 100644
--- a/server/red_channel.c
+++ b/server/red_channel.c
@@ -30,6 +30,9 @@
 #include 
 #include 
 #include 
+#ifdef HAVE_LINUX_SOCKIOS_H
+#include  /* SIOCOUTQ */
+#endif
 
 #include "common/generated_server_marshallers.h"
 #include "common/ring.h"
@@ -722,9 +725,11 @@ static void red_channel_client_ping_timer(void *opaque)
 
 spice_assert(rcc->latency_monitor.state == PING_STATE_TIMER);
 red_channel_client_cancel_ping_timer(rcc);
+
+#ifdef HAVE_LINUX_SOCKIOS_H /* SIOCOUTQ is a Linux only ioctl on sockets. */
 /* retrieving the occupied size of the socket's tcp snd buffer (unacked + 
unsent) */
-if (ioctl(rcc->stream->socket, TIOCOUTQ, &so_unsent_size) == -1) {
-spice_printerr("ioctl(TIOCOUTQ) failed, %s", strerror(errno));
+if (ioctl(rcc->stream->socket, SIOCOUTQ, &so_unsent_size) == -1) {
+spice_printerr("ioctl(SIOCOUTQ) failed, %s", strerror(errno));
 }
 if (so_unsent_size > 0) {
 /* tcp snd buffer is still occupied. rescheduling ping */
@@ -732,6 +737,10 @@ static void red_channel_client_ping_timer(void *opaque)
 } else {
 red_channel_client_push_ping(rcc);
 }
+#else /* ifdef HAVE_LINUX_SOCKIOS_H */
+/* More portable alternative code path (less accurate but avoids bogus 
ioctls)*/
+red_channel_client_push_ping(rcc);
+#endif /* ifdef HAVE_LINUX_SOCKIOS_H */
 }
 
 RedChannelClient *red_channel_client_create(int size, RedChannel *channel, 
RedClient  *client,
-- 
1.7.7.6

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

[Spice-devel] [(spice) PATCH RFC 2/2] TIOCOUTQ -> SIOCOUTQ and portability ifdefs

2013-07-18 Thread Nahum Shalman 
The ioctl on sockets is actually named SIOCOUTQ though its value
is identical to TIOCOUTQ which is for terminals.
SIOCOUTQ is linux specific so we add a header check and ifdef based
on the presence of the header / macro
This prevents bogus ioctls on non-Linux platforms
---
 configure.ac |1 +
 server/red_channel.c |   14 --
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index a549ed9..fa1ba31 100644
--- a/configure.ac
+++ b/configure.ac
@@ -51,6 +51,7 @@ PKG_PROG_PKG_CONFIG
 
 AC_CHECK_HEADERS([sys/time.h])
 AC_CHECK_HEADERS([execinfo.h])
+AC_CHECK_HEADERS([linux/sockios.h])
 AC_FUNC_ALLOCA
 
 AC_DEFINE([__STDC_FORMAT_MACROS],[],[Force definition of format macros for 
C++])
diff --git a/server/red_channel.c b/server/red_channel.c
index 85d7ebc..4b92a3b 100644
--- a/server/red_channel.c
+++ b/server/red_channel.c
@@ -30,6 +30,9 @@
 #include 
 #include 
 #include 
+#ifdef HAVE_LINUX_SOCKIOS_H
+#include  /* SIOCOUTQ */
+#endif
 
 #include "common/generated_server_marshallers.h"
 #include "common/ring.h"
@@ -722,9 +725,11 @@ static void red_channel_client_ping_timer(void *opaque)
 
 spice_assert(rcc->latency_monitor.state == PING_STATE_TIMER);
 red_channel_client_cancel_ping_timer(rcc);
+
+#ifdef SIOCOUTQ /* SIOCOUTQ is a Linux only ioctl on sockets. */
 /* retrieving the occupied size of the socket's tcp snd buffer (unacked + 
unsent) */
-if (ioctl(rcc->stream->socket, TIOCOUTQ, &so_unsent_size) == -1) {
-spice_printerr("ioctl(TIOCOUTQ) failed, %s", strerror(errno));
+if (ioctl(rcc->stream->socket, SIOCOUTQ, &so_unsent_size) == -1) {
+spice_printerr("ioctl(SIOCOUTQ) failed, %s", strerror(errno));
 }
 if (so_unsent_size > 0) {
 /* tcp snd buffer is still occupied. rescheduling ping */
@@ -732,6 +737,11 @@ static void red_channel_client_ping_timer(void *opaque)
 } else {
 red_channel_client_push_ping(rcc);
 }
+#endif /* ifdef SIOCOUTQ */
+
+#ifndef SIOCOUTQ /* More portable alternative code path (less accurate but 
avoids bogus ioctls)*/
+red_channel_client_push_ping(rcc);
+#endif /* ifndef SIOCOUTQ */
 }
 
 RedChannelClient *red_channel_client_create(int size, RedChannel *channel, 
RedClient  *client,
-- 
1.7.7.6

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

[Spice-devel] [(spice) PATCH RFC 0/2] Portability patch for non-Linux

2013-07-18 Thread Nahum Shalman 
This patch series includes one typo correction and one patch
for which I'd like feedback.

I don't fully understand the latency monitoring system, but I
did manage to figure out two interesting pieces of information:

1. TIOCOUTQ and SIOCOUTQ are the same number
$ grep TIOCOUTQ /usr/include/linux/sockios.h
#define SIOCOUTQTIOCOUTQ/* output queue size (not sent + not 
acked) */

2. SIOCOUTQ is Linux only (as hinted by the fact that the header is a linux 
one.)

Other platforms (I'm on illumos) don't support reading the length of the TCP 
output 
queue. I've tried to write a simple patch with some reasonable ifdef'ery to mark
that code Linux only and to fall back to a less ideal, but hopefully acceptable
alternative.

Comments very much requested.

Thanks!

-Nahum

 configure.ac |3 ++-
 server/red_channel.c |   14 --
 2 files changed, 14 insertions(+), 3 deletions(-)
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [(spice) PATCH RFC 1/2] configure.ac comment typo nit

2013-07-18 Thread Nahum Shalman 
---
 configure.ac |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/configure.ac b/configure.ac
index edda8e9..a549ed9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -20,7 +20,7 @@ m4_define([SPICE_AGE], [8])
 # Note on the library name on linux (SONAME) produced by libtool (for 
reference, gleaned
 # from looking at libtool 2.4.2)
 #
-#  libspice-servver.so.current-age.age.revision
+#  libspice-server.so.current-age.age.revision
 
 AC_INIT(spice, [m4_esyscmd(build-aux/git-version-gen .tarball-version)],
 [spice-devel@lists.freedesktop.org], spice)
-- 
1.7.7.6

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

Re: [Spice-devel] Fedora 17 Guests for Desktop Virtualization

2013-01-08 Thread Nahum Shalman 
Below I've pasted a stripped down version of a kickstart file I used to 
get Fedora 17 working as a spice VM.

(original is at http://www.shalman.org/spice/fedora17.ks)

It gave me a working KDE VM.

A plain install of Fedora 17 seemed to freeze for me, but I haven't done 
any digging into the differences between that plain install and the 
stuff in this kickstart file (modified from an older version for Fedora 16).


Kevin, perhaps you could give this kickstart file (or the full original) 
a try and see if that helps (the install will still prompt for a few 
details)?


As an aside, it might be interesting for there to be a yum group that 
installs all the right packages for a VM that uses spice so that someday 
one could run "yum groupinstall spicevm" or put "@spicevm" in a 
kickstart file and have everything "Just Work"...


-Nahum

--

install
url --url=http://mirrors.us.kernel.org/fedora/releases/17/Fedora/x86_64/os/
reboot
lang en_US.UTF-8
keyboard us
network --onboot yes --device eth0 --bootproto dhcp --noipv6
authconfig --enableshadow --passalgo=sha512
selinux --disabled
firewall --disabled

repo --name="Fedora 17" 
--baseurl=http://mirrors.us.kernel.org/fedora/releases/17/Fedora/x86_64/os/
repo --name="Fedora 17 updates" 
--baseurl=http://mirrors.us.kernel.org/fedora/updates/17/x86_64/


%packages
@Base
@Fonts
@Core
@online-docs
xorg-x11-drv-qxl
xorg-x11-drv-evdev
spice-vdagent
kdm
konsole
kde-settings-pulseaudio
kdemultimedia-kmix
kwin-gles
xorg-x11-xauth
xorg-x11-xinit
xorg-x11-utils
xorg-x11-server-Xorg
xorg-x11-server-utils
xorg-x11-fonts-*
xorg-x11-font-utils
firstboot
smolt-firstboot
less
vim
openssh-clients
openssh-server
pulseaudio
phonon-backend-gstreamer
libusb*
usbutils*
usbredir*
%end
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] Tripping assertions in spice-common/common/ring.h

2012-07-25 Thread Nahum Shalman 

On 07/25/2012 07:39 AM, Yonit Halperin wrote:
Thanks for the detailed description. I've reproduced and found the 
bug. Just sent the patch. You are welcomed to try it :)


That patch seems to be working for me as well.
+1

Thanks!
-Nahum
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] Tripping assertions in spice-common/common/ring.h

2012-07-23 Thread Nahum Shalman 

On 07/22/2012 02:14 PM, Yonit Halperin wrote:

Hi,
the trace implies some problem in the drawables tree structure. Please 
describe in more details how you reproduce it: which web site, which 
browser you use, what operations you execute on the web site? In 
addition, what is the qxl driver version, and are surfaces and/or 
caching enabled?

What is the spice-server version? Is it a modified version?


So far I've trimmed the crash process down to:
1. Boot the Fedora 16 VM
2. Log in, icewm is the window manager.
3. Launch google-chrome
4. Open pandora.com and get some music playing (serves as a non-visual 
cue that the VM has crashed, doesn't need to be visible on-screen)
5. Open another tab and go to http://arbesman.net (make sure that the 
cross-fading images are on-screen and are cross-fading correctly 
(sometimes needs a refresh of the web page))
6. In a little while the VM will crash enough that the music stops and 
the UI becomes unresponsive (but QEMU hasn't crashed)


The spice-server is the master branch from the spice git repositories 
(as of ~July 17, I think) with two patches applied:

One is the ENOPROTOOPT patch that I just sent to the mailing list
The second is the alteration of the spice_assert macro to invoke 
spice_backtrace on errors (which enabled me to trace the crash).


The QEMU server has some modifications including that it is listening on 
a UNIX socket and a different process is listening on a TCP socket and 
passing the connection through (thus the need for the ENOPROTOOPT 
patch). None of the modifications touch the qxl device code.


I'm not sure of the qxl driver version, we may have made a small 
modification to it to alter which screen resolutions it would run, but 
it's probably relatively old.
The offscreen surfaces are disabled (they were giving us a performance 
regression with GTK applications like firefox), but the image and 
fallback caches are enabled.

Here's some of the output it placed in Xorg.0.log:
[ 8.183] (II) LoadModule: "qxl"
[ 8.183] (II) Loading /usr/lib64/xorg/modules/drivers/qxl_drv.so
[ 8.185] (II) Module qxl: vendor="X.Org Foundation"
[ 8.185] (II) qxl: Driver for QXL virtual graphics: QXL 1
[ 8.201] (II) Loading /usr/lib64/xorg/modules/drivers/qxl_drv.so
[ 8.201] (**) qxl(0): Depth 24, (--) framebuffer bpp 32
[ 8.201] (==) qxl(0): RGB weight 888
[ 8.201] (==) qxl(0): Default visual is TrueColor
[ 8.201] (==) qxl(0): Using gamma correction (1.0, 1.0, 1.0)
[ 8.201] (II) qxl(0): Offscreen Surfaces: Disabled
[ 8.201] (II) qxl(0): Image Cache: Enabled
[ 8.201] (II) qxl(0): Fallback Cache: Enabled
[ 8.209] (II) qxl(0): framebuffer at 0x7f67dfcb1000 (16384 KB)
[ 8.209] (II) qxl(0): command ram at 0x7f67e0cb1000 (32760 KB)
[ 8.209] (II) qxl(0): vram at 0x7f67dbcb1000 (65536 KB)
[ 8.209] (II) qxl(0): rom at 0x7f67e4bc9000
[ 8.210] (II) qxl(0): Device version 0.0
[ 8.210] (II) qxl(0): Compression level 0, log level 0
[ 8.210] (II) qxl(0): 12286 io pages at 0x7f67dfcb1000
[ 8.210] (II) qxl(0): RAM header offset: 0x3ffe000
[ 8.210] (II) qxl(0): Correct RAM signature 41525851
[ 8.210] (II) qxl(0): 49144 KB of video RAM
[ 8.210] (II) qxl(0): 1024 surfaces
[ 8.210] (II) qxl(0): : Using hsync range of 
29.00-160.00 kHz
[ 8.210] (II) qxl(0): : Using vrefresh range of 
50.00-75.00 Hz

[ 8.210] (II) qxl(0): Clock range:  10.00 to 400.00 MHz

[ 8.219] (II) qxl(0): PreInit complete
[ 8.219] (II) qxl(0): git commit 5fc9ef5
[ 8.225] (II) qxl(0): framebuffer at 0x7f67dfcb1000 (16384 KB)
[ 8.225] (II) qxl(0): command ram at 0x7f67e0cb1000 (32760 KB)
[ 8.225] (II) qxl(0): vram at 0x7f67dbcb1000 (65536 KB)
[ 8.225] (II) qxl(0): rom at 0x7f67e4bc9000

Let me know if you need additional details or have variations of my test 
you'd like me to try.


Thanks so much,
Nahum

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

[Spice-devel] [PATCH] server: don't fail on ENOPROTOOPT from setsockopt

2012-07-20 Thread Nahum Shalman 
If we allow listening on arbitrary sockets like unix sockets,
we can get ENOPROTOOPT errors from setsockopt calls that set TCP
specific options.  This should be allowed to happen.

This time it's reds.c

see also: 20c7323c9efb22c1aae37557814f21cf58c2a322
---
 server/reds.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/server/reds.c b/server/reds.c
index c33c86c..0c6b7ba 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -2525,7 +2525,7 @@ static RedLinkInfo *reds_init_client_connection(int 
socket)
 }
 
 if (setsockopt(socket, IPPROTO_TCP, TCP_NODELAY, &delay_val, 
sizeof(delay_val)) == -1) {
-if (errno != ENOTSUP) {
+if (errno != ENOTSUP && errno != ENOPROTOOPT) {
 spice_error("setsockopt failed, %s", strerror(errno));
 }
 }
-- 
1.7.7.2

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

[Spice-devel] Tripping assertions in spice-common/common/ring.h

2012-07-19 Thread Nahum Shalman 

I need help tracking this down:
((null):66324): SpiceWorker-ERROR **: 
../spice-common/common/ring.h:56:ring_add: assertion `ring->next != NULL 
&& ring->prev != NULL' failed


I've been seeing this from time to time on my VMs.
When it happens, the UI stops being responsive, but QEMU doesn't crash.

I've avoided reporting it because I was using a non-release version of QEMU.
I've recently had help getting the Solaris(ish) patches applied to QEMU 
1.1.0 and using that, I still tripped the assert.
I launched a VM (Fedora 16), opened a browser and went to a site that 
has some weird overly fancy graphics and poked around on it for a little 
while until everything locked up.

And there in the log was my old friend the assertion failure in ring.h

I'm pretty sure the culprit is somewhere in the Spice code base.



In the end I added a call to spice_backtrace to the spice_assert macro, 
and while the backtrace it provides wasn't all that helpful
("main_channel_handle_parsed: agent start" was all I saw in the logs), 
it gave me a probe point for dtrace.



Here's the dtrace script I used to hunt down the error:

#!/usr/sbin/dtrace -s

#pragma D option quiet

pid$target:libspice-server.so.1:ring_add:entry
{
self->follow = 1;
}

pid$target:libspice-server.so.1:ring_add:return
/self->follow >= 1/
{
self->follow = 0;
/*  printf("ring_add returned cleanly\n"); */
}

pid$target:libspice-server.so.1:spice_backtrace:entry
/self->follow >= 1/
{
printf("called spice_backtrace from ring_add\n");
ustack();
}


And this is the output I got when the VM decided to hang:

called spice_backtrace from ring_add

  libspice-server.so.1`spice_backtrace
  libspice-server.so.1`ring_add+0x2e
  libspice-server.so.1`ring_add_after+0x23
  libspice-server.so.1`__current_add_drawable+0x61
  libspice-server.so.1`red_current_add_equal+0xee
  libspice-server.so.1`red_current_add+0x18d
  libspice-server.so.1`red_current_add_qxl+0xa7
  libspice-server.so.1`red_process_drawable+0x223
  libspice-server.so.1`red_process_commands+0x231
  libspice-server.so.1`handle_dev_oom+0x13a
  libspice-server.so.1`dispatcher_handle_single_read+0x14b
  libspice-server.so.1`dispatcher_handle_recv_read+0x19
  libspice-server.so.1`handle_dev_input+0x32
  libspice-server.so.1`red_worker_main+0x287
  libc.so.1`_thrp_setup+0x83
  libc.so.1`_lwp_start

called spice_backtrace from ring_add

  libspice-server.so.1`spice_backtrace
  libspice-server.so.1`spice_logv+0x1e7
  libspice-server.so.1`spice_log+0xbc
  libspice-server.so.1`ring_add+0x59
  libspice-server.so.1`ring_add_after+0x23
  libspice-server.so.1`__current_add_drawable+0x61
  libspice-server.so.1`red_current_add_equal+0xee
  libspice-server.so.1`red_current_add+0x18d
  libspice-server.so.1`red_current_add_qxl+0xa7
  libspice-server.so.1`red_process_drawable+0x223
  libspice-server.so.1`red_process_commands+0x231
  libspice-server.so.1`handle_dev_oom+0x13a
  libspice-server.so.1`dispatcher_handle_single_read+0x14b
  libspice-server.so.1`dispatcher_handle_recv_read+0x19
  libspice-server.so.1`handle_dev_input+0x32
  libspice-server.so.1`red_worker_main+0x287
  libc.so.1`_thrp_setup+0x83
  libc.so.1`_lwp_start

Does this give any insight into what I might be suffering from?

-Nahum

P.S. In addition to those errors, I've also seen some of:
((null):7636): SpiceWorker-ERROR **: 
../spice-common/common/ring.h:83:ring_remove: assertion `item->next != 
NULL && item->prev != NULL' failed
((null):8838): SpiceWorker-ERROR **: 
../spice-common/common/ring.h:84:ring_remove: assertion `item->next != 
item' failed


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

Re: [Spice-devel] xf86-video-qxl performance

2012-05-24 Thread Nahum Shalman 

Jeremy White wrote:

I need to improve the performance of the xf86-video-qxl driver; aka
xspice; by a fairly substantial margin.

I've set up a test case - LibreOffice over an 80 ms latency connection -
that demonstrates that it's got quite a long way to go.  (The current
case appears to suffer from an excessive set of draw fill operations).


We were seeing some bad performance from Firefox as compared with Chrome.
Recompiling the xorg driver with the offscreen surfaces disabled 
eliminated the performance discrepancy.


I'd be curious if you would see an improvement in LibreOffice 
performance by doing the same thing...


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

[Spice-devel] [PATCH v2] cleanup x11 library detection for building client

2012-05-03 Thread Nahum Shalman 
Consolidate two separate chunks of library hunting that depend on the
same check.

Check if we're actually building the client before looking for
client only libraries.

Hide some of the final output if we're not building the client.
---
I think this should address the rest of the concerns about
the previous version of this patch.

 configure.ac |   21 +
 1 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/configure.ac b/configure.ac
index 4b24c7d..6853d05 100644
--- a/configure.ac
+++ b/configure.ac
@@ -183,7 +183,7 @@ 
AC_DEFINE_UNQUOTED([POSIX_YIELD_FUNC],$posix_yield_func,[The POSIX RT yield func
 
 SPICE_REQUIRES=""
 
-if test "x$enable_gui" = "xyes"; then
+if test "x$enable_gui" = "xyes" && test "x$enable_client" = "xyes" ; then
 PKG_CHECK_MODULES(CEGUI06, CEGUI-0.6 >= 0.6.0 CEGUI-0.6 < 0.7.0,
 [
 AC_SUBST(CEGUI06_CFLAGS)
@@ -254,7 +254,7 @@ SPICE_REQUIRES+=" openssl"
 # AC_SUBST(GL_LIBS)
 # SPICE_REQUIRES+=" gl glu"
 
-if test "x$enable_opengl" = "xyes"; then
+if test "x$enable_opengl" = "xyes" && test "x$enable_client" = "xyes" ; then
AC_CHECK_LIB(GL, glBlendFunc, GL_LIBS="$GL_LIBS -lGL", enable_opengl=no)
AC_CHECK_LIB(GLU, gluSphere, GL_LIBS="$GL_LIBS -lGLU", enable_opengl=no)
AC_DEFINE([USE_OPENGL], [1], [Define to build with OpenGL support])
@@ -269,11 +269,14 @@ AC_SUBST(GL_CFLAGS)
 AC_SUBST(GL_LIBS)
 SPICE_NONPKGCONFIG_LIBS+=" $GL_LIBS"
 
-if test "$red_target" = "x11"; then
+if test "x$red_target" = "xx11" && test "x$enable_client" = "xyes" ; then
PKG_CHECK_MODULES(XRANDR, xrandr)
PKG_CHECK_MODULES(XFIXES, xfixes)
+   PKG_CHECK_MODULES(MISC_X, x11 xext xrender)
AC_SUBST(XRANDR_CFLAGS)
AC_SUBST(XRANDR_LIBS)
+   AC_SUBST(MISC_X_CFLAGS)
+   AC_SUBST(MISC_X_LIBS)
 
PKG_CHECK_MODULES(XRANDR12,
xrandr >= 1.2,
@@ -288,12 +291,6 @@ if test "x$have_xrandr12" = "xyes" ; then
   AC_DEFINE([HAVE_XRANDR12], [], [Define if we have XRANDR 12])
 fi
 
-if test "$red_target" = "x11"; then
-   PKG_CHECK_MODULES(MISC_X, x11 xext xrender)
-   AC_SUBST(MISC_X_CFLAGS)
-   AC_SUBST(MISC_X_LIBS)
-fi
-
 PKG_CHECK_MODULES(XINERAMA,
 xinerama >= 1.0,
 have_xinerama=yes,
@@ -485,18 +482,18 @@ echo "
 python:   ${PYTHON}
 
 Build Spice client:   ${enable_client}
-
+" ; if test "x$enable_client" == "xyes"; then echo "
 Have XRANDR 1.2:  ${have_xrandr12}
 
 Have Xinerama:${have_xinerama}
 
-Support tunneling:${enable_tunnel}
-
 Red target:   ${red_target}
 
 OpenGL:   ${enable_opengl}
 
 GUI:  ${enable_gui}
+" ; fi ; echo "
+Support tunneling:${enable_tunnel}
 
 Smartcard:${enable_smartcard}
 
-- 
1.7.7.6

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

[Spice-devel] [PATCH] cleanup x11 library detection for building client

2012-05-03 Thread Nahum Shalman 
consolidate two separate chunks of library hunting that depend on the
same check, and check if we're actually building the client.

hide some of the final output if we're not building the client
---
 configure.ac |   15 ++-
 1 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/configure.ac b/configure.ac
index 4b24c7d..aaa388c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -269,11 +269,14 @@ AC_SUBST(GL_CFLAGS)
 AC_SUBST(GL_LIBS)
 SPICE_NONPKGCONFIG_LIBS+=" $GL_LIBS"
 
-if test "$red_target" = "x11"; then
+if test "$red_target" = "x11" && test "$enable_client" = "yes" ; then
PKG_CHECK_MODULES(XRANDR, xrandr)
PKG_CHECK_MODULES(XFIXES, xfixes)
+   PKG_CHECK_MODULES(MISC_X, x11 xext xrender)
AC_SUBST(XRANDR_CFLAGS)
AC_SUBST(XRANDR_LIBS)
+   AC_SUBST(MISC_X_CFLAGS)
+   AC_SUBST(MISC_X_LIBS)
 
PKG_CHECK_MODULES(XRANDR12,
xrandr >= 1.2,
@@ -288,12 +291,6 @@ if test "x$have_xrandr12" = "xyes" ; then
   AC_DEFINE([HAVE_XRANDR12], [], [Define if we have XRANDR 12])
 fi
 
-if test "$red_target" = "x11"; then
-   PKG_CHECK_MODULES(MISC_X, x11 xext xrender)
-   AC_SUBST(MISC_X_CFLAGS)
-   AC_SUBST(MISC_X_LIBS)
-fi
-
 PKG_CHECK_MODULES(XINERAMA,
 xinerama >= 1.0,
 have_xinerama=yes,
@@ -485,11 +482,11 @@ echo "
 python:   ${PYTHON}
 
 Build Spice client:   ${enable_client}
-
+" ; if test $enable_client == "yes"; then echo "
 Have XRANDR 1.2:  ${have_xrandr12}
 
 Have Xinerama:${have_xinerama}
-
+" ; fi ; echo "
 Support tunneling:${enable_tunnel}
 
 Red target:   ${red_target}
-- 
1.7.7.6

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

Re: [Spice-devel] [bug] tripping an assert (regularly!) in release_drawable in red_worker.c

2012-05-01 Thread Nahum Shalman 

On 05/01/2012 01:17 PM, Yonit Halperin wrote:
I'm doing some changes in this part of the code, but so far I haven't 
tripped into this assert (before applying my changes, and without 
pulling changes from the last week or two). Can you bisect this?

What guest are you running?

I probably can't bisect this, but I can provide a bunch more detail for you.

Guest OS: Fedora 16

Things we've noticed:
Using the stock xorg-x11-drv-qxl from the repositories, it is not 
possible to disable the caching or surfaces features of the xorg 
driver.  Performance is not optimal.


If you recompile the xorg qxl driver from source from about 2 weeks ago, 
you can cause the spice server to start tripping over asserts if you put 
a file like this into xorg.conf.d and start watching videos in the VM:


Section "Device"
Identifier  "Videocard0"
Driver  "qxl"
Option  "DPI"   "96 x 96"
Option "ENABLE_IMAGE_CACHE" "False"
Option "ENABLE_FALLBACK_CACHE" "False"
Option "ENABLE_SURFACES" "False"
EndSection


Without that file, if you run Firefox, you get tons of "Out of surfaces" 
messages in the Xorg log file, and firefox's performance goes down to 
unusable.  Google chrome running at the same time doesn't cause the spew 
of messages, and performs fine.


With that same driver, if you remove the 2 lines with CACHE in them, but 
leave in the line to disable surfaces, Firefox stops causing
the spew of "out of surfaces" messages (because there are none) and 
performs fine.


If you recompile the xorg qxl driver from source as of today, and don't 
disable any of the features, running Firefox no longer causes a spew of 
"out of surfaces" messages, but the performance is still somewhat shoddy.


If you disable the surfaces only (leave caches enabled), firefox 
performance goes back to performing well.


So, to sum up:

If you want to cause assertion failures in the spice server, manually 
compile the xorg qxl driver from 2 weeks ago and install it in a fedora 
16 VM, and disable the cache features and the surface feature, and then 
try to stream video.  It seems like a badly behaving qxl driver can 
crash the spice server which is a flaw.  It should detect a badly 
behaving client more robustly and try to recover.


There are still performance problems with the surfaces feature, and GTK 
applications seem to hit them more so than non-gtk applications.


On the bright side it seems like with mainline spice server and mainline 
xorg qxl driver, if you disable the surfaces, performance on Fedora 16 
goes back to being the awesomeness that Spice is supposed to deliver. :)


Thanks!
-Nahum
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [bug] tripping an assert (regularly!) in release_drawable in red_worker.c

2012-05-01 Thread Nahum Shalman 
I'm much happier about this one since it now happens to me pretty much 
every time I use a VM.
Once again, I'm using the latest bits as of a couple days ago with a 
modified qemu running under illumos.


I'm reliably tripping an assert in red_worker.c
(it's annoying when that happens... qemu just sort of spins off into 
crazy land rather than dumping core and exiting.)


This is the tripped assert:
SpiceWorker-ERROR **: red_worker.c:1736:release_drawable: assertion 
`!drawable->stream' failed


I hacked up all the calls to release_drawable to include a matching 
assert above the call to isolate which call was tripping it, and it  
looks like it's the last one in red_process_drawable


I have a couple of thoughts:

1. Can someone familiar with the (scary) innards of red_worker.c take a 
look and see if anything stands out?
2. Should we consider beefing up the implementation of red_assert to 
have it generate a stack trace and/or have it cause qemu to dump core?  
This seems reasonable to me since QEMU seems to go crazy when this happens.


Thanks for your time,
Nahum
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] server bug: Spice-ERROR **: snd_worker.c:434:snd_receive: assertion `n' failed

2012-04-05 Thread Nahum Shalman 

The price of living on the edge is I find the fun bugs... :)

I have VMs running that have been crashing when someone is watching 
video (I'm pretty sure there's always sound at the same time).
I'm running on code from spice.git at 
8cd92109d42db45fd5b3aa9674b2148550ffe17b


The last thing I get on in the log is:
((null):17981): Spice-ERROR **: snd_worker.c:434:snd_receive: assertion 
`n' failed


Immediate context:
   431  for (;;) {
   432  ssize_t n;
   433  n = channel->recive_data.end - channel->recive_data.now;
   434  spice_assert(n);
   435  n = reds_stream_read(channel->stream, 
channel->recive_data.now, n);

   436  if (n <= 0) {
   437  if (n == 0) {
   438  snd_disconnect_channel(channel);
   439  return;
   440  }


I haven't looked into this much further; it doesn't immediately trip 
every time you stream sound, so I'm not sure of the exact trigger.  From 
talking to elmarco on IRC, it seems that the error in the code might be 
immediately obvious to someone who knows what to look for:


9:41 < nahamu> http://fpaste.org/iMPB/
09:41 < nahamu> I tripped over an assert in snd_worker ^^^
09:41 < nahamu> trying to figure out what I should do to isolate it.
09:48 < elmarco> nahamu: you could report a bug, the code looks suspicious
09:49 < elmarco> this line in particular  channel->recive_data.now = 
channel->recive_data.buf + n;
09:51 < nahamu> elmarco: do I need anything else in such a bug report, 
or is it enough to let people know that the

VM tripped over the assert?
09:51 < elmarco> if think the code is sufficiently suspicious :)
09:52 < nahamu> elmarco: thanks.  I'll fire off something to the mailing 
list.


Let me know if there's anything I can do to help isolate this further.

-Nahum

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

[Spice-devel] BUG? NULL pointer check in quic_encode, but quic_encode is always called with a NULL

2012-03-27 Thread Nahum Shalman 

Hey everyone, I think I found a bug:

I've been living on the bleeding edge of the spice repos and a vm I was 
running was tripping over

quic.c line 1242 (lots of errors spewing on the console).

It looks like a sane check for NULL pointers in the quic_encode function:
1241if (line == NULL) {
1242spice_warn_if_reached();
1243return QUIC_ERROR;
1244}

The only problem is that the *only* call to quic_encode in the spice 
server...


~/spice $ git grep -n quic_encode
server/red_worker.c:6119:size = quic_encode(quic, type, src->x, 
src->y, NULL, 0, stride,


*calls it with a NULL* !?

git blame points at c1403ee6bf4dfdd8f614f84ef145083b06a9f23e so I'm 
CC'ing the author.


I think that either the NULL pointer check is wrong, or that red_worker 
shouldn't be passing a NULL to quic_encode...

but I'm not sure which is the case.

Thanks!
-Nahum
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 1/2] server: don't fail on ENOPROTOOPT from setsockopt

2012-03-12 Thread Nahum Shalman 
Before I get too far into changing lots of code I wanted to ask the 
following questions:


spice/server]$ git grep "setsockopt(" | wc -l
10

There are 10 calls to setsockopt in the server code base.

All of them allow ENOTSUP, and won't even complain if they get that 
error code.
Only the two that I modified actually cause the calling function to fail 
on the other error codes.

The rest just red_printf a warning and continue.

Why do those two need to fail?
Why don't the rest?
Do those two need to fail?

-Nahum

On 03/10/2012 06:00 AM, Alon Levy wrote:

On Fri, Mar 09, 2012 at 12:26:50PM -0500, Nahum Shalman wrote:

If we allow listening on arbitrary sockets like unix sockets,
we can get ENOPROTOOPT errors from setsockopt calls that set TCP
specific options.  This should be allowed to happen.

I'm a little concerned that we will ignore actual errors for TCP sockets
this way. I think this patch is ok, but I'd like to see an additional
patch that added a boolean or enum for the socket type and thus could
avoid doing the IPPROTO_TCP in the first place. Or maybe you can get
that information from a filedescriptor without caching it yourself.


---
  server/inputs_channel.c |2 +-
  server/spicevmc.c   |2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/server/inputs_channel.c b/server/inputs_channel.c
index a3f26c0..fb25fe0 100644
--- a/server/inputs_channel.c
+++ b/server/inputs_channel.c
@@ -470,7 +470,7 @@ static int inputs_channel_config_socket(RedChannelClient 
*rcc)

  if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY,
  &delay_val, sizeof(delay_val)) == -1) {
-if (errno != ENOTSUP) {
+if (errno != ENOTSUP&&  errno != ENOPROTOOPT) {
  red_printf("setsockopt failed, %s", strerror(errno));
  return FALSE;
  }
diff --git a/server/spicevmc.c b/server/spicevmc.c
index 30aaf2f..9449c1e 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -92,7 +92,7 @@ static int 
spicevmc_red_channel_client_config_socket(RedChannelClient *rcc)
  if (rcc->channel->type == SPICE_CHANNEL_USBREDIR) {
  if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY,
  &delay_val, sizeof(delay_val)) != 0) {
-if (errno != ENOTSUP) {
+if (errno != ENOTSUP&&  errno != ENOPROTOOPT) {
  red_printf("setsockopt failed, %s", strerror(errno));
  return FALSE;
  }
--
1.7.7.6

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


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

[Spice-devel] [PATCH 1/2] server: don't fail on ENOPROTOOPT from setsockopt

2012-03-09 Thread Nahum Shalman 
If we allow listening on arbitrary sockets like unix sockets,
we can get ENOPROTOOPT errors from setsockopt calls that set TCP
specific options.  This should be allowed to happen.
---
 server/inputs_channel.c |2 +-
 server/spicevmc.c   |2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/server/inputs_channel.c b/server/inputs_channel.c
index a3f26c0..fb25fe0 100644
--- a/server/inputs_channel.c
+++ b/server/inputs_channel.c
@@ -470,7 +470,7 @@ static int inputs_channel_config_socket(RedChannelClient 
*rcc)
 
 if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY,
 &delay_val, sizeof(delay_val)) == -1) {
-if (errno != ENOTSUP) {
+if (errno != ENOTSUP && errno != ENOPROTOOPT) {
 red_printf("setsockopt failed, %s", strerror(errno));
 return FALSE;
 }
diff --git a/server/spicevmc.c b/server/spicevmc.c
index 30aaf2f..9449c1e 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -92,7 +92,7 @@ static int 
spicevmc_red_channel_client_config_socket(RedChannelClient *rcc)
 if (rcc->channel->type == SPICE_CHANNEL_USBREDIR) {
 if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY,
 &delay_val, sizeof(delay_val)) != 0) {
-if (errno != ENOTSUP) {
+if (errno != ENOTSUP && errno != ENOPROTOOPT) {
 red_printf("setsockopt failed, %s", strerror(errno));
 return FALSE;
 }
-- 
1.7.7.6

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

[Spice-devel] [PATCH 2/2] server: listen on a pre-opened file descriptor

2012-03-09 Thread Nahum Shalman 
Allow applications to pre-open a file descriptor and have spice listen
on it.

Thanks to Daniel Berrange for his comments
---
 server/reds.c|   21 +
 server/spice-server.syms |1 +
 server/spice.h   |1 +
 3 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/server/reds.c b/server/reds.c
index 5fc03ea..dc009f4 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -94,6 +94,7 @@ static SpiceMigrateInstance *migration_interface = NULL;
 
 static int spice_port = -1;
 static int spice_secure_port = -1;
+static int spice_listen_socket_fd = -1;
 static char spice_addr[256];
 static int spice_family = PF_UNSPEC;
 static char *default_renderer = "sw";
@@ -2995,6 +2996,19 @@ static int reds_init_net(void)
 red_error("set fd handle failed");
 }
 }
+
+if (spice_listen_socket_fd != -1 ) {
+reds->listen_socket = spice_listen_socket_fd;
+if (-1 == reds->listen_socket) {
+return -1;
+}
+reds->listen_watch = core->watch_add(reds->listen_socket,
+ SPICE_WATCH_EVENT_READ,
+ reds_accept, NULL);
+if (reds->listen_watch == NULL) {
+red_error("set fd handle failed");
+}
+}
 return 0;
 }
 
@@ -3787,6 +3801,13 @@ SPICE_GNUC_VISIBLE void 
spice_server_set_addr(SpiceServer *s, const char *addr,
 }
 }
 
+SPICE_GNUC_VISIBLE int spice_server_set_listen_socket_fd(SpiceServer *s, int 
listen_fd)
+{
+ASSERT(reds == s);
+spice_listen_socket_fd = listen_fd;
+return 0;
+}
+
 SPICE_GNUC_VISIBLE int spice_server_set_noauth(SpiceServer *s)
 {
 ASSERT(reds == s);
diff --git a/server/spice-server.syms b/server/spice-server.syms
index 272548e..4b842a3 100644
--- a/server/spice-server.syms
+++ b/server/spice-server.syms
@@ -106,4 +106,5 @@ SPICE_SERVER_0.10.2 {
 global:
 spice_server_set_name;
 spice_server_set_uuid;
+spice_server_set_listen_socket_fd;
 } SPICE_SERVER_0.10.1;
diff --git a/server/spice.h b/server/spice.h
index 151b3db..8dd1c3d 100644
--- a/server/spice.h
+++ b/server/spice.h
@@ -421,6 +421,7 @@ int spice_server_set_compat_version(SpiceServer *s,
 spice_compat_version_t version);
 int spice_server_set_port(SpiceServer *s, int port);
 void spice_server_set_addr(SpiceServer *s, const char *addr, int flags);
+int spice_server_set_listen_socket_fd(SpiceServer *s, int listen_fd);
 int spice_server_set_noauth(SpiceServer *s);
 int spice_server_set_sasl(SpiceServer *s, int enabled);
 int spice_server_set_sasl_appname(SpiceServer *s, const char *appname);
-- 
1.7.7.6

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

[Spice-devel] [PATCH 0/2] server: listen on a pre-opened file descriptor

2012-03-09 Thread Nahum Shalman 
With thanks to Daniel Berrange for his guidance, this is a redo of my
previous pair of patches.

Listening on a unix socket was hitting ENOPROTOOPT, so I'm now allowing
that error.

I've also generalized the naming per Daniel's suggestion since this could
be flexibly used in lots of different scenarios.

Nahum Shalman (2):
  server: don't fail on ENOPROTOOPT from setsockopt
  server: listen on a pre-opened file descriptor

 server/inputs_channel.c  |2 +-
 server/reds.c|   21 +
 server/spice-server.syms |1 +
 server/spice.h   |1 +
 server/spicevmc.c|2 +-
 5 files changed, 25 insertions(+), 2 deletions(-)

-- 
1.7.7.6

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

Re: [Spice-devel] [PATCH 1/2] server: don't fail on ENOSUP from setsockopt calls

2012-03-09 Thread Nahum Shalman 

On 03/09/2012 11:15 AM, Daniel P. Berrange wrote:
Huh. This code already avoids failure when you get ENOTSUP. You're 
removing the code path where the errno is *not* ENOTSUP 

You're right.  I need to go back and figure out which error we were getting.

Thank you.

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

[Spice-devel] [PATCH 1/2] server: don't fail on ENOSUP from setsockopt calls

2012-03-09 Thread Nahum Shalman 
These are the only instances in the code base where functions fail as a
result of setsockopt calls returning ENOSUP.
Those calls will always return ENOSUP when we are listening on a UNIX
socket rather than a TCP port.
We'd prefer not to fail.
---
 server/inputs_channel.c |1 -
 server/spicevmc.c   |1 -
 2 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/server/inputs_channel.c b/server/inputs_channel.c
index a3f26c0..3cae408 100644
--- a/server/inputs_channel.c
+++ b/server/inputs_channel.c
@@ -472,7 +472,6 @@ static int inputs_channel_config_socket(RedChannelClient 
*rcc)
 &delay_val, sizeof(delay_val)) == -1) {
 if (errno != ENOTSUP) {
 red_printf("setsockopt failed, %s", strerror(errno));
-return FALSE;
 }
 }
 
diff --git a/server/spicevmc.c b/server/spicevmc.c
index 30aaf2f..2967bcd 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -94,7 +94,6 @@ static int 
spicevmc_red_channel_client_config_socket(RedChannelClient *rcc)
 &delay_val, sizeof(delay_val)) != 0) {
 if (errno != ENOTSUP) {
 red_printf("setsockopt failed, %s", strerror(errno));
-return FALSE;
 }
 }
 }
-- 
1.7.7.6

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

[Spice-devel] [PATCH 0/2] server: support listening on a UNIX socket

2012-03-09 Thread Nahum Shalman 
These patches are to allow listening on a UNIX socket rather
than a TCP port.  Support is needed in QEMU.

Example QEMU patches (against a different QEMU tree) can be found at
https://github.com/nshalman/illumos-kvm-cmd

Feedback requested!

Nahum Shalman (2):
  server: don't fail on ENOSUP from setsockopt calls
  server: support listening on a UNIX socket

 server/inputs_channel.c  |1 -
 server/reds.c|   21 +
 server/spice-server.syms |1 +
 server/spice.h   |1 +
 server/spicevmc.c|1 -
 5 files changed, 23 insertions(+), 2 deletions(-)

-- 
1.7.7.6

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

[Spice-devel] [PATCH 2/2] server: support listening on a UNIX socket

2012-03-09 Thread Nahum Shalman 
We leave all the hard work of setting up the socket to qemu.
This will cause all of the TCP setsockopt calls to return ENOSUP
but that is okay.
---
 server/reds.c|   21 +
 server/spice-server.syms |1 +
 server/spice.h   |1 +
 3 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/server/reds.c b/server/reds.c
index 5fc03ea..7f9ffcc 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -94,6 +94,7 @@ static SpiceMigrateInstance *migration_interface = NULL;
 
 static int spice_port = -1;
 static int spice_secure_port = -1;
+static int spice_unix_socket = -1;
 static char spice_addr[256];
 static int spice_family = PF_UNSPEC;
 static char *default_renderer = "sw";
@@ -2995,6 +2996,19 @@ static int reds_init_net(void)
 red_error("set fd handle failed");
 }
 }
+
+if (spice_unix_socket != -1 ) {
+reds->listen_socket = spice_unix_socket;
+if (-1 == reds->listen_socket) {
+return -1;
+}
+reds->listen_watch = core->watch_add(reds->listen_socket,
+ SPICE_WATCH_EVENT_READ,
+ reds_accept, NULL);
+if (reds->listen_watch == NULL) {
+red_error("set fd handle failed");
+}
+}
 return 0;
 }
 
@@ -3787,6 +3801,13 @@ SPICE_GNUC_VISIBLE void 
spice_server_set_addr(SpiceServer *s, const char *addr,
 }
 }
 
+SPICE_GNUC_VISIBLE int spice_server_set_unix_socket(SpiceServer *s, int 
unix_socket)
+{
+ASSERT(reds == s);
+spice_unix_socket = unix_socket;
+return 0;
+}
+
 SPICE_GNUC_VISIBLE int spice_server_set_noauth(SpiceServer *s)
 {
 ASSERT(reds == s);
diff --git a/server/spice-server.syms b/server/spice-server.syms
index 272548e..51c6ffa 100644
--- a/server/spice-server.syms
+++ b/server/spice-server.syms
@@ -106,4 +106,5 @@ SPICE_SERVER_0.10.2 {
 global:
 spice_server_set_name;
 spice_server_set_uuid;
+spice_server_set_unix_socket;
 } SPICE_SERVER_0.10.1;
diff --git a/server/spice.h b/server/spice.h
index 151b3db..7ace03b 100644
--- a/server/spice.h
+++ b/server/spice.h
@@ -421,6 +421,7 @@ int spice_server_set_compat_version(SpiceServer *s,
 spice_compat_version_t version);
 int spice_server_set_port(SpiceServer *s, int port);
 void spice_server_set_addr(SpiceServer *s, const char *addr, int flags);
+int spice_server_set_unix_socket(SpiceServer *s, int unix_socket);
 int spice_server_set_noauth(SpiceServer *s);
 int spice_server_set_sasl(SpiceServer *s, int enabled);
 int spice_server_set_sasl_appname(SpiceServer *s, const char *appname);
-- 
1.7.7.6

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

[Spice-devel] [PATCH spice-protocol] Fix SPICE_STAT_SHM_NAME to be portable

2012-01-06 Thread Nahum Shalman 

From SHM_OPEN(3):
  For portable use, a shared memory object should be identified by a name
  of the  form  /somename; that is, a null-terminated string of up to
  NAME_MAX (i.e., 255) characters consisting of an initial slash,
  followed by one or more characters, none of which are slashes.
---
 spice/stats.h |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/spice/stats.h b/spice/stats.h
index eea5478..452d89e 100644
--- a/spice/stats.h
+++ b/spice/stats.h
@@ -33,7 +33,7 @@

 #include 

-#define SPICE_STAT_SHM_NAME "spice.%u"
+#define SPICE_STAT_SHM_NAME "/spice.%u"
 #define SPICE_STAT_NODE_NAME_MAX 20
 #define SPICE_STAT_MAGIC (*(uint32_t*)"STAT")
 #define SPICE_STAT_VERSION 1
--
1.7.7.4

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