[Spice-devel] [PATCH v2] Switch over to using keycodemapdb submodule

[Daniel P. Berrange]

Signed-off-by: Daniel P. Berrange 
---

Changed in v2:

 - Newer submodule hash to pull in python 3 portability fixes

 .gitmodules   |   3 +
 configure.ac  |  11 --
 src/Makefile.am   |  30 ++--
 src/keycodemapdb  |   1 +
 src/keymap-gen.pl | 214 ---
 src/keymaps.csv   | 511 --
 6 files changed, 17 insertions(+), 753 deletions(-)
 create mode 16 src/keycodemapdb
 delete mode 100755 src/keymap-gen.pl
 delete mode 100644 src/keymaps.csv

diff --git a/.gitmodules b/.gitmodules
index 0c618ee..82467e4 100644
--- a/.gitmodules
+++ b/.gitmodules
@@ -1,3 +1,6 @@
 [submodule "spice-common"]
path = spice-common
url = ../spice-common
+[submodule "src/keycodemapdb"]
+   path = src/keycodemapdb
+   url = https://gitlab.com/keycodemap/keycodemapdb.git
diff --git a/configure.ac b/configure.ac
index 463fbe0..763d14b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -86,17 +86,6 @@ AC_SUBST(SPICE_GTK_MICRO_VERSION)
 dnl =
 dnl Chek optional features
 
-srcdir="$(dirname $0)"
-if test ! -e "$srcdir/src/vncdisplaykeymap_osx2xtkbd.c"; then
-  AC_MSG_CHECKING([for Text::CSV Perl module])
-  perl -MText::CSV -e "" >/dev/null 2>&1
-  if test $? -ne 0 ; then
-AC_MSG_RESULT([not found])
-AC_MSG_ERROR([Text::CSV Perl module is required to compile this package])
-  fi
-  AC_MSG_RESULT([found])
-fi
-
 SPICE_GLIB_REQUIRES=""
 SPICE_GTK_REQUIRES=""
 
diff --git a/src/Makefile.am b/src/Makefile.am
index 7542fac..76c2755 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -26,14 +26,13 @@ GLIBGENS =  \
spice-widget-enums.h\
$(NULL)
 
-CLEANFILES = $(GLIBGENS)
+CLEANFILES = $(GLIBGENS) $(KEYMAPS)
 BUILT_SOURCES = $(GLIBGENS) $(KEYMAPS)
 
 EXTRA_DIST =   \
-   $(KEYMAPS)  \
decode-glz-tmpl.c   \
-   keymap-gen.pl   \
-   keymaps.csv \
+   $(KEYMAP_CSV)   \
+   $(KEYMAP_GEN)   \
map-file\
spice-glib-sym-file \
spice-gtk-sym-file  \
@@ -66,7 +65,8 @@ GTK_SYMBOLS_LDFLAGS = -export-symbols 
${srcdir}/spice-gtk-sym-file
 GTK_SYMBOLS_FILE = spice-gtk-sym-file
 endif
 
-KEYMAP_GEN = $(srcdir)/keymap-gen.pl
+KEYMAP_GEN = keycodemapdb/tools/keymap-gen
+KEYMAP_CSV = keycodemapdb/data/keymaps.csv
 
 SPICE_COMMON_CPPFLAGS =\
-DSPICE_COMPILATION \
@@ -483,32 +483,28 @@ spice-widget-enums.h: spice-widget.h
 
 
 vncdisplaykeymap.c: $(KEYMAPS)
+$(KEYMAPS): $(srcdir)/$(KEYMAP_GEN) $(srcdir)/$(KEYMAP_CSV)
 
-$(KEYMAPS): $(KEYMAP_GEN) keymaps.csv
-
-# Note despite being autogenerated these are not part of CLEANFILES, they
-# are actually a part of EXTRA_DIST to avoid the need for perl(Text::CSV) by
-# end users
 vncdisplaykeymap_xorgevdev2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgevdev xtkbd > $@ || 
rm $@
+   $(AM_V_GEN)$(PYTHON) $(KEYMAP_GEN) --lang glib2 --varname 
keymap_xorgevdev2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) xorgevdev xtkbd > $@ || 
rm $@
 
 vncdisplaykeymap_xorgkbd2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgkbd xtkbd > $@ || rm 
$@
+   $(AM_V_GEN)$(PYTHON) $(KEYMAP_GEN) --lang glib2 --varname 
keymap_xorgkbd2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) xorgkbd xtkbd > $@ || rm 
$@
 
 vncdisplaykeymap_xorgxquartz2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgxquartz xtkbd > $@ 
|| rm $@
+   $(AM_V_GEN)$(PYTHON) $(KEYMAP_GEN) --lang glib2 --varname 
keymap_xorgxquartz2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) xorgxquartz xtkbd > 
$@ || rm $@
 
 vncdisplaykeymap_xorgxwin2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgxwin xtkbd > $@ || 
rm $@
+   $(AM_V_GEN)$(PYTHON) $(KEYMAP_GEN) --lang glib2 --varname 
keymap_xorgxwin2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) xorgxwin xtkbd > $@ || 
rm $@
 
 vncdisplaykeymap_osx2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv osx xtkbd > $@ || rm $@
+   $(AM_V_GEN)$(PYTHON) $(KEYMAP_GEN) --lang glib2 --varname 
keymap_osx2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) osx xtkbd > $@ || rm $@
 
 vncdisplaykeymap_win322xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv win32 xtkbd > $@ || rm $@
+   $(AM_V_GEN)$(PYTHON) $(KEYMAP_GEN) --lang glib2 --varname 
keymap_win322xtkbd code-map $(srcdir)/$(KEYMAP_CSV) win32 xtkbd > $@ || rm $@
 
 vncdisplaykeymap_x112xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv x11 xtkbd > $@ || rm $@
+   $(AM_V_GEN)$(PYTHON) $(KEYMAP_GEN) --lang glib2 --varname 
keymap_x112xtkbd 

Re: [Spice-devel] bool or gboolean

[Jonathon Jongsma]

On Wed, 2017-02-15 at 18:01 +0100, Christophe Fergeau wrote:
> On Wed, Feb 15, 2017 at 11:39:32AM -0500, Frediano Ziglio wrote:
> > Hi,
> >   question was raised recently on the ML and IRC.
> > 
> > Some time ago we decided to use gboolean but some of us would like
> > to discuss again.
> 
> So... As said a few times, I would have preferred to wait for a bit
> to
> have that discussion as I have a concrete patch which it would go
> well
> with...
> 
> This started with Jonathon's comment in
> https://lists.freedesktop.org/archives/spice-devel/2017-February/0355
> 79.html
> where I returned -1 from a function which should be returning a
> boolean.
> The function was declared as 'int foo()', so it was not clear from
> its
> prototype that it's returning a bool, and I had totally missed that.
> So I decided to go ahead and grep'ed for 'return (TRUE|FALSE)' over
> the
> code base, and make sure all these functions don't return int.
> 
> 
> I started with 'gboolean', but then realized that using 'bool', the
> compiler would be able to help me.
> 
> static int foo(void);
> 
> static gboolean foo(void)
> { }
> 
> does not raise warnings while
> 
> static int foo(void);
> 
> static bool foo(void)
> { }
> 
> does raise a warning, and it's the same with function pointers (see
> attached bool.c file for a test case)
> 
> 
> So after realizing that, I'd favour bool over gboolean, at least in
> function prototypes, as it gives us slightly stronger typing.

That's a pretty strong argument for bool, in my opinion. In general,
I'm in favor of increased type-safety. Daniel's point about glib
requiring gboolean for callbacks etc is valid, but I'm curious about
how many cases there would be. It looks like the patch that you
attached only attempts to change int types to bool, but doesn't touch
any existing gboolean types, right? It would be interesting to try to
convert gboolean to bool to see how many of them can be easily changed.


> 
> Regarding TRUE vs true, I don't really care, I'd stick with TRUE as
> that
> means less churn on the codebase, but I agree it's not really nice.

After you told me that trick with tig blame, I'm not quite as concerned
about code churn as I used to be ;) A third option would be to
standardize on 'true/false' for new code and slowly change existing
TRUE/FALSE as we modify code...

Jonathon
___
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] sound: Use memcpy instead of manually copy volume array

[Frediano Ziglio]

> 
> On Wed, 2017-02-15 at 12:07 -0500, Frediano Ziglio wrote:
> > > 
> > > On Thu, Feb 02, 2017 at 04:29:37PM +, Frediano Ziglio wrote:
> > > > Signed-off-by: Frediano Ziglio 
> > > > ---
> > > >  server/sound.c | 6 ++
> > > >  1 file changed, 2 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/server/sound.c b/server/sound.c
> > > > index 7c36174..b692646 100644
> > > > --- a/server/sound.c
> > > > +++ b/server/sound.c
> > > > @@ -384,7 +384,6 @@ static int
> > > > snd_playback_send_migrate(PlaybackChannelClient *client)
> > > >  static int snd_send_volume(SndChannelClient *client, uint32_t
> > > > cap, int
> > > >  msg)
> > > >  {
> > > >  SpiceMsgAudioVolume *vol;
> > > > -uint8_t c;
> > > >  RedChannelClient *rcc = RED_CHANNEL_CLIENT(client);
> > > >  SpiceMarshaller *m =
> > > > red_channel_client_get_marshaller(rcc);
> > > >  SndChannel *channel =
> > > >  SND_CHANNEL(red_channel_client_get_channel(rcc));
> > > > @@ -398,9 +397,8 @@ static int snd_send_volume(SndChannelClient
> > > > *client,
> > > > uint32_t cap, int msg)
> > > >   st->volume_nchannels * sizeof (uint16_t));
> > > >  red_channel_client_init_send_data(rcc, msg);
> > > >  vol->nchannels = st->volume_nchannels;
> > > > -for (c = 0; c < st->volume_nchannels; ++c) {
> > > > -vol->volume[c] = st->volume[c];
> > > > -}
> > > > +SPICE_VERIFY(sizeof(vol->volume[0]) == sizeof(st-
> > > > >volume[0]));
> > > > +memcpy(vol->volume, st->volume, sizeof(st->volume[0]) *
> > > > st->volume_nchannels);
> > > 
> > > I think I prefer the loop version (easier to read).
> > > If you really insist on having the memcpy version,
> > > Acked-by: Christophe Fergeau 
> > > 
> > 
> > Let's size decide!
> > 
> > Before:
> >    text    data bss dec hex
> > filename
> >   16588   0  96   16684    412c
> > server/.libs/sound.o
> > 
> > After:
> >    text    data bss dec hex
> > filename
> >   16684   0  96   16780    418c
> > server/.libs/sound.o
> > 
> I guess it is due to the SPICE_VERIFY usage
> 

No, SPICE_VERIFY is a compile time check, I verified, it
take 0 byte.
I think that when gcc when see memcpy it thinks it must do the
more fast thing and inline the memcpy to optimize large
memory copies.

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] sound: Use memcpy instead of manually copy volume array

[Pavel Grunt]

On Wed, 2017-02-15 at 12:07 -0500, Frediano Ziglio wrote:
> > 
> > On Thu, Feb 02, 2017 at 04:29:37PM +, Frediano Ziglio wrote:
> > > Signed-off-by: Frediano Ziglio 
> > > ---
> > >  server/sound.c | 6 ++
> > >  1 file changed, 2 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/server/sound.c b/server/sound.c
> > > index 7c36174..b692646 100644
> > > --- a/server/sound.c
> > > +++ b/server/sound.c
> > > @@ -384,7 +384,6 @@ static int
> > > snd_playback_send_migrate(PlaybackChannelClient *client)
> > >  static int snd_send_volume(SndChannelClient *client, uint32_t
> > > cap, int
> > >  msg)
> > >  {
> > >  SpiceMsgAudioVolume *vol;
> > > -uint8_t c;
> > >  RedChannelClient *rcc = RED_CHANNEL_CLIENT(client);
> > >  SpiceMarshaller *m =
> > > red_channel_client_get_marshaller(rcc);
> > >  SndChannel *channel =
> > >  SND_CHANNEL(red_channel_client_get_channel(rcc));
> > > @@ -398,9 +397,8 @@ static int snd_send_volume(SndChannelClient
> > > *client,
> > > uint32_t cap, int msg)
> > >   st->volume_nchannels * sizeof (uint16_t));
> > >  red_channel_client_init_send_data(rcc, msg);
> > >  vol->nchannels = st->volume_nchannels;
> > > -for (c = 0; c < st->volume_nchannels; ++c) {
> > > -vol->volume[c] = st->volume[c];
> > > -}
> > > +SPICE_VERIFY(sizeof(vol->volume[0]) == sizeof(st-
> > > >volume[0]));
> > > +memcpy(vol->volume, st->volume, sizeof(st->volume[0]) *
> > > st->volume_nchannels);
> > 
> > I think I prefer the loop version (easier to read).
> > If you really insist on having the memcpy version,
> > Acked-by: Christophe Fergeau 
> > 
> 
> Let's size decide!
> 
> Before:
>    text      data bss dec hex 
> filename
>   16588     0  96   16684    412c 
> server/.libs/sound.o
> 
> After:
>    text      data bss dec hex 
> filename
>   16684     0  96   16780    418c 
> server/.libs/sound.o
> 
I guess it is due to the SPICE_VERIFY usage

> Damn! Dropped!
> 
> 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 spice-server 2/2] sound: Use memcpy instead of manually copy volume array

[Frediano Ziglio]

> 
> On Thu, Feb 02, 2017 at 04:29:37PM +, Frediano Ziglio wrote:
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  server/sound.c | 6 ++
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/server/sound.c b/server/sound.c
> > index 7c36174..b692646 100644
> > --- a/server/sound.c
> > +++ b/server/sound.c
> > @@ -384,7 +384,6 @@ static int
> > snd_playback_send_migrate(PlaybackChannelClient *client)
> >  static int snd_send_volume(SndChannelClient *client, uint32_t cap, int
> >  msg)
> >  {
> >  SpiceMsgAudioVolume *vol;
> > -uint8_t c;
> >  RedChannelClient *rcc = RED_CHANNEL_CLIENT(client);
> >  SpiceMarshaller *m = red_channel_client_get_marshaller(rcc);
> >  SndChannel *channel =
> >  SND_CHANNEL(red_channel_client_get_channel(rcc));
> > @@ -398,9 +397,8 @@ static int snd_send_volume(SndChannelClient *client,
> > uint32_t cap, int msg)
> >   st->volume_nchannels * sizeof (uint16_t));
> >  red_channel_client_init_send_data(rcc, msg);
> >  vol->nchannels = st->volume_nchannels;
> > -for (c = 0; c < st->volume_nchannels; ++c) {
> > -vol->volume[c] = st->volume[c];
> > -}
> > +SPICE_VERIFY(sizeof(vol->volume[0]) == sizeof(st->volume[0]));
> > +memcpy(vol->volume, st->volume, sizeof(st->volume[0]) *
> > st->volume_nchannels);
> 
> I think I prefer the loop version (easier to read).
> If you really insist on having the memcpy version,
> Acked-by: Christophe Fergeau 
> 

Let's size decide!

Before:
   textdata bss dec hex filename
  16588   0  96   16684412c server/.libs/sound.o

After:
   textdata bss dec hex filename
  16684   0  96   16780418c server/.libs/sound.o

Damn! Dropped!

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

Re: [Spice-devel] bool or gboolean

[Christophe Fergeau]

On Wed, Feb 15, 2017 at 11:39:32AM -0500, Frediano Ziglio wrote:
> Hi,
>   question was raised recently on the ML and IRC.
> 
> Some time ago we decided to use gboolean but some of us would like
> to discuss again.

So... As said a few times, I would have preferred to wait for a bit to
have that discussion as I have a concrete patch which it would go well
with...

This started with Jonathon's comment in
https://lists.freedesktop.org/archives/spice-devel/2017-February/035579.html
where I returned -1 from a function which should be returning a boolean.
The function was declared as 'int foo()', so it was not clear from its
prototype that it's returning a bool, and I had totally missed that.
So I decided to go ahead and grep'ed for 'return (TRUE|FALSE)' over the
code base, and make sure all these functions don't return int.


I started with 'gboolean', but then realized that using 'bool', the
compiler would be able to help me.

static int foo(void);

static gboolean foo(void)
{ }

does not raise warnings while

static int foo(void);

static bool foo(void)
{ }

does raise a warning, and it's the same with function pointers (see
attached bool.c file for a test case)


So after realizing that, I'd favour bool over gboolean, at least in
function prototypes, as it gives us slightly stronger typing.

Regarding TRUE vs true, I don't really care, I'd stick with TRUE as that
means less churn on the codebase, but I agree it's not really nice.

In addition to the testcase, I'm attaching that s/int/bool patch even
though this version is not going to apply on git master, and is not
tested.

Christophe
From db86accb52c56067fdf2707d53c7ecb4570db446 Mon Sep 17 00:00:00 2001
From: Christophe Fergeau 
Date: Mon, 13 Feb 2017 14:55:08 +0100
Subject: [spice-server] s/int/bool

---
 server/char-device.c  | 20 ++--
 server/char-device.h  | 18 +-
 server/common-graphics-channel.c  |  2 +-
 server/common-graphics-channel.h  |  2 +-
 server/dcc-send.c | 36 +--
 server/dcc.c  | 40 +++
 server/dcc.h  | 12 ++--
 server/display-channel.c  | 16 
 server/display-channel.h  |  2 +-
 server/glz-encoder-dict.c |  4 ++--
 server/glz-encoder.c  |  2 +-
 server/image-cache.c  |  2 +-
 server/image-encoders.c   | 30 ++---
 server/image-encoders.h   | 28 +--
 server/inputs-channel.c   | 12 ++--
 server/main-channel-client.c  |  4 ++--
 server/main-channel.c | 10 +-
 server/mjpeg-encoder.c|  4 ++--
 server/pixmap-cache.c |  2 +-
 server/pixmap-cache.h |  2 +-
 server/red-channel-client.c   | 20 ++--
 server/red-channel-client.h   | 14 +++---
 server/red-channel.c  | 18 +-
 server/red-channel.h  | 30 ++---
 server/red-parse-qxl.c|  2 +-
 server/red-worker.c   |  2 +-
 server/reds-stream.c  |  2 +-
 server/reds-stream.h  |  2 +-
 server/reds.c | 12 ++--
 server/reds.h |  4 ++--
 server/smartcard-channel-client.c | 16 
 server/smartcard-channel-client.h | 16 
 server/sound.c| 38 ++---
 server/spicevmc.c | 18 +-
 server/stream.c   | 12 ++--
 server/tree.c |  2 +-
 server/tree.h |  2 +-
 37 files changed, 229 insertions(+), 229 deletions(-)

diff --git a/server/char-device.c b/server/char-device.c
index c40ed65..d325932 100644
--- a/server/char-device.c
+++ b/server/char-device.c
@@ -306,7 +306,7 @@ static void 
red_char_device_send_msg_to_clients(RedCharDevice *dev,
 }
 }
 
-static int red_char_device_read_from_device(RedCharDevice *dev)
+static bool red_char_device_read_from_device(RedCharDevice *dev)
 {
 uint64_t max_send_tokens;
 int did_read = FALSE;
@@ -720,13 +720,13 @@ static RedCharDeviceClient 
*red_char_device_client_new(RedClient *client,
 return dev_client;
 }
 
-int red_char_device_client_add(RedCharDevice *dev,
-   RedClient *client,
-   int do_flow_control,
-   uint32_t max_send_queue_size,
-   uint32_t num_client_tokens,
-   uint32_t num_send_tokens,
-   int wait_for_migrate_data)
+bool red_char_device_client_add(RedCharDevice *dev,
+RedClient *client,
+int do_flow_control,
+uint32_t 

Re: [Spice-devel] bool or gboolean

[Daniel P. Berrange]

On Wed, Feb 15, 2017 at 11:39:32AM -0500, Frediano Ziglio wrote:
> Hi,
>   question was raised recently on the ML and IRC.
> 
> Some time ago we decided to use gboolean but some of us would like
> to discuss again.
> 
> As any style changes there's no right or wrong, mainly personal
> opinions but I think consistency is quite important.
> 
> Some consideration (feel free to add/remove/comment)
> - gboolean is more used in the code (about 76%)
> - TRUE/FALSE are more used (96%)
> - bool is C99 convention, defined in stdbool.h
> - using gcc the bool type is a bit different from gboolean
>   (which basically is an int) catching some problems as
>   warnings (like cast between different function pointers
>   using bool instead of gboolean/int)
> - bool is easier to write (OT: and my vim is more happy too)

As noted above gboolean & bool are different types, in fact they
are different sizes too (4 bytes vs 1 byte).

If you're using glib2, you'll find various places where it wants
callbacks which have a gboolean return value, or parameters. You
can't pass in a callback which uses bool, as that's not type
compatible. So no matter what, you'll always need to use gboolean
in some portion of the code, even if you would rather have bool.

Thus if consistency is the goal, then gboolean is the winner as
you're more likely to be able to kill off all uses of bool, than
be able to kill off all uses of gboolean.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server 1/3] Use class name in comment

[Frediano Ziglio]

Yes, changed all, sending a v2

> 
> On Wed, Feb 15, 2017 at 11:46:21AM -0500, Frediano Ziglio wrote:
> > > 
> > > ACK, maybe rcc: in the short log
> > > 
> > 
> > I'd use red-channel-client: as consistent with the second...
> > or change both to rcc!
> 
> I meant to suggest to change the second one to rcc but forgot. Only
> reason for red-channel-client vs rcc is that the latter is much shorter.
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server 2/3] red-channel-client: Remove unused vfuncs

[Christophe Fergeau]

On Wed, Feb 15, 2017 at 11:44:53AM -0500, Frediano Ziglio wrote:
> > 
> > On Wed, Feb 15, 2017 at 12:49:06PM +, Frediano Ziglio wrote:
> > > RedChannelClient is responsible for talking to the client so it knows
> > > how if is connected or not.
> > 
> > s/how//
> > 
> > I'd mention explicitly that it's RedChannelClient::is_connected and
> > RedChannelClient::disconnect which are dropped (would even belong in the
> > shortlog if it's not too long).
> > 
> 
> What about
> 
> "red-channel-client: Remove unused 
> RedChannelClient::{is_connected,disconnect}"

Works for me! (s/red-channel-client/rcc to keep the shortlog short)

Christophe


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

Re: [Spice-devel] [PATCH spice-server 1/3] Use class name in comment

[Christophe Fergeau]

On Wed, Feb 15, 2017 at 11:46:21AM -0500, Frediano Ziglio wrote:
> > 
> > ACK, maybe rcc: in the short log
> > 
> 
> I'd use red-channel-client: as consistent with the second...
> or change both to rcc!

I meant to suggest to change the second one to rcc but forgot. Only
reason for red-channel-client vs rcc is that the latter is much shorter.


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

Re: [Spice-devel] [PATCH spice-server 2/2] sound: Use memcpy instead of manually copy volume array

[Christophe Fergeau]

On Thu, Feb 02, 2017 at 04:29:37PM +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/sound.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/server/sound.c b/server/sound.c
> index 7c36174..b692646 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -384,7 +384,6 @@ static int 
> snd_playback_send_migrate(PlaybackChannelClient *client)
>  static int snd_send_volume(SndChannelClient *client, uint32_t cap, int msg)
>  {
>  SpiceMsgAudioVolume *vol;
> -uint8_t c;
>  RedChannelClient *rcc = RED_CHANNEL_CLIENT(client);
>  SpiceMarshaller *m = red_channel_client_get_marshaller(rcc);
>  SndChannel *channel = SND_CHANNEL(red_channel_client_get_channel(rcc));
> @@ -398,9 +397,8 @@ static int snd_send_volume(SndChannelClient *client, 
> uint32_t cap, int msg)
>   st->volume_nchannels * sizeof (uint16_t));
>  red_channel_client_init_send_data(rcc, msg);
>  vol->nchannels = st->volume_nchannels;
> -for (c = 0; c < st->volume_nchannels; ++c) {
> -vol->volume[c] = st->volume[c];
> -}
> +SPICE_VERIFY(sizeof(vol->volume[0]) == sizeof(st->volume[0]));
> +memcpy(vol->volume, st->volume, sizeof(st->volume[0]) * 
> st->volume_nchannels);

I think I prefer the loop version (easier to read).
If you really insist on having the memcpy version,
Acked-by: Christophe Fergeau 


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

Re: [Spice-devel] [PATCH spice-server 2/3] red-channel-client: Remove unused vfuncs

[Christophe Fergeau]

On Wed, Feb 15, 2017 at 12:49:06PM +, Frediano Ziglio wrote:
> RedChannelClient is responsible for talking to the client so it knows
> how if is connected or not.

s/how//

I'd mention explicitly that it's RedChannelClient::is_connected and
RedChannelClient::disconnect which are dropped (would even belong in the
shortlog if it's not too long).

> About disconnect you think that we loose a bit of flexibility however on
> disconnect the channel is triggered so additional needed process can be
> handled in destructor.

I'd drop that paragraph, as we don't have any classes overriding the
vfunc anyway, and client disconnection and the last ref being dropped
may occur at different times.


> These vfuncs where used by DummyChannel used by SoundChannel.


Acked-by: Christophe Fergeau 

> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/red-channel-client.c | 27 ++-
>  server/red-channel-client.h |  3 ---
>  2 files changed, 2 insertions(+), 28 deletions(-)
> 
> diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> index 32db186..16e5446 100644
> --- a/server/red-channel-client.c
> +++ b/server/red-channel-client.c
> @@ -364,10 +364,6 @@ static void 
> red_channel_client_initable_interface_init(GInitableIface *iface)
>  iface->init = red_channel_client_initable_init;
>  }
>  
> -static gboolean red_channel_client_default_is_connected(RedChannelClient 
> *rcc);
> -static void red_channel_client_default_disconnect(RedChannelClient *rcc);
> -
> -
>  static void red_channel_client_constructed(GObject *object)
>  {
>  RedChannelClient *self =  RED_CHANNEL_CLIENT(object);
> @@ -400,9 +396,6 @@ static void 
> red_channel_client_class_init(RedChannelClientClass *klass)
>  object_class->finalize = red_channel_client_finalize;
>  object_class->constructed = red_channel_client_constructed;
>  
> -klass->is_connected = red_channel_client_default_is_connected;
> -klass->disconnect = red_channel_client_default_disconnect;
> -
>  spec = g_param_spec_pointer("stream", "stream",
>  "Associated RedStream",
>  G_PARAM_STATIC_STRINGS
> @@ -1701,20 +1694,12 @@ gboolean 
> red_channel_client_is_mini_header(RedChannelClient *rcc)
>  return rcc->priv->is_mini_header;
>  }
>  
> -static gboolean red_channel_client_default_is_connected(RedChannelClient 
> *rcc)
> +gboolean red_channel_client_is_connected(RedChannelClient *rcc)
>  {
>  return rcc->priv->channel
>  && (g_list_find(red_channel_get_clients(rcc->priv->channel), rcc) != 
> NULL);
>  }
>  
> -gboolean red_channel_client_is_connected(RedChannelClient *rcc)
> -{
> -RedChannelClientClass *klass = RED_CHANNEL_CLIENT_GET_CLASS(rcc);
> -
> -g_return_val_if_fail(klass->is_connected != NULL, FALSE);
> -return klass->is_connected(rcc);
> -}
> -
>  static void red_channel_client_clear_sent_item(RedChannelClient *rcc)
>  {
>  rcc->priv->send_data.blocked = FALSE;
> @@ -1752,7 +1737,7 @@ void red_channel_client_push_set_ack(RedChannelClient 
> *rcc)
>  red_channel_client_pipe_add_type(rcc, RED_PIPE_ITEM_TYPE_SET_ACK);
>  }
>  
> -static void red_channel_client_default_disconnect(RedChannelClient *rcc)
> +void red_channel_client_disconnect(RedChannelClient *rcc)
>  {
>  RedChannel *channel = rcc->priv->channel;
>  SpiceCoreInterfaceInternal *core = 
> red_channel_get_core_interface(channel);
> @@ -1781,14 +1766,6 @@ static void 
> red_channel_client_default_disconnect(RedChannelClient *rcc)
>  red_channel_on_disconnect(channel, rcc);
>  }
>  
> -void red_channel_client_disconnect(RedChannelClient *rcc)
> -{
> -RedChannelClientClass *klass = RED_CHANNEL_CLIENT_GET_CLASS(rcc);
> -
> -g_return_if_fail(klass->is_connected != NULL);
> -klass->disconnect(rcc);
> -}
> -
>  int red_channel_client_is_blocked(RedChannelClient *rcc)
>  {
>  return rcc && rcc->priv->send_data.blocked;
> diff --git a/server/red-channel-client.h b/server/red-channel-client.h
> index 75d6cc3..1b0b810 100644
> --- a/server/red-channel-client.h
> +++ b/server/red-channel-client.h
> @@ -192,9 +192,6 @@ struct RedChannelClient
>  struct RedChannelClientClass
>  {
>  GObjectClass parent_class;
> -
> -gboolean (*is_connected)(RedChannelClient *rcc);
> -void (*disconnect)(RedChannelClient *rcc);
>  };
>  
>  #define SPICE_SERVER_ERROR spice_server_error_quark()
> -- 
> 2.9.3
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [PATCH spice-server 2/3] red-channel-client: Remove unused vfuncs

[Frediano Ziglio]

> 
> On Wed, Feb 15, 2017 at 12:49:06PM +, Frediano Ziglio wrote:
> > RedChannelClient is responsible for talking to the client so it knows
> > how if is connected or not.
> 
> s/how//
> 
> I'd mention explicitly that it's RedChannelClient::is_connected and
> RedChannelClient::disconnect which are dropped (would even belong in the
> shortlog if it's not too long).
> 

What about

"red-channel-client: Remove unused RedChannelClient::{is_connected,disconnect}"

> > About disconnect you think that we loose a bit of flexibility however on
> > disconnect the channel is triggered so additional needed process can be
> > handled in destructor.
> 
> I'd drop that paragraph, as we don't have any classes overriding the
> vfunc anyway, and client disconnection and the last ref being dropped
> may occur at different times.
> 
> 
> > These vfuncs where used by DummyChannel used by SoundChannel.
> 
> 
> Acked-by: Christophe Fergeau 
> 
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  server/red-channel-client.c | 27 ++-
> >  server/red-channel-client.h |  3 ---
> >  2 files changed, 2 insertions(+), 28 deletions(-)
> > 
> > diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> > index 32db186..16e5446 100644
> > --- a/server/red-channel-client.c
> > +++ b/server/red-channel-client.c
> > @@ -364,10 +364,6 @@ static void
> > red_channel_client_initable_interface_init(GInitableIface *iface)
> >  iface->init = red_channel_client_initable_init;
> >  }
> >  
> > -static gboolean red_channel_client_default_is_connected(RedChannelClient
> > *rcc);
> > -static void red_channel_client_default_disconnect(RedChannelClient *rcc);
> > -
> > -
> >  static void red_channel_client_constructed(GObject *object)
> >  {
> >  RedChannelClient *self =  RED_CHANNEL_CLIENT(object);
> > @@ -400,9 +396,6 @@ static void
> > red_channel_client_class_init(RedChannelClientClass *klass)
> >  object_class->finalize = red_channel_client_finalize;
> >  object_class->constructed = red_channel_client_constructed;
> >  
> > -klass->is_connected = red_channel_client_default_is_connected;
> > -klass->disconnect = red_channel_client_default_disconnect;
> > -
> >  spec = g_param_spec_pointer("stream", "stream",
> >  "Associated RedStream",
> >  G_PARAM_STATIC_STRINGS
> > @@ -1701,20 +1694,12 @@ gboolean
> > red_channel_client_is_mini_header(RedChannelClient *rcc)
> >  return rcc->priv->is_mini_header;
> >  }
> >  
> > -static gboolean red_channel_client_default_is_connected(RedChannelClient
> > *rcc)
> > +gboolean red_channel_client_is_connected(RedChannelClient *rcc)
> >  {
> >  return rcc->priv->channel
> >  && (g_list_find(red_channel_get_clients(rcc->priv->channel), rcc)
> >  != NULL);
> >  }
> >  
> > -gboolean red_channel_client_is_connected(RedChannelClient *rcc)
> > -{
> > -RedChannelClientClass *klass = RED_CHANNEL_CLIENT_GET_CLASS(rcc);
> > -
> > -g_return_val_if_fail(klass->is_connected != NULL, FALSE);
> > -return klass->is_connected(rcc);
> > -}
> > -
> >  static void red_channel_client_clear_sent_item(RedChannelClient *rcc)
> >  {
> >  rcc->priv->send_data.blocked = FALSE;
> > @@ -1752,7 +1737,7 @@ void red_channel_client_push_set_ack(RedChannelClient
> > *rcc)
> >  red_channel_client_pipe_add_type(rcc, RED_PIPE_ITEM_TYPE_SET_ACK);
> >  }
> >  
> > -static void red_channel_client_default_disconnect(RedChannelClient *rcc)
> > +void red_channel_client_disconnect(RedChannelClient *rcc)
> >  {
> >  RedChannel *channel = rcc->priv->channel;
> >  SpiceCoreInterfaceInternal *core =
> >  red_channel_get_core_interface(channel);
> > @@ -1781,14 +1766,6 @@ static void
> > red_channel_client_default_disconnect(RedChannelClient *rcc)
> >  red_channel_on_disconnect(channel, rcc);
> >  }
> >  
> > -void red_channel_client_disconnect(RedChannelClient *rcc)
> > -{
> > -RedChannelClientClass *klass = RED_CHANNEL_CLIENT_GET_CLASS(rcc);
> > -
> > -g_return_if_fail(klass->is_connected != NULL);
> > -klass->disconnect(rcc);
> > -}
> > -
> >  int red_channel_client_is_blocked(RedChannelClient *rcc)
> >  {
> >  return rcc && rcc->priv->send_data.blocked;
> > diff --git a/server/red-channel-client.h b/server/red-channel-client.h
> > index 75d6cc3..1b0b810 100644
> > --- a/server/red-channel-client.h
> > +++ b/server/red-channel-client.h
> > @@ -192,9 +192,6 @@ struct RedChannelClient
> >  struct RedChannelClientClass
> >  {
> >  GObjectClass parent_class;
> > -
> > -gboolean (*is_connected)(RedChannelClient *rcc);
> > -void (*disconnect)(RedChannelClient *rcc);
> >  };
> >  
> >  #define SPICE_SERVER_ERROR spice_server_error_quark()

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

Re: [Spice-devel] [PATCH spice-server 3/3] Move stuff only related to RedChannelClient into red-channel-client.c

[Christophe Fergeau]

I'd change the log to something like:

"rcc: Make some functions/macros private

Some constants/macros/function prototypes are defined in
red-channel-client.h while they are only used by red-channel-client.c.
This commit moves them to the .c file to make them private"



Acked-by: Christophe Fergeau 
(did not check much more than "it compiles")


On Wed, Feb 15, 2017 at 12:49:07PM +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/red-channel-client.c | 25 -
>  server/red-channel-client.h | 23 ---
>  2 files changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> index 16e5446..a0e213f 100644
> --- a/server/red-channel-client.c
> +++ b/server/red-channel-client.c
> @@ -37,6 +37,14 @@
>  #include "red-client.h"
>  #include "glib-compat.h"
>  
> +#define CLIENT_ACK_WINDOW 20
> +
> +#define MAX_HEADER_SIZE sizeof(SpiceDataHeader)
> +
> +#ifndef IOV_MAX
> +#define IOV_MAX 1024
> +#endif
> +
>  typedef struct SpiceDataHeaderOpaque SpiceDataHeaderOpaque;
>  
>  typedef uint16_t (*get_msg_type_proc)(SpiceDataHeaderOpaque *header);
> @@ -147,6 +155,21 @@ static const SpiceDataHeaderOpaque mini_header_wrapper;
>  static void red_channel_client_clear_sent_item(RedChannelClient *rcc);
>  static void red_channel_client_destroy_remote_caps(RedChannelClient* rcc);
>  static void red_channel_client_initable_interface_init(GInitableIface 
> *iface);
> +static void red_channel_client_set_message_serial(RedChannelClient *channel, 
> uint64_t);
> +
> +/*
> + * When an error occurs over a channel, we treat it as a warning
> + * for spice-server and shutdown the channel.
> + */
> +#define spice_channel_client_error(rcc, format, ...) 
> \
> +do { 
> \
> +RedChannel *_ch = red_channel_client_get_channel(rcc);   
> \
> +uint32_t _type, _id; 
> \
> +g_object_get(_ch, "channel-type", &_type, "id", &_id, NULL); 
> \
> +spice_warning("rcc %p type %u id %u: " format, rcc,  
> \
> +type, id, ## __VA_ARGS__);   
> \
> +red_channel_client_shutdown(rcc);
> \
> +} while (0)
>  
>  G_DEFINE_TYPE_WITH_CODE(RedChannelClient, red_channel_client, G_TYPE_OBJECT,
>  G_IMPLEMENT_INTERFACE(G_TYPE_INITABLE,
> @@ -1568,7 +1591,7 @@ uint64_t 
> red_channel_client_get_message_serial(RedChannelClient *rcc)
>  return rcc->priv->send_data.last_sent_serial + 1;
>  }
>  
> -void red_channel_client_set_message_serial(RedChannelClient *rcc, uint64_t 
> serial)
> +static void red_channel_client_set_message_serial(RedChannelClient *rcc, 
> uint64_t serial)
>  {
>  rcc->priv->send_data.last_sent_serial = serial - 1;
>  }
> diff --git a/server/red-channel-client.h b/server/red-channel-client.h
> index 1b0b810..474a5cd 100644
> --- a/server/red-channel-client.h
> +++ b/server/red-channel-client.h
> @@ -20,7 +20,6 @@
>  
>  #include 
>  #include 
> -#include 
>  #include 
>  
>  #include "red-pipe-item.h"
> @@ -29,13 +28,6 @@
>  
>  G_BEGIN_DECLS
>  
> -#define MAX_HEADER_SIZE sizeof(SpiceDataHeader)
> -#define CLIENT_ACK_WINDOW 20
> -
> -#ifndef IOV_MAX
> -#define IOV_MAX 1024
> -#endif
> -
>  #define RED_TYPE_CHANNEL_CLIENT red_channel_client_get_type()
>  
>  #define RED_CHANNEL_CLIENT(obj) \
> @@ -55,20 +47,6 @@ typedef struct RedChannelClientPrivate 
> RedChannelClientPrivate;
>  
>  GType red_channel_client_get_type(void) G_GNUC_CONST;
>  
> -/*
> - * When an error occurs over a channel, we treat it as a warning
> - * for spice-server and shutdown the channel.
> - */
> -#define spice_channel_client_error(rcc, format, ...) 
> \
> -do { 
> \
> -RedChannel *_ch = red_channel_client_get_channel(rcc);   
> \
> -uint32_t _type, _id; 
> \
> -g_object_get(_ch, "channel-type", &_type, "id", &_id, NULL); 
> \
> -spice_warning("rcc %p type %u id %u: " format, rcc,  
> \
> -type, id, ## __VA_ARGS__);   
> \
> -red_channel_client_shutdown(rcc);
> \
> -} while (0)
> -
>  RedChannelClient *red_channel_client_create(RedChannel *channel,
>  RedClient *client, RedsStream 
> *stream,
>  int 

[Spice-devel] bool or gboolean

[Frediano Ziglio]

Hi,
  question was raised recently on the ML and IRC.

Some time ago we decided to use gboolean but some of us would like
to discuss again.

As any style changes there's no right or wrong, mainly personal
opinions but I think consistency is quite important.

Some consideration (feel free to add/remove/comment)
- gboolean is more used in the code (about 76%)
- TRUE/FALSE are more used (96%)
- bool is C99 convention, defined in stdbool.h
- using gcc the bool type is a bit different from gboolean
  (which basically is an int) catching some problems as
  warnings (like cast between different function pointers
  using bool instead of gboolean/int)
- bool is easier to write (OT: and my vim is more happy too)

There are 2 kind of decision:
- prefer bool or gboolean
- stay consistent with constants (bool -> true/false,
  gboolean -> TRUE/FALSE), continue to use TRUE/FALSE.

I think as usual new code should follow style while old
for "blame" purposes should stay as is (unless code stop
compiling for instance).

For opinions

- bool/gboolean
  - bool
> +1
  - gboolean

- consistent with type
  - yes
> +1 
  - no

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/3] Use class name in comment

[Christophe Fergeau]

ACK, maybe rcc: in the short log

On Wed, Feb 15, 2017 at 12:49:05PM +, Frediano Ziglio wrote:
> red_channel seems a variable name.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/red-channel-client.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/server/red-channel-client.h b/server/red-channel-client.h
> index 4f1d481..75d6cc3 100644
> --- a/server/red-channel-client.h
> +++ b/server/red-channel-client.h
> @@ -205,7 +205,7 @@ typedef enum
>  SPICE_SERVER_ERROR_FAILED
>  } SpiceServerError;
>  
> -/* Messages handled by red_channel
> +/* Messages handled by RedChannel
>   * SET_ACK - sent to client on channel connection
>   * Note that the numbers don't have to correspond to spice message types,
>   * but we keep the 100 first allocated for base channel approach.
> -- 
> 2.9.3
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [PATCH 1/2] authentication: Handle failed SASL authentication separately

[Snir Sheriber]

Hi,

Most of them will never show up, and if they will, it always starts with 
"Unable to authenticate" so i thought it's good enough , but no problem, 
i can show the user only the relevant ones and log the others (or to 
ignore them :] )



On 02/15/2017 05:00 PM, Christophe Fergeau wrote:

On Wed, Feb 15, 2017 at 02:56:36PM +0200, Snir Sheriber wrote:

Hi,
yes, the idea is to present errors which are generated on the sasl server
side, in the err window on the user/sasl-client side (only errors- without
sasl_ok, continue , interact) by sending the error number to the client and
print the relevant string (i'll send these patches again with another one
that do this later so it will be clearer )
imho, this would be better then the current err msg that is being printed..

I think there are 2 separate issues here
1) you want to improve the error message which is presented to the user
2) you are saying that this error message should be the output of
sasl_error()

I'm all for 1), but we should have our own error messages, I don't think
we should directly show SASL error messages in the UI (though it's fine
with me to have them in a debug log).
Most of the error messages from sasl_errstring do not make sense to show to the
user (ie I don't understand half of them) apart from the few that you listed
already. The bigger problem is that these messages are untranslated.

const char *sasl_errstring(int saslerr,
   const char *langlist __attribute__((unused)),
   const char **outlang)
{
   if (outlang) *outlang="en-us";

   switch(saslerr)
 {
 case SASL_CONTINUE: return "another step is needed in authentication";
 case SASL_OK:   return "successful result";
 case SASL_FAIL: return "generic failure";
 case SASL_NOMEM:return "no memory available";
 case SASL_BUFOVER:  return "overflowed buffer";
 case SASL_NOMECH:   return "no mechanism available";
 case SASL_BADPROT:  return "bad protocol / cancel";
 case SASL_NOTDONE:  return "can't request information until later in 
exchange";
 case SASL_BADPARAM: return "invalid parameter supplied";
 case SASL_TRYAGAIN: return "transient failure (e.g., weak key)";
 case SASL_BADMAC:   return "integrity check failed";
 case SASL_NOTINIT:  return "SASL library is not initialized";
  /* -- client only codes -- */
 case SASL_INTERACT:   return "needs user interaction";
 case SASL_BADSERV:return "server failed mutual authentication step";
 case SASL_WRONGMECH:  return "mechanism doesn't support requested feature";
  /* -- server only codes -- */
 case SASL_BADAUTH:return "authentication failure";
 case SASL_NOAUTHZ:return "authorization failure";
 case SASL_TOOWEAK:return "mechanism too weak for this user";
 case SASL_ENCRYPT:return "encryption needed to use mechanism";
 case SASL_TRANS:  return "One time use of a plaintext password will enable 
requested mechanism for user";
 case SASL_EXPIRED:return "passphrase expired, has to be reset";
 case SASL_DISABLED:   return "account disabled";
 case SASL_NOUSER: return "user not found";
 case SASL_BADVERS:return "version mismatch with plug-in";
 case SASL_UNAVAIL:return "remote authentication server unavailable";
 case SASL_NOVERIFY:   return "user exists, but no verifier for user";
 case SASL_PWLOCK: return "passphrase locked";
 case SASL_NOCHANGE:   return "requested change was not needed";
 case SASL_WEAKPASS:   return "passphrase is too weak for security policy";
 case SASL_NOUSERPASS: return "user supplied passwords are not permitted";
 case SASL_NEED_OLD_PASSWD: return "sasl_setpass needs old password in order 
"
   to perform password change";
 case SASL_CONSTRAINT_VIOLAT: return "sasl_setpass can't store a property 
because "
 "of a constraint violation";
 case SASL_BADBINDING: return "channel binding failure";
 case SASL_CONFIGERR:  return "error when parsing configuration file";

 default:   return "undefined error!";
 }
}

Christophe


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

Re: [Spice-devel] [PATCH] Switch over to using keycodemapdb submodule

[Daniel P. Berrange]

On Wed, Feb 15, 2017 at 04:44:22PM +0100, Pavel Grunt wrote:
> nice, although the python script looks pretty heavy.

Different users of the keymap database have different output formats.
We don't want ever user re-implementing the same logic, as that's
just as bad what we have today. So the tool is designed to satisfy
current usage requireents from libvirt, gtk-vnc & spice-gtk

> Anyway, in spice-gtk we support python3, imo it cannot go in without
> the python3 support.

Ok, that's easy enough todo.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-server v2 14/14] rcc: Consistently name RedChannelClient 'rcc'

[Jonathon Jongsma]

On Wed, 2017-02-15 at 08:51 +0100, Christophe Fergeau wrote:
> On Tue, Feb 14, 2017 at 12:14:56PM -0600, Jonathon Jongsma wrote:
> > To be honest, I could go either way on this one. I like the
> > consistency, but I don't like the fact that it makes things like
> > "git
> > blame" less useful. I'll leave the decision to others ;)
> 
> I've pushed the series without this patch for now, 'self' is only
> used
> in GObject boilerplate, so the inconsistency is not too bad (at least
> the patch series itself did not add more inconsistencies this time).
> On the other hand, 'tig blame' and the ',' shortcut help a lot making
> git blame useful even with such commits.
> 
> Christophe


Brilliant. I have been using 'tig' for a while but didn't know about
the blame view, nor did I know about the ',' shortcut. That makes
things much easier. Thanks for the tip. 

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

Re: [Spice-devel] [spice-server v4 5/5] Change some spice_printerr() to spice_debug()

[Christophe de Dinechin]

> On 15 Feb 2017, at 12:45, Frediano Ziglio  wrote:
> 
>> @@ -134,7 +134,7 @@ static void red_qxl_display_migrate(RedChannelClient
>> *rcc)
>> }
>> g_object_get(channel, "channel-type", , "id", , NULL);
>> dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel),
>> "dispatcher");
>> -spice_printerr("channel type %u id %u", type, id);
>> +spice_debug("channel type %u id %u", type, id);
>> payload.rcc = rcc;
>> dispatcher_send_message(dispatcher,
>> RED_WORKER_MESSAGE_DISPLAY_MIGRATE,
> 
> Looks like there's lot of debugging on migration.
> Like we are not really sure the code is working.

Maybe someone once needed a lot of debug information, and kept it around in 
case things start going south. That probably means we have annotations that may 
end up being annoying for everybody debugging anything else than migration.

This leads me to a meta-question: would it make sense to add “traces” to the 
spice code, i.e. dynamically configurable flags that activate sets of related 
printf. Today, we have spice “errors”, “debug” or “info”, but we could have 
finer-grained logging for specific topics, e.g. migration or encoding.

In Tao3D, there are topical traces like this declared here: 
https://github.com/c3d/tao-3D/blob/master/tao/traces.tbl. If you want to have 
the “font” trace activated, you simply run the program with the -tfont option, 
or set a flag from within a debugger. Within the code, traces are tested with 
something like if (TRACE(font)), e.g. 
https://github.com/c3d/tao-3D/blob/master/tao/font.cpp#L102. A special form a 
printf takes a trace name as input, e.g. we could have spice_trace(font, …). 
The cost of a not-taken trace is a mere not-taken if statement with a bit-field 
read (one trace = one bit), so the overhead is really low. This means that you 
can leave verbose debug information in place in case it helps addressing a 
specific kind of debug situation.

Would anything like this make sense for spice?


>> @@ -148,7 +148,7 @@ static void red_qxl_set_cursor_peer(RedChannel *channel,
>> RedClient *client, Reds
>> {
>> RedWorkerMessageCursorConnect payload = {0,};
>> Dispatcher *dispatcher = (Dispatcher
>> *)g_object_get_data(G_OBJECT(channel), "dispatcher");
>> -spice_printerr("");
>> +spice_debug("");
>> payload.client = client;
>> payload.stream = stream;
>> payload.migration = migration;
> 
> Remove
> 
>> @@ -176,7 +176,7 @@ static void
>> red_qxl_disconnect_cursor_peer(RedChannelClient *rcc)
>> }
>> 
>> dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel),
>> "dispatcher");
>> -spice_printerr("");
>> +spice_debug("");
>> payload.rcc = rcc;
>> 
>> dispatcher_send_message(dispatcher,
> 
> Remove
> 
>> @@ -196,7 +196,7 @@ static void red_qxl_cursor_migrate(RedChannelClient *rcc)
>> }
>> g_object_get(channel, "channel-type", , "id", , NULL);
>> dispatcher = (Dispatcher *)g_object_get_data(G_OBJECT(channel),
>> "dispatcher");
>> -spice_printerr("channel type %u id %u", type, id);
>> +spice_debug("channel type %u id %u", type, id);
>> payload.rcc = rcc;
>> dispatcher_send_message(dispatcher,
>> RED_WORKER_MESSAGE_CURSOR_MIGRATE,
>> @@ -652,7 +652,7 @@ static void red_qxl_loadvm_commands(QXLState *qxl_state,
>> {
>> RedWorkerMessageLoadvmCommands payload;
>> 
>> -spice_printerr("");
>> +spice_debug("");
>> payload.count = count;
>> payload.ext = ext;
>> dispatcher_send_message(qxl_state->dispatcher,
>> diff --git a/server/smartcard.c b/server/smartcard.c
>> index a7cc614..4d27eff 100644
>> --- a/server/smartcard.c
>> +++ b/server/smartcard.c
>> @@ -213,7 +213,7 @@ static void smartcard_remove_client(RedCharDevice *self,
>> RedClient *client)
>> RedCharDeviceSmartcard *dev = RED_CHAR_DEVICE_SMARTCARD(self);
>> RedChannelClient *rcc = RED_CHANNEL_CLIENT(dev->priv->scc);
>> 
>> -spice_printerr("smartcard  dev %p, client %p", dev, client);
>> +spice_debug("smartcard  dev %p, client %p", dev, client);
>> spice_assert(dev->priv->scc &&
>>  red_channel_client_get_client(rcc) == client);
>> red_channel_client_shutdown(rcc);
> 
> Not sure. Not really into smartcard to have an opinion.
> 
>> diff --git a/server/spicevmc.c b/server/spicevmc.c
>> index 4c9f442..2b3290a 100644
>> --- a/server/spicevmc.c
>> +++ b/server/spicevmc.c
>> @@ -436,7 +436,7 @@ static void spicevmc_char_dev_remove_client(RedCharDevice
>> *self,
>> RedCharDeviceSpiceVmc *vmc = RED_CHAR_DEVICE_SPICEVMC(self);
>> RedVmcChannel *channel = RED_VMC_CHANNEL(vmc->channel);
>> 
>> -spice_printerr("vmc channel %p, client %p", channel, client);
>> +spice_debug("vmc channel %p, client %p", channel, client);
>> spice_assert(channel->rcc &&
>>  red_channel_client_get_client(channel->rcc) == client);
>> 
> 
> Same as smartcard
> 
> 

[Spice-devel] [PATCH] Switch over to using keycodemapdb submodule

[Daniel P. Berrange]

Signed-off-by: Daniel P. Berrange 
---
 .gitmodules   |   3 +
 configure.ac  |  11 --
 src/Makefile.am   |  30 ++--
 src/keycodemapdb  |   1 +
 src/keymap-gen.pl | 214 ---
 src/keymaps.csv   | 511 --
 6 files changed, 17 insertions(+), 753 deletions(-)
 create mode 16 src/keycodemapdb
 delete mode 100755 src/keymap-gen.pl
 delete mode 100644 src/keymaps.csv

diff --git a/.gitmodules b/.gitmodules
index 0c618ee..82467e4 100644
--- a/.gitmodules
+++ b/.gitmodules
@@ -1,3 +1,6 @@
 [submodule "spice-common"]
path = spice-common
url = ../spice-common
+[submodule "src/keycodemapdb"]
+   path = src/keycodemapdb
+   url = https://gitlab.com/keycodemap/keycodemapdb.git
diff --git a/configure.ac b/configure.ac
index 463fbe0..763d14b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -86,17 +86,6 @@ AC_SUBST(SPICE_GTK_MICRO_VERSION)
 dnl =
 dnl Chek optional features
 
-srcdir="$(dirname $0)"
-if test ! -e "$srcdir/src/vncdisplaykeymap_osx2xtkbd.c"; then
-  AC_MSG_CHECKING([for Text::CSV Perl module])
-  perl -MText::CSV -e "" >/dev/null 2>&1
-  if test $? -ne 0 ; then
-AC_MSG_RESULT([not found])
-AC_MSG_ERROR([Text::CSV Perl module is required to compile this package])
-  fi
-  AC_MSG_RESULT([found])
-fi
-
 SPICE_GLIB_REQUIRES=""
 SPICE_GTK_REQUIRES=""
 
diff --git a/src/Makefile.am b/src/Makefile.am
index 7542fac..76c2755 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -26,14 +26,13 @@ GLIBGENS =  \
spice-widget-enums.h\
$(NULL)
 
-CLEANFILES = $(GLIBGENS)
+CLEANFILES = $(GLIBGENS) $(KEYMAPS)
 BUILT_SOURCES = $(GLIBGENS) $(KEYMAPS)
 
 EXTRA_DIST =   \
-   $(KEYMAPS)  \
decode-glz-tmpl.c   \
-   keymap-gen.pl   \
-   keymaps.csv \
+   $(KEYMAP_CSV)   \
+   $(KEYMAP_GEN)   \
map-file\
spice-glib-sym-file \
spice-gtk-sym-file  \
@@ -66,7 +65,8 @@ GTK_SYMBOLS_LDFLAGS = -export-symbols 
${srcdir}/spice-gtk-sym-file
 GTK_SYMBOLS_FILE = spice-gtk-sym-file
 endif
 
-KEYMAP_GEN = $(srcdir)/keymap-gen.pl
+KEYMAP_GEN = keycodemapdb/tools/keymap-gen
+KEYMAP_CSV = keycodemapdb/data/keymaps.csv
 
 SPICE_COMMON_CPPFLAGS =\
-DSPICE_COMPILATION \
@@ -483,32 +483,28 @@ spice-widget-enums.h: spice-widget.h
 
 
 vncdisplaykeymap.c: $(KEYMAPS)
+$(KEYMAPS): $(srcdir)/$(KEYMAP_GEN) $(srcdir)/$(KEYMAP_CSV)
 
-$(KEYMAPS): $(KEYMAP_GEN) keymaps.csv
-
-# Note despite being autogenerated these are not part of CLEANFILES, they
-# are actually a part of EXTRA_DIST to avoid the need for perl(Text::CSV) by
-# end users
 vncdisplaykeymap_xorgevdev2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgevdev xtkbd > $@ || 
rm $@
+   $(AM_V_GEN)$(PYTHON) $(KEYMAP_GEN) --lang glib2 --varname 
keymap_xorgevdev2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) xorgevdev xtkbd > $@ || 
rm $@
 
 vncdisplaykeymap_xorgkbd2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgkbd xtkbd > $@ || rm 
$@
+   $(AM_V_GEN)$(PYTHON) $(KEYMAP_GEN) --lang glib2 --varname 
keymap_xorgkbd2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) xorgkbd xtkbd > $@ || rm 
$@
 
 vncdisplaykeymap_xorgxquartz2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgxquartz xtkbd > $@ 
|| rm $@
+   $(AM_V_GEN)$(PYTHON) $(KEYMAP_GEN) --lang glib2 --varname 
keymap_xorgxquartz2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) xorgxquartz xtkbd > 
$@ || rm $@
 
 vncdisplaykeymap_xorgxwin2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv xorgxwin xtkbd > $@ || 
rm $@
+   $(AM_V_GEN)$(PYTHON) $(KEYMAP_GEN) --lang glib2 --varname 
keymap_xorgxwin2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) xorgxwin xtkbd > $@ || 
rm $@
 
 vncdisplaykeymap_osx2xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv osx xtkbd > $@ || rm $@
+   $(AM_V_GEN)$(PYTHON) $(KEYMAP_GEN) --lang glib2 --varname 
keymap_osx2xtkbd code-map $(srcdir)/$(KEYMAP_CSV) osx xtkbd > $@ || rm $@
 
 vncdisplaykeymap_win322xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv win32 xtkbd > $@ || rm $@
+   $(AM_V_GEN)$(PYTHON) $(KEYMAP_GEN) --lang glib2 --varname 
keymap_win322xtkbd code-map $(srcdir)/$(KEYMAP_CSV) win32 xtkbd > $@ || rm $@
 
 vncdisplaykeymap_x112xtkbd.c:
-   $(AM_V_GEN)$(KEYMAP_GEN) $(srcdir)/keymaps.csv x11 xtkbd > $@ || rm $@
+   $(AM_V_GEN)$(PYTHON) $(KEYMAP_GEN) --lang glib2 --varname 
keymap_x112xtkbd code-map $(srcdir)/$(KEYMAP_CSV) x11 xtkbd > $@ || rm $@
 
 -include 

Re: [Spice-devel] [PATCH spice-server 3/4] reds-stream: Introduce reds_stream_set_no_delay() helper

[Pavel Grunt]

On Wed, 2017-02-15 at 15:16 +0100, Christophe Fergeau wrote:
> On Wed, Feb 15, 2017 at 09:14:10AM -0500, Frediano Ziglio wrote:
> > > 
> > > On Wed, Feb 15, 2017 at 06:39:13AM -0500, Frediano Ziglio wrote:
> > > > 
> > > > Patch is good however the use of bool is inconsistent.
> > > > Although I _much_ prefer bool/true/false we decided for
> > > > gboolean/TRUE/FALSE.
> > > 
> > > reds-stream.[ch] is already exclusively using bool instead of
> > > gboolean,
> > > so I'd prefer to stick with this. I have a patch (series) in
> > > preparation
> > > changing gboolean to bool (with justification), so I would not
> > > change
> > > reds-stream.[ch] just yet.
> > > 
> > > Christophe
> > > 
> > 
> > Can you be at least consistent in server/reds-stream.c ?
> > 
> > bool -> true/false
> > 
> 
> Existing code already uses bool together with TRUE/FALSE, so I would
> keep the discussion about TRUE vs true for another time as well ;)
> 

I am against mixing, TRUE is gboolean, true is bool

imo it is simple: spice server requires C99 => we have stdbool => we
can use it if it has benefits

Pavel

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

Re: [Spice-devel] [PATCH spice-server 3/4] reds-stream: Introduce reds_stream_set_no_delay() helper

[Christophe Fergeau]

On Wed, Feb 15, 2017 at 09:14:10AM -0500, Frediano Ziglio wrote:
> > 
> > On Wed, Feb 15, 2017 at 06:39:13AM -0500, Frediano Ziglio wrote:
> > > 
> > > Patch is good however the use of bool is inconsistent.
> > > Although I _much_ prefer bool/true/false we decided for
> > > gboolean/TRUE/FALSE.
> > 
> > reds-stream.[ch] is already exclusively using bool instead of gboolean,
> > so I'd prefer to stick with this. I have a patch (series) in preparation
> > changing gboolean to bool (with justification), so I would not change
> > reds-stream.[ch] just yet.
> > 
> > Christophe
> > 
> 
> Can you be at least consistent in server/reds-stream.c ?
> 
> bool -> true/false
> 

Existing code already uses bool together with TRUE/FALSE, so I would
keep the discussion about TRUE vs true for another time as well ;)

Christophe


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

Re: [Spice-devel] [PATCH spice-server 3/4] reds-stream: Introduce reds_stream_set_no_delay() helper

[Christophe Fergeau]

On Wed, Feb 15, 2017 at 06:39:13AM -0500, Frediano Ziglio wrote:
> 
> Patch is good however the use of bool is inconsistent.
> Although I _much_ prefer bool/true/false we decided for gboolean/TRUE/FALSE.

reds-stream.[ch] is already exclusively using bool instead of gboolean,
so I'd prefer to stick with this. I have a patch (series) in preparation
changing gboolean to bool (with justification), so I would not change
reds-stream.[ch] just yet.

Christophe


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

Re: [Spice-devel] [PATCH spice-server] sound: Avoid unused IOV_MAX definition

[Frediano Ziglio]

> 
> Hi,
> 
> On Wed, 2017-02-15 at 12:37 +, Frediano Ziglio wrote:
> > The usage was removed with commit
> > 7ea1f2c133a8e57523078ba3112cec6a1d2f353e
> > ("sound: Use RedChannelClient to receive/send data").
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  server/sound.c | 4 
> >  1 file changed, 4 deletions(-)
> > 
> > diff --git a/server/sound.c b/server/sound.c
> > index 530a7ec..afe9c96 100644
> > --- a/server/sound.c
> > +++ b/server/sound.c
> > @@ -39,10 +39,6 @@
> >  #include "sound.h"
> >  #include "main-channel-client.h"
> >  
> > -#ifndef IOV_MAX
> > -#define IOV_MAX 1024
> > -#endif
> > -
> 
> Actually it was "redefined" - its definition is in red-channel-
> client.h which is included through main-channel-client.h.
> 

Can't be redefined, it's using a #ifndef.
But it's not used in this source file so there is no
need to have it defined. Was used with DummyChannel
(DummyChannel == Voldemort!).

> There is also some usage in reds-stream.c (I am not sure if it
> includes the header) besides that it can be private to red-channel-
> client.c
> 
> Pavel
> 

I sent already a patch doing this.
I think reds-stream.c use it as a flag to limit iovcnt.
Note that reds-stream.c do not include red-channel-client.h.

> >  #define SND_RECEIVE_BUF_SIZE (16 * 1024 * 2)
> >  #define RECORD_SAMPLES_SIZE (SND_RECEIVE_BUF_SIZE >> 2)
> >  
> 

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

Re: [Spice-devel] [PATCH spice-server] sound: Avoid unused IOV_MAX definition

[Pavel Grunt]

Hi,

On Wed, 2017-02-15 at 12:37 +, Frediano Ziglio wrote:
> The usage was removed with commit
> 7ea1f2c133a8e57523078ba3112cec6a1d2f353e
> ("sound: Use RedChannelClient to receive/send data").
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/sound.c | 4 
>  1 file changed, 4 deletions(-)
> 
> diff --git a/server/sound.c b/server/sound.c
> index 530a7ec..afe9c96 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -39,10 +39,6 @@
>  #include "sound.h"
>  #include "main-channel-client.h"
>  
> -#ifndef IOV_MAX
> -#define IOV_MAX 1024
> -#endif
> -

Actually it was "redefined" - its definition is in red-channel-
client.h which is included through main-channel-client.h.

There is also some usage in reds-stream.c (I am not sure if it
includes the header) besides that it can be private to red-channel-
client.c

Pavel

>  #define SND_RECEIVE_BUF_SIZE (16 * 1024 * 2)
>  #define RECORD_SAMPLES_SIZE (SND_RECEIVE_BUF_SIZE >> 2)
>  
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH 1/2] authentication: Handle failed SASL authentication separately

[Snir Sheriber]

Hi,


On 02/14/2017 06:42 PM, Christophe Fergeau wrote:

On Mon, Feb 13, 2017 at 03:49:44PM +0200, Snir Sheriber wrote:

Remove handling with failures in the SASL authentication
process to separate function and display the error message
as reported by the SASL client (could also display SASL
server error message if error number was sent to the client)
---
  src/spice-channel.c | 42 +-
  1 file changed, 29 insertions(+), 13 deletions(-)

diff --git a/src/spice-channel.c b/src/spice-channel.c
index 6556db3..37e36d9 100644
--- a/src/spice-channel.c
+++ b/src/spice-channel.c
@@ -1113,28 +1113,44 @@ static int spice_channel_read(SpiceChannel *channel, 
void *data, size_t length)
  return length;
  }
  
+#if HAVE_SASL

  /* coroutine context */
-static void spice_channel_failed_authentication(SpiceChannel *channel,
-gboolean invalidPassword)
+static void spice_channel_failed_sasl_authentication(SpiceChannel *channel, 
int err)
  {
  SpiceChannelPrivate *c = channel->priv;
+gint err_code; /* Affects the authentication window fileds */
  
  if (c->auth_needs_username && c->auth_needs_password)

-g_set_error_literal(>error,
-SPICE_CLIENT_ERROR,
-
SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD_AND_USERNAME,
-_("Authentication failed: password and username are 
required"));
+err_code = SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD_AND_USERNAME;
  else if (c->auth_needs_username)
+err_code = SPICE_CLIENT_ERROR_AUTH_NEEDS_USERNAME;
+else
+err_code = SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD;
+
+if (err < 0)
  g_set_error_literal(>error,
  SPICE_CLIENT_ERROR,
-SPICE_CLIENT_ERROR_AUTH_NEEDS_USERNAME,
-_("Authentication failed: username is required"));
-else if (c->auth_needs_password)
+err_code,
+sasl_errstring(err, NULL, NULL));

I'm not sure what you mean by "display" in the commit log. If you want
this string to appear in the debug log, this sounds fine to me. If the
goal is to show that to the user, I'm not so sure about this, the errors
listed at
https://docs.oracle.com/cd/E53394_01/html/E54774/sasl-errors-3sasl.html#REFMAN3Bsasl-errors-3sasl
do not seem so useful.

Christophe


yes, the idea is to present errors which are generated on the sasl 
server side, in the err window on the user/sasl-client side (only 
errors- without sasl_ok, continue , interact) by sending the error 
number to the client and print the relevant string (i'll send these 
patches again with another one that do this later so it will be clearer )
imho, this would be better then the current err msg that is being 
printed.. e.g "Authentication failure" would printed if password is 
incorrect or "User not found"  for wrong\nonexist user in oppose to 
"password and user-name are required" that currently printed for both.
It is possible that other message will show up and i think it's also 
fine but it is also possible to filter only specific errors and print 
whatever we want.


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

[Spice-devel] [PATCH spice-server 3/3] Move stuff only related to RedChannelClient into red-channel-client.c

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 server/red-channel-client.c | 25 -
 server/red-channel-client.h | 23 ---
 2 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/server/red-channel-client.c b/server/red-channel-client.c
index 16e5446..a0e213f 100644
--- a/server/red-channel-client.c
+++ b/server/red-channel-client.c
@@ -37,6 +37,14 @@
 #include "red-client.h"
 #include "glib-compat.h"
 
+#define CLIENT_ACK_WINDOW 20
+
+#define MAX_HEADER_SIZE sizeof(SpiceDataHeader)
+
+#ifndef IOV_MAX
+#define IOV_MAX 1024
+#endif
+
 typedef struct SpiceDataHeaderOpaque SpiceDataHeaderOpaque;
 
 typedef uint16_t (*get_msg_type_proc)(SpiceDataHeaderOpaque *header);
@@ -147,6 +155,21 @@ static const SpiceDataHeaderOpaque mini_header_wrapper;
 static void red_channel_client_clear_sent_item(RedChannelClient *rcc);
 static void red_channel_client_destroy_remote_caps(RedChannelClient* rcc);
 static void red_channel_client_initable_interface_init(GInitableIface *iface);
+static void red_channel_client_set_message_serial(RedChannelClient *channel, 
uint64_t);
+
+/*
+ * When an error occurs over a channel, we treat it as a warning
+ * for spice-server and shutdown the channel.
+ */
+#define spice_channel_client_error(rcc, format, ...)   
  \
+do {   
  \
+RedChannel *_ch = red_channel_client_get_channel(rcc); 
  \
+uint32_t _type, _id;   
  \
+g_object_get(_ch, "channel-type", &_type, "id", &_id, NULL);   
  \
+spice_warning("rcc %p type %u id %u: " format, rcc,
  \
+type, id, ## __VA_ARGS__); 
  \
+red_channel_client_shutdown(rcc);  
  \
+} while (0)
 
 G_DEFINE_TYPE_WITH_CODE(RedChannelClient, red_channel_client, G_TYPE_OBJECT,
 G_IMPLEMENT_INTERFACE(G_TYPE_INITABLE,
@@ -1568,7 +1591,7 @@ uint64_t 
red_channel_client_get_message_serial(RedChannelClient *rcc)
 return rcc->priv->send_data.last_sent_serial + 1;
 }
 
-void red_channel_client_set_message_serial(RedChannelClient *rcc, uint64_t 
serial)
+static void red_channel_client_set_message_serial(RedChannelClient *rcc, 
uint64_t serial)
 {
 rcc->priv->send_data.last_sent_serial = serial - 1;
 }
diff --git a/server/red-channel-client.h b/server/red-channel-client.h
index 1b0b810..474a5cd 100644
--- a/server/red-channel-client.h
+++ b/server/red-channel-client.h
@@ -20,7 +20,6 @@
 
 #include 
 #include 
-#include 
 #include 
 
 #include "red-pipe-item.h"
@@ -29,13 +28,6 @@
 
 G_BEGIN_DECLS
 
-#define MAX_HEADER_SIZE sizeof(SpiceDataHeader)
-#define CLIENT_ACK_WINDOW 20
-
-#ifndef IOV_MAX
-#define IOV_MAX 1024
-#endif
-
 #define RED_TYPE_CHANNEL_CLIENT red_channel_client_get_type()
 
 #define RED_CHANNEL_CLIENT(obj) \
@@ -55,20 +47,6 @@ typedef struct RedChannelClientPrivate 
RedChannelClientPrivate;
 
 GType red_channel_client_get_type(void) G_GNUC_CONST;
 
-/*
- * When an error occurs over a channel, we treat it as a warning
- * for spice-server and shutdown the channel.
- */
-#define spice_channel_client_error(rcc, format, ...)   
  \
-do {   
  \
-RedChannel *_ch = red_channel_client_get_channel(rcc); 
  \
-uint32_t _type, _id;   
  \
-g_object_get(_ch, "channel-type", &_type, "id", &_id, NULL);   
  \
-spice_warning("rcc %p type %u id %u: " format, rcc,
  \
-type, id, ## __VA_ARGS__); 
  \
-red_channel_client_shutdown(rcc);  
  \
-} while (0)
-
 RedChannelClient *red_channel_client_create(RedChannel *channel,
 RedClient *client, RedsStream 
*stream,
 int monitor_latency,
@@ -92,7 +70,6 @@ int red_channel_client_handle_message(RedChannelClient *rcc, 
uint16_t type,
 void red_channel_client_init_send_data(RedChannelClient *rcc, uint16_t 
msg_type);
 
 uint64_t red_channel_client_get_message_serial(RedChannelClient *channel);
-void red_channel_client_set_message_serial(RedChannelClient *channel, 
uint64_t);
 
 /* When sending a msg. Should first call red_channel_client_begin_send_message.
  * It will first send the pending urgent data, if there is any, and then
-- 
2.9.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org

[Spice-devel] [PATCH spice-server 1/3] Use class name in comment

[Frediano Ziglio]

red_channel seems a variable name.

Signed-off-by: Frediano Ziglio 
---
 server/red-channel-client.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server/red-channel-client.h b/server/red-channel-client.h
index 4f1d481..75d6cc3 100644
--- a/server/red-channel-client.h
+++ b/server/red-channel-client.h
@@ -205,7 +205,7 @@ typedef enum
 SPICE_SERVER_ERROR_FAILED
 } SpiceServerError;
 
-/* Messages handled by red_channel
+/* Messages handled by RedChannel
  * SET_ACK - sent to client on channel connection
  * Note that the numbers don't have to correspond to spice message types,
  * but we keep the 100 first allocated for base channel approach.
-- 
2.9.3

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

[Spice-devel] [PATCH spice-server 2/3] red-channel-client: Remove unused vfuncs

[Frediano Ziglio]

RedChannelClient is responsible for talking to the client so it knows
how if is connected or not.
About disconnect you think that we loose a bit of flexibility however on
disconnect the channel is triggered so additional needed process can be
handled in destructor.
These vfuncs where used by DummyChannel used by SoundChannel.

Signed-off-by: Frediano Ziglio 
---
 server/red-channel-client.c | 27 ++-
 server/red-channel-client.h |  3 ---
 2 files changed, 2 insertions(+), 28 deletions(-)

diff --git a/server/red-channel-client.c b/server/red-channel-client.c
index 32db186..16e5446 100644
--- a/server/red-channel-client.c
+++ b/server/red-channel-client.c
@@ -364,10 +364,6 @@ static void 
red_channel_client_initable_interface_init(GInitableIface *iface)
 iface->init = red_channel_client_initable_init;
 }
 
-static gboolean red_channel_client_default_is_connected(RedChannelClient *rcc);
-static void red_channel_client_default_disconnect(RedChannelClient *rcc);
-
-
 static void red_channel_client_constructed(GObject *object)
 {
 RedChannelClient *self =  RED_CHANNEL_CLIENT(object);
@@ -400,9 +396,6 @@ static void 
red_channel_client_class_init(RedChannelClientClass *klass)
 object_class->finalize = red_channel_client_finalize;
 object_class->constructed = red_channel_client_constructed;
 
-klass->is_connected = red_channel_client_default_is_connected;
-klass->disconnect = red_channel_client_default_disconnect;
-
 spec = g_param_spec_pointer("stream", "stream",
 "Associated RedStream",
 G_PARAM_STATIC_STRINGS
@@ -1701,20 +1694,12 @@ gboolean 
red_channel_client_is_mini_header(RedChannelClient *rcc)
 return rcc->priv->is_mini_header;
 }
 
-static gboolean red_channel_client_default_is_connected(RedChannelClient *rcc)
+gboolean red_channel_client_is_connected(RedChannelClient *rcc)
 {
 return rcc->priv->channel
 && (g_list_find(red_channel_get_clients(rcc->priv->channel), rcc) != 
NULL);
 }
 
-gboolean red_channel_client_is_connected(RedChannelClient *rcc)
-{
-RedChannelClientClass *klass = RED_CHANNEL_CLIENT_GET_CLASS(rcc);
-
-g_return_val_if_fail(klass->is_connected != NULL, FALSE);
-return klass->is_connected(rcc);
-}
-
 static void red_channel_client_clear_sent_item(RedChannelClient *rcc)
 {
 rcc->priv->send_data.blocked = FALSE;
@@ -1752,7 +1737,7 @@ void red_channel_client_push_set_ack(RedChannelClient 
*rcc)
 red_channel_client_pipe_add_type(rcc, RED_PIPE_ITEM_TYPE_SET_ACK);
 }
 
-static void red_channel_client_default_disconnect(RedChannelClient *rcc)
+void red_channel_client_disconnect(RedChannelClient *rcc)
 {
 RedChannel *channel = rcc->priv->channel;
 SpiceCoreInterfaceInternal *core = red_channel_get_core_interface(channel);
@@ -1781,14 +1766,6 @@ static void 
red_channel_client_default_disconnect(RedChannelClient *rcc)
 red_channel_on_disconnect(channel, rcc);
 }
 
-void red_channel_client_disconnect(RedChannelClient *rcc)
-{
-RedChannelClientClass *klass = RED_CHANNEL_CLIENT_GET_CLASS(rcc);
-
-g_return_if_fail(klass->is_connected != NULL);
-klass->disconnect(rcc);
-}
-
 int red_channel_client_is_blocked(RedChannelClient *rcc)
 {
 return rcc && rcc->priv->send_data.blocked;
diff --git a/server/red-channel-client.h b/server/red-channel-client.h
index 75d6cc3..1b0b810 100644
--- a/server/red-channel-client.h
+++ b/server/red-channel-client.h
@@ -192,9 +192,6 @@ struct RedChannelClient
 struct RedChannelClientClass
 {
 GObjectClass parent_class;
-
-gboolean (*is_connected)(RedChannelClient *rcc);
-void (*disconnect)(RedChannelClient *rcc);
 };
 
 #define SPICE_SERVER_ERROR spice_server_error_quark()
-- 
2.9.3

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

[Spice-devel] [PATCH spice-server] sound: Avoid unused IOV_MAX definition

[Frediano Ziglio]

The usage was removed with commit 7ea1f2c133a8e57523078ba3112cec6a1d2f353e
("sound: Use RedChannelClient to receive/send data").

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

diff --git a/server/sound.c b/server/sound.c
index 530a7ec..afe9c96 100644
--- a/server/sound.c
+++ b/server/sound.c
@@ -39,10 +39,6 @@
 #include "sound.h"
 #include "main-channel-client.h"
 
-#ifndef IOV_MAX
-#define IOV_MAX 1024
-#endif
-
 #define SND_RECEIVE_BUF_SIZE (16 * 1024 * 2)
 #define RECORD_SAMPLES_SIZE (SND_RECEIVE_BUF_SIZE >> 2)
 
-- 
2.9.3

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

Re: [Spice-devel] [PATCH spice-server 2/4] Do not set TCP_NODELAY flag twice

[Christophe Fergeau]

On Wed, Feb 15, 2017 at 06:48:32AM -0500, Frediano Ziglio wrote:
> > 
> > On Wed, Feb 15, 2017 at 11:31:59AM +, Frediano Ziglio wrote:
> > > TCP_NODELAY flag is set by default for all connection inside
> > > reds.c so there's no need to set again for the single
> > > client channel.
> > 
> > Would have been nice to keep "Do not set not blocking flag twice"
> > together with this patch, and push them at the same time..
> > 
> > > 
> > > Note that there are still some call to setsockopt to set this
> > > option but in this case the flag can reset the flag.
> > 
> > 'some calls'
> > the flag can reset the flag?
> > 
> 
> Yes, confusing, what about:
> 
> "Note that there are still some call to setsockopt to change this
> option."

"some calls", and yes, much clearer :)

Christophe


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

Re: [Spice-devel] [PATCH spice-server 2/2] sound: Use memcpy instead of manually copy volume array

[Frediano Ziglio]

Ping

> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/sound.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/server/sound.c b/server/sound.c
> index 7c36174..b692646 100644
> --- a/server/sound.c
> +++ b/server/sound.c
> @@ -384,7 +384,6 @@ static int
> snd_playback_send_migrate(PlaybackChannelClient *client)
>  static int snd_send_volume(SndChannelClient *client, uint32_t cap, int msg)
>  {
>  SpiceMsgAudioVolume *vol;
> -uint8_t c;
>  RedChannelClient *rcc = RED_CHANNEL_CLIENT(client);
>  SpiceMarshaller *m = red_channel_client_get_marshaller(rcc);
>  SndChannel *channel = SND_CHANNEL(red_channel_client_get_channel(rcc));
> @@ -398,9 +397,8 @@ static int snd_send_volume(SndChannelClient *client,
> uint32_t cap, int msg)
>   st->volume_nchannels * sizeof (uint16_t));
>  red_channel_client_init_send_data(rcc, msg);
>  vol->nchannels = st->volume_nchannels;
> -for (c = 0; c < st->volume_nchannels; ++c) {
> -vol->volume[c] = st->volume[c];
> -}
> +SPICE_VERIFY(sizeof(vol->volume[0]) == sizeof(st->volume[0]));
> +memcpy(vol->volume, st->volume, sizeof(st->volume[0]) *
> st->volume_nchannels);
>  spice_marshall_SpiceMsgAudioVolume(m, vol);
>  
>  red_channel_client_begin_send_message(rcc);
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH v2 2/6] qxl-wddm-dod: Option to provide frequency data to the OS

[Yuri Benditovich]

Concentrate filling of signal info in single procedure.
Fill signal info with specific or default frequency data
according to the global flag of VSync support.
Note that the state of this flobal flag must be defined only
on driver startup (based on OS version or any other information
available at DriverEntry) and can't be changed later.

Signed-off-by: Yuri Benditovich 
---
 qxldod/QxlDod.cpp | 71 ---
 qxldod/QxlDod.h   |  2 ++
 2 files changed, 43 insertions(+), 30 deletions(-)

diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
index dca263a..4e40d4d 100755
--- a/qxldod/QxlDod.cpp
+++ b/qxldod/QxlDod.cpp
@@ -19,6 +19,10 @@
   (QXL_INTERRUPT_CURSOR) | \
   (QXL_INTERRUPT_IO_CMD))
 
+#define VSYNC_RATE  75
+
+BOOLEAN g_bSupportVSync;
+
 // BEGIN: Non-Paged Code
 
 // Bit is 1 from Idx to end of byte, with bit count starting at high order
@@ -826,6 +830,39 @@ NTSTATUS QxlDod::AddSingleSourceMode(_In_ CONST 
DXGK_VIDPNSOURCEMODESET_INTERFAC
 return STATUS_SUCCESS;
 }
 
+static VOID FillSignalInfo(D3DKMDT_VIDEO_SIGNAL_INFO& SignalInfo, const 
VIDEO_MODE_INFORMATION *pVideoModeInfo, LPCSTR caller)
+{
+PAGED_CODE();
+SignalInfo.VideoStandard = D3DKMDT_VSS_OTHER;
+SignalInfo.TotalSize.cx = pVideoModeInfo->VisScreenWidth;
+SignalInfo.TotalSize.cy = pVideoModeInfo->VisScreenHeight;
+SignalInfo.ActiveSize = SignalInfo.TotalSize;
+if (g_bSupportVSync)
+{
+UINT val;
+SignalInfo.VSyncFreq.Numerator = VSYNC_RATE;
+SignalInfo.VSyncFreq.Denominator = 1;
+val =
+SignalInfo.VSyncFreq.Numerator *
+pVideoModeInfo->VisScreenWidth *
+pVideoModeInfo->VisScreenHeight;
+SignalInfo.PixelRate = val;
+SignalInfo.HSyncFreq.Numerator = val / pVideoModeInfo->VisScreenHeight;
+SignalInfo.HSyncFreq.Denominator = 1;
+DbgPrint(TRACE_LEVEL_INFORMATION, ("by %s: filling with frequency data 
for %dx%d\n", caller, pVideoModeInfo->VisScreenWidth, 
pVideoModeInfo->VisScreenHeight));
+}
+else
+{
+SignalInfo.VSyncFreq.Numerator = D3DKMDT_FREQUENCY_NOTSPECIFIED;
+SignalInfo.VSyncFreq.Denominator = D3DKMDT_FREQUENCY_NOTSPECIFIED;
+SignalInfo.HSyncFreq.Numerator = D3DKMDT_FREQUENCY_NOTSPECIFIED;
+SignalInfo.HSyncFreq.Denominator = D3DKMDT_FREQUENCY_NOTSPECIFIED;
+SignalInfo.PixelRate = D3DKMDT_FREQUENCY_NOTSPECIFIED;
+DbgPrint(TRACE_LEVEL_INFORMATION, ("by %s: filling without frequency 
data for %dx%d\n", caller, pVideoModeInfo->VisScreenWidth, 
pVideoModeInfo->VisScreenHeight));
+}
+SignalInfo.ScanLineOrdering = D3DDDI_VSSLO_PROGRESSIVE;
+}
+
 // Add the current mode information (acquired from the POST frame buffer) as 
the target mode.
 NTSTATUS QxlDod::AddSingleTargetMode(_In_ CONST 
DXGK_VIDPNTARGETMODESET_INTERFACE* pVidPnTargetModeSetInterface,
D3DKMDT_HVIDPNTARGETMODESET 
hVidPnTargetModeSet,
@@ -849,16 +886,8 @@ NTSTATUS QxlDod::AddSingleTargetMode(_In_ CONST 
DXGK_VIDPNTARGETMODESET_INTERFAC
 DbgPrint(TRACE_LEVEL_ERROR, ("pfnCreateNewModeInfo failed with 
Status = 0x%X, hVidPnTargetModeSet = 0x%I64x", Status, hVidPnTargetModeSet));
 return Status;
 }
-pVidPnTargetModeInfo->VideoSignalInfo.VideoStandard = 
D3DKMDT_VSS_OTHER;
-pVidPnTargetModeInfo->VideoSignalInfo.TotalSize.cx = 
pModeInfo->VisScreenWidth;
-pVidPnTargetModeInfo->VideoSignalInfo.TotalSize.cy = 
pModeInfo->VisScreenHeight;
-pVidPnTargetModeInfo->VideoSignalInfo.ActiveSize = 
pVidPnTargetModeInfo->VideoSignalInfo.TotalSize;
-pVidPnTargetModeInfo->VideoSignalInfo.VSyncFreq.Numerator = 
D3DKMDT_FREQUENCY_NOTSPECIFIED;
-pVidPnTargetModeInfo->VideoSignalInfo.VSyncFreq.Denominator = 
D3DKMDT_FREQUENCY_NOTSPECIFIED;
-pVidPnTargetModeInfo->VideoSignalInfo.HSyncFreq.Numerator = 
D3DKMDT_FREQUENCY_NOTSPECIFIED;
-pVidPnTargetModeInfo->VideoSignalInfo.HSyncFreq.Denominator = 
D3DKMDT_FREQUENCY_NOTSPECIFIED;
-pVidPnTargetModeInfo->VideoSignalInfo.PixelRate = 
D3DKMDT_FREQUENCY_NOTSPECIFIED;
-pVidPnTargetModeInfo->VideoSignalInfo.ScanLineOrdering = 
D3DDDI_VSSLO_PROGRESSIVE;
+FillSignalInfo(pVidPnTargetModeInfo->VideoSignalInfo, pModeInfo, 
__FUNCTION__);
+
 // We add this as PREFERRED since it is the only supported target
 pVidPnTargetModeInfo->Preference = D3DKMDT_MP_NOTPREFERRED; // TODO: 
another logic for prefferred mode. Maybe the pinned source mode
 
@@ -900,16 +929,7 @@ NTSTATUS QxlDod::AddSingleMonitorMode(_In_ CONST 
DXGKARG_RECOMMENDMONITORMODES*
 pVbeModeInfo = 
m_pHWDevice->GetModeInfo(m_pHWDevice->GetCurrentModeIndex());
 
 // Since we don't know the real monitor timing information, just use the 
current display mode (from the POST device) with unknown 

[Spice-devel] [PATCH v2 6/6] qxl-wddm-dod: Preparation for VSync activation

[Yuri Benditovich]

Prepare all the procedures needed for VSync feature but
do not turn it on yet. Manual uncomment still required to
enable it.
Advanced users may enable VSync feature and report functional
problem with it. The driver is expected to pass all the formal
tests under HLK 1607 with device rev.3 and rev.4 but with
device rev.4 on setups with high load or long round-trip delay
video system may detect timeout during rendering operation
and disable the driver with error 43.

Signed-off-by: Yuri Benditovich 
---
 qxldod/driver.cpp | 52 
 qxldod/driver.h   | 15 +++
 2 files changed, 67 insertions(+)

diff --git a/qxldod/driver.cpp b/qxldod/driver.cpp
index dc84aa8..5bd57b4 100755
--- a/qxldod/driver.cpp
+++ b/qxldod/driver.cpp
@@ -32,6 +32,19 @@ DriverEntry(
 
 DbgPrint(TRACE_LEVEL_FATAL, ("---> KMDOD build on on %s %s\n", __DATE__, 
__TIME__));
 
+RTL_OSVERSIONINFOW versionInfo;
+versionInfo.dwOSVersionInfoSize = sizeof(versionInfo);
+
+
+RtlGetVersion();
+if (versionInfo.dwBuildNumber >= 14393 || versionInfo.dwBuildNumber <= 
9600)
+{
+//g_bSupportVSync = TRUE;
+}
+DbgPrint(TRACE_LEVEL_WARNING, ("VSync support %sabled for %d.%d.%d\n",
+g_bSupportVSync ? "en" : "dis",
+versionInfo.dwMajorVersion, versionInfo.dwMinorVersion, 
versionInfo.dwBuildNumber));
+
 // Initialize DDI function pointers and dxgkrnl
 KMDDOD_INITIALIZATION_DATA InitialData = {0};
 
@@ -67,6 +80,11 @@ DriverEntry(
 InitialData.DxgkDdiStopDeviceAndReleasePostDisplayOwnership = 
DodStopDeviceAndReleasePostDisplayOwnership;
 InitialData.DxgkDdiSystemDisplayEnable  = DodSystemDisplayEnable;
 InitialData.DxgkDdiSystemDisplayWrite   = DodSystemDisplayWrite;
+if (g_bSupportVSync)
+{
+InitialData.DxgkDdiControlInterrupt = DodControlInterrupt;
+InitialData.DxgkDdiGetScanLine = DodGetScanLine;
+}
 
 NTSTATUS Status = DxgkInitializeDisplayOnlyDriver(pDriverObject, 
pRegistryPath, );
 if (!NT_SUCCESS(Status))
@@ -559,6 +577,40 @@ DodQueryVidPnHWCapability(
 return pQxl->QueryVidPnHWCapability(pVidPnHWCaps);
 }
 
+NTSTATUS
+APIENTRY
+DodControlInterrupt(
+IN_CONST_HANDLE hAdapter,
+IN_CONST_DXGK_INTERRUPT_TYPEInterruptType,
+IN_BOOLEAN  EnableInterrupt
+)
+{
+PAGED_CODE();
+QxlDod* pQxl = reinterpret_cast(hAdapter);
+if (InterruptType == DXGK_INTERRUPT_DISPLAYONLY_VSYNC)
+{
+pQxl->EnableVsync(EnableInterrupt);
+return STATUS_SUCCESS;
+}
+return  STATUS_NOT_IMPLEMENTED;
+}
+
+NTSTATUS
+APIENTRY
+DodGetScanLine(
+IN_CONST_HANDLE hAdapter,
+INOUT_PDXGKARG_GETSCANLINE  pGetScanLine
+)
+{
+PAGED_CODE();
+// Currently we do not see any practical use case when this procedure is 
called
+// IDirectDraw has an interface for querying scan line
+// Leave it not implemented like remote desktop does
+// until we recognize use case for more intelligent implementation
+DbgPrint(TRACE_LEVEL_ERROR, ("<---> %s\n", __FUNCTION__));
+return STATUS_NOT_IMPLEMENTED;
+}
+
 //END: Paged Code
 #pragma code_seg(pop)
 
diff --git a/qxldod/driver.h b/qxldod/driver.h
index 97b9415..2dcbda4 100755
--- a/qxldod/driver.h
+++ b/qxldod/driver.h
@@ -217,6 +217,21 @@ DodSystemDisplayWrite(
 _In_  UINT  PositionX,
 _In_  UINT  PositionY);
 
+NTSTATUS
+APIENTRY
+DodControlInterrupt(
+IN_CONST_HANDLE hAdapter,
+IN_CONST_DXGK_INTERRUPT_TYPEInterruptType,
+IN_BOOLEAN  EnableInterrupt
+);
+
+NTSTATUS
+APIENTRY
+DodGetScanLine(
+IN_CONST_HANDLE hAdapter,
+INOUT_PDXGKARG_GETSCANLINE  pGetScanLine
+);
+
 #if DBG
 
 extern int nDebugLevel;
-- 
2.7.0.windows.1

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

[Spice-devel] [PATCH v2 0/6] Preparation to pass HLK tests

[Yuri Benditovich]

These patches address failures in Windows Hardware Lab Tests:
Requirement for EDID data
Requirement for refresh rate of 75/60 Hz

Now the driver contains data in EDID format and reports it to the system 

(requires manual uncomment)
In order to report refresh rate the driver reports support for
VSync control, reports refresh rate for each video mode and provides
periodic interrupt notifications to the system.

Advanced users may enable VSync control feature and report functional
problems with it. When VSync enabled, the driver is expected to pass
all the formal tests under HLK 1607 and HCK 2.1 with device rev.3 and rev.4
but with device rev.4 on setups with high load or long round-trip delay
video system may detect timeout during rendering operation
and disable the driver with error 43. Vsync control feature will be
enabled only after the problem is solved

Changes from V1:
* Fixed minor mistakes in EDID data, added output of edid-decode,
procedure that returns EDID data made bullet-proof
* Interrupt handling optimized and now VSync interrupt flows do
not touch device registers and do not wake host
* VSync feature is prepared but not enabled

Yuri Benditovich (6):
  qxl-wddm-dod: Return EDID data to the OS
  qxl-wddm-dod: Option to provide frequency data to the OS
  qxl-wddm-dod: Fix video modes enumeration
  qxl-wddm-dod: Simplify interrupt handling for rev4 device
  qxl-wddm-dod: Timer-based VSync interrupt indication
  qxl-wddm-dod: Preparation for VSync activation

 qxldod/QxlDod.cpp | 319 +-
 qxldod/QxlDod.h   |  20 +++-
 qxldod/driver.cpp |  52 +
 qxldod/driver.h   |  15 +++
 4 files changed, 326 insertions(+), 80 deletions(-)

-- 
2.7.0.windows.1

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

[Spice-devel] [PATCH v2 3/6] qxl-wddm-dod: Fix video modes enumeration

[Yuri Benditovich]

When the video mode is changed and then the driver disabled and
enabled, it did not enumerate available video modes with lower
resolution than current one. All modes starting from 1024x768
should be available regardless what is current resolution
on driver startup

Signed-off-by: Yuri Benditovich 
---
 qxldod/QxlDod.cpp | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
index 4e40d4d..2d2a022 100755
--- a/qxldod/QxlDod.cpp
+++ b/qxldod/QxlDod.cpp
@@ -2560,8 +2560,8 @@ NTSTATUS VgaDevice::GetModeList(DXGK_DISPLAY_INFORMATION* 
pDispInfo)
 
 DbgPrint(TRACE_LEVEL_INFORMATION, ("ModeTemp = 0x%X %dx%d@%d\n", 
ModeTemp, tmpModeInfo.XResolution, tmpModeInfo.YResolution, 
tmpModeInfo.BitsPerPixel));
 
-if (tmpModeInfo.XResolution >= Width &&
-tmpModeInfo.YResolution >= Height &&
+if (tmpModeInfo.XResolution >= MIN_WIDTH_SIZE &&
+tmpModeInfo.YResolution >= MIN_HEIGHT_SIZE &&
 tmpModeInfo.BitsPerPixel == BitsPerPixel &&
 tmpModeInfo.PhysBasePtr != 0)
 {
@@ -3186,8 +3186,8 @@ NTSTATUS QxlDevice::GetModeList(DXGK_DISPLAY_INFORMATION* 
pDispInfo)
 
 DbgPrint(TRACE_LEVEL_INFORMATION, ("%s: modes[%d] x_res = %d, y_res = 
%d, bits = %d BitsPerPixel = %d\n", __FUNCTION__, CurrentMode, 
tmpModeInfo->x_res, tmpModeInfo->y_res, tmpModeInfo->bits, BitsPerPixel));
 
-if (tmpModeInfo->x_res >= Width &&
-tmpModeInfo->y_res >= Height &&
+if (tmpModeInfo->x_res >= MIN_WIDTH_SIZE &&
+tmpModeInfo->y_res >= MIN_HEIGHT_SIZE &&
 tmpModeInfo->bits == QXL_BPP)
 {
 m_ModeNumbers[SuitableModeCount] = SuitableModeCount;
-- 
2.7.0.windows.1

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

[Spice-devel] [PATCH v2 5/6] qxl-wddm-dod: Timer-based VSync interrupt indication

[Yuri Benditovich]

In case the driver supports VSync control feature, it
maintains timer for VSync interrupt indication.
In further commits this timer will be started upon
class driver request. The interrupt notification and
related DPC do not access device registers.

Signed-off-by: Yuri Benditovich 
---
 qxldod/QxlDod.cpp | 82 +++
 qxldod/QxlDod.h   | 14 ++
 2 files changed, 96 insertions(+)

diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
index cd0a1e5..67a0a4e 100755
--- a/qxldod/QxlDod.cpp
+++ b/qxldod/QxlDod.cpp
@@ -82,6 +82,12 @@ QxlDod::QxlDod(_In_ DEVICE_OBJECT* pPhysicalDeviceObject) : 
m_pPhysicalDevice(pP
 RtlZeroMemory(m_CurrentModes, sizeof(m_CurrentModes));
 RtlZeroMemory(_PointerShape, sizeof(m_PointerShape));
 m_pHWDevice = NULL;
+
+KeInitializeDpc(_VsyncTimerDpc, VsyncTimerProcGate, this);
+KeInitializeTimer(_VsyncTimer);
+m_VsyncFiredCounter = 0;
+m_bVsyncEnabled = FALSE;
+
 DbgPrint(TRACE_LEVEL_INFORMATION, ("<--- %s\n", __FUNCTION__));
 }
 
@@ -198,6 +204,7 @@ NTSTATUS QxlDod::StopDevice(VOID)
 {
 PAGED_CODE();
 m_Flags.DriverStarted = FALSE;
+EnableVsync(FALSE);
 return STATUS_SUCCESS;
 }
 
@@ -518,6 +525,7 @@ NTSTATUS QxlDod::QueryAdapterInfo(_In_ CONST 
DXGKARG_QUERYADAPTERINFO* pQueryAda
 pDriverCaps->PointerCaps.Color = 1;
 
 pDriverCaps->SupportNonVGA = m_pHWDevice->IsBIOSCompatible();
+pDriverCaps->SchedulingCaps.VSyncPowerSaveAware = g_bSupportVSync;
 
 DbgPrint(TRACE_LEVEL_VERBOSE, ("<--- %s 1\n", __FUNCTION__));
 return STATUS_SUCCESS;
@@ -4813,6 +4821,14 @@ BOOLEAN QxlDevice::InterruptRoutine(_In_ 
PDXGKRNL_INTERFACE pDxgkInterface, _In_
 }
 
 QXL_NON_PAGED
+VOID QxlDevice::VSyncInterruptPostProcess(_In_ PDXGKRNL_INTERFACE 
pDxgkInterface)
+{
+if (!pDxgkInterface->DxgkCbQueueDpc(pDxgkInterface->DeviceHandle)) {
+DbgPrint(TRACE_LEVEL_WARNING, ("---> %s can't enqueue DPC, pending 
interrupts %X\n", __FUNCTION__, m_Pending));
+}
+}
+
+QXL_NON_PAGED
 VOID QxlDevice::DpcRoutine(PVOID ptr)
 {
 LONG intStatus = InterlockedExchange(_Pending, 0);
@@ -4915,3 +4931,69 @@ NTSTATUS 
HwDeviceInterface::AcquireDisplayInfo(DXGK_DISPLAY_INFORMATION& DispInf
 }
 return Status;
 }
+
+// Vga device does not generate interrupts
+QXL_NON_PAGED VOID VgaDevice::VSyncInterruptPostProcess(_In_ 
PDXGKRNL_INTERFACE pxface)
+{
+pxface->DxgkCbQueueDpc(pxface->DeviceHandle);
+}
+
+QXL_NON_PAGED VOID QxlDod::IndicateVSyncInterrupt()
+{
+DXGKARGCB_NOTIFY_INTERRUPT_DATA data = {};
+data.InterruptType = DXGK_INTERRUPT_DISPLAYONLY_VSYNC;
+m_DxgkInterface.DxgkCbNotifyInterrupt(m_DxgkInterface.DeviceHandle, );
+m_pHWDevice->VSyncInterruptPostProcess(_DxgkInterface);
+}
+
+QXL_NON_PAGED BOOLEAN QxlDod::VsyncTimerSynchRoutine(PVOID context)
+{
+QxlDod* pQxl = reinterpret_cast(context);
+pQxl->IndicateVSyncInterrupt();
+return FALSE;
+}
+
+QXL_NON_PAGED VOID QxlDod::VsyncTimerProc()
+{
+BOOLEAN bDummy;
+DbgPrint(TRACE_LEVEL_VERBOSE, ("<--- %s\n", __FUNCTION__));
+if (m_bVsyncEnabled && m_AdapterPowerState == PowerDeviceD0)
+{
+m_DxgkInterface.DxgkCbSynchronizeExecution(
+m_DxgkInterface.DeviceHandle,
+VsyncTimerSynchRoutine,
+this,
+0,
+
+);
+INCREMENT_VSYNC_COUNTER(_VsyncFiredCounter);
+}
+}
+
+VOID QxlDod::EnableVsync(BOOLEAN bEnable)
+{
+PAGED_CODE();
+if (g_bSupportVSync)
+{
+m_bVsyncEnabled = bEnable;
+if (!m_bVsyncEnabled)
+{
+DbgPrint(TRACE_LEVEL_WARNING, ("Disabled VSync(fired %d)\n", 
InterlockedExchange(_VsyncFiredCounter, 0)));
+KeCancelTimer(_VsyncTimer);
+}
+else
+{
+LARGE_INTEGER li;
+LONG period = 1000 / VSYNC_RATE;
+DbgPrint(TRACE_LEVEL_WARNING, ("Enabled VSync(fired %d)\n", 
m_VsyncFiredCounter));
+li.QuadPart = -1000 / VSYNC_RATE;
+KeSetTimerEx(_VsyncTimer, li, period, _VsyncTimerDpc);
+}
+}
+}
+
+QXL_NON_PAGED VOID QxlDod::VsyncTimerProcGate(_In_ _KDPC *dpc, _In_ PVOID 
context, _In_ PVOID arg1, _In_ PVOID arg2)
+{
+QxlDod* pQxl = reinterpret_cast(context);
+pQxl->VsyncTimerProc();
+}
diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h
index 53724e8..9cfb6de 100755
--- a/qxldod/QxlDod.h
+++ b/qxldod/QxlDod.h
@@ -239,6 +239,7 @@ public:
 QXL_NON_PAGED virtual BOOLEAN InterruptRoutine(_In_ PDXGKRNL_INTERFACE 
pDxgkInterface, _In_  ULONG MessageNumber) = 0;
 QXL_NON_PAGED virtual VOID DpcRoutine(PVOID) = 0;
 QXL_NON_PAGED virtual VOID ResetDevice(void) = 0;
+QXL_NON_PAGED virtual VOID VSyncInterruptPostProcess(_In_ 
PDXGKRNL_INTERFACE) = 0;
 virtual NTSTATUS AcquireFrameBuffer(CURRENT_BDD_MODE* pCurrentBddMode) { 
return STATUS_SUCCESS; }
 virtual NTSTATUS 

Re: [Spice-devel] [PATCH spice-server 2/4] Do not set TCP_NODELAY flag twice

[Frediano Ziglio]

> 
> On Wed, Feb 15, 2017 at 11:31:59AM +, Frediano Ziglio wrote:
> > TCP_NODELAY flag is set by default for all connection inside
> > reds.c so there's no need to set again for the single
> > client channel.
> 
> Would have been nice to keep "Do not set not blocking flag twice"
> together with this patch, and push them at the same time..
> 
> > 
> > Note that there are still some call to setsockopt to set this
> > option but in this case the flag can reset the flag.
> 
> 'some calls'
> the flag can reset the flag?
> 

Yes, confusing, what about:

"Note that there are still some call to setsockopt to change this
option."

shorter and (I hope) clearer.

> Acked-by: Christophe Fergeau 
> 
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  server/inputs-channel.c | 15 ---
> >  server/spicevmc.c   | 19 ---
> >  2 files changed, 34 deletions(-)
> > 
> > diff --git a/server/inputs-channel.c b/server/inputs-channel.c
> > index 3672d7e..223f46f 100644
> > --- a/server/inputs-channel.c
> > +++ b/server/inputs-channel.c
> > @@ -19,11 +19,7 @@
> >  #include 
> >  #endif
> >  
> > -#include  // IPPROTO_TCP
> > -#include  // TCP_NODELAY
> > -#include 
> >  #include  // NULL
> > -#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -490,17 +486,6 @@ static void inputs_pipe_add_init(RedChannelClient
> > *rcc)
> >  
> >  static int inputs_channel_config_socket(RedChannelClient *rcc)
> >  {
> > -int delay_val = 1;
> > -RedsStream *stream = red_channel_client_get_stream(rcc);
> > -
> > -if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY,
> > -_val, sizeof(delay_val)) == -1) {
> > -if (errno != ENOTSUP && errno != ENOPROTOOPT) {
> > -spice_printerr("setsockopt failed, %s", strerror(errno));
> > -return FALSE;
> > -}
> > -}
> > -
> >  return TRUE;
> >  }
> >  
> > diff --git a/server/spicevmc.c b/server/spicevmc.c
> > index 4c9f442..4b46e54 100644
> > --- a/server/spicevmc.c
> > +++ b/server/spicevmc.c
> > @@ -23,10 +23,7 @@
> >  #endif
> >  
> >  #include 
> > -#include 
> >  #include 
> > -#include  // IPPROTO_TCP
> > -#include  // TCP_NODELAY
> >  #ifdef USE_LZ4
> >  #include 
> >  #endif
> > @@ -445,22 +442,6 @@ static void
> > spicevmc_char_dev_remove_client(RedCharDevice *self,
> >  
> >  static int spicevmc_red_channel_client_config_socket(RedChannelClient
> >  *rcc)
> >  {
> > -int delay_val = 1;
> > -RedsStream *stream = red_channel_client_get_stream(rcc);
> > -RedChannel *channel = red_channel_client_get_channel(rcc);
> > -uint32_t type;
> > -
> > -g_object_get(channel, "channel-type", , NULL);
> > -if (type == SPICE_CHANNEL_USBREDIR) {
> > -if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY,
> > -_val, sizeof(delay_val)) != 0) {
> > -if (errno != ENOTSUP && errno != ENOPROTOOPT) {
> > -spice_printerr("setsockopt failed, %s", strerror(errno));
> > -return FALSE;
> > -}
> > -}
> > -}
> > -
> >  return TRUE;
> >  }
> >  

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

Re: [Spice-devel] [spice-server v2] gstreamer: Add #ifdef around zero_copy()

[Pavel Grunt]

On Wed, 2017-02-15 at 12:29 +0100, Christophe Fergeau wrote:
> Since c3d237 "gstreamer: Avoid memory copy if strides are different"
> is
> only needed when zero copy is disabled. This moves the function
> definition to an already existing #ifdef block.

Ack,
Pavel
> ---
> 
> Changes since v1:
> - do not add redundant #ifdef block
> 
>  server/gstreamer-encoder.c | 61 +++--
> -
>  1 file changed, 31 insertions(+), 30 deletions(-)
> 
> diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> index dd2926d..991eb51 100644
> --- a/server/gstreamer-encoder.c
> +++ b/server/gstreamer-encoder.c
> @@ -1140,36 +1140,6 @@ static int is_chunk_stride_aligned(const
> SpiceBitmap *bitmap, uint32_t index)
>  return TRUE;
>  }
>  
> -/* A helper for push_raw_frame() */
> -static inline int line_copy(SpiceGstEncoder *encoder, const
> SpiceBitmap *bitmap,
> -uint32_t chunk_offset, uint32_t
> stream_stride,
> -uint32_t height, uint8_t *buffer)
> -{
> - uint8_t *dst = buffer;
> - SpiceChunks *chunks = bitmap->data;
> - uint32_t chunk_index = 0;
> - for (int l = 0; l < height; l++) {
> - /* We may have to move forward by more than one chunk the
> first
> -  * time around. This also protects us against 0-byte
> chunks.
> -  */
> - while (chunk_offset >= chunks->chunk[chunk_index].len) {
> - if (!is_chunk_stride_aligned(bitmap, chunk_index)) {
> - return FALSE;
> - }
> - chunk_offset -= chunks->chunk[chunk_index].len;
> - chunk_index++;
> - }
> -
> - /* Copy the line */
> - uint8_t *src = chunks->chunk[chunk_index].data +
> chunk_offset;
> - memcpy(dst, src, stream_stride);
> - dst += stream_stride;
> - chunk_offset += bitmap->stride;
> - }
> - spice_return_val_if_fail(dst - buffer == stream_stride *
> height, FALSE);
> - return TRUE;
> -}
> -
>  #ifdef DO_ZERO_COPY
>  typedef struct {
>  gint refs;
> @@ -1265,6 +1235,37 @@ static void
> clear_zero_copy_queue(SpiceGstEncoder *encoder, gboolean unref_queue
>  {
>  /* Nothing to do */
>  }
> +
> +/* A helper for push_raw_frame() */
> +static inline int line_copy(SpiceGstEncoder *encoder, const
> SpiceBitmap *bitmap,
> +uint32_t chunk_offset, uint32_t
> stream_stride,
> +uint32_t height, uint8_t *buffer)
> +{
> + uint8_t *dst = buffer;
> + SpiceChunks *chunks = bitmap->data;
> + uint32_t chunk_index = 0;
> + for (int l = 0; l < height; l++) {
> + /* We may have to move forward by more than one chunk the
> first
> +  * time around. This also protects us against 0-byte
> chunks.
> +  */
> + while (chunk_offset >= chunks->chunk[chunk_index].len) {
> + if (!is_chunk_stride_aligned(bitmap, chunk_index)) {
> + return FALSE;
> + }
> + chunk_offset -= chunks->chunk[chunk_index].len;
> + chunk_index++;
> + }
> +
> + /* Copy the line */
> + uint8_t *src = chunks->chunk[chunk_index].data +
> chunk_offset;
> + memcpy(dst, src, stream_stride);
> + dst += stream_stride;
> + chunk_offset += bitmap->stride;
> + }
> + spice_return_val_if_fail(dst - buffer == stream_stride *
> height, FALSE);
> + return TRUE;
> +}
> +
>  #endif
>  
>  /* A helper for push_raw_frame() */
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-server 1/4] Clear "msg" pointers after releasing

[Christophe Fergeau]

On Wed, Feb 15, 2017 at 11:31:58AM +, Frediano Ziglio wrote:
> Avoid possible dangling pointers.


Acked-by: Christophe Fergeau 

> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/red-channel-client.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> index 9ab22e4..32db186 100644
> --- a/server/red-channel-client.c
> +++ b/server/red-channel-client.c
> @@ -1277,6 +1277,7 @@ static void 
> red_channel_client_handle_incoming(RedChannelClient *rcc)
>  if (bytes_read == -1) {
>  red_channel_client_release_msg_buf(rcc, msg_type, msg_size,
> buffer->msg);
> +buffer->msg = NULL;
>  red_channel_client_disconnect(rcc);
>  return;
>  }
> @@ -1296,6 +1297,7 @@ static void 
> red_channel_client_handle_incoming(RedChannelClient *rcc)
>  red_channel_client_release_msg_buf(rcc,
> msg_type, msg_size,
> buffer->msg);
> +buffer->msg = NULL;
>  red_channel_client_disconnect(rcc);
>  return;
>  }
> -- 
> 2.9.3
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [spice-server v4 5/5] Change some spice_printerr() to spice_debug()

[Frediano Ziglio]

> 
> Some debug messages were using spice_printerr(), which should be limited
> to actual errors.
> 
> Signed-off-by: Christophe Fergeau 
> ---
>  server/inputs-channel.c  |  4 ++--
>  server/main-channel-client.c | 28 ++--
>  server/main-channel.c| 10 +-
>  server/red-channel-client.c  |  4 ++--
>  server/red-client.c  |  4 ++--
>  server/red-qxl.c | 12 ++--
>  server/smartcard.c   |  2 +-
>  server/spicevmc.c|  2 +-
>  8 files changed, 33 insertions(+), 33 deletions(-)
> 
> diff --git a/server/inputs-channel.c b/server/inputs-channel.c
> index 3672d7e..89fb90e 100644
> --- a/server/inputs-channel.c
> +++ b/server/inputs-channel.c
> @@ -516,7 +516,7 @@ static void inputs_connect(RedChannel *channel, RedClient
> *client,
>  "keyboard channel is insecure");
>  }
>  
> -spice_printerr("inputs channel client create");
> +spice_debug("inputs channel client create");
>  rcc = inputs_channel_client_create(channel, client, stream, FALSE,
> num_common_caps, common_caps,
> num_caps, caps);

agreed

> @@ -714,7 +714,7 @@ int inputs_channel_has_tablet(InputsChannel *inputs)
>  
>  void inputs_channel_detach_tablet(InputsChannel *inputs, SpiceTabletInstance
>  *tablet)
>  {
> -spice_printerr("");
> +spice_debug("");
>  inputs->tablet = NULL;
>  }
>  

I vote to remove

> diff --git a/server/main-channel-client.c b/server/main-channel-client.c
> index 6aace29..44e0b63 100644
> --- a/server/main-channel-client.c
> +++ b/server/main-channel-client.c
> @@ -445,7 +445,7 @@ void
> main_channel_client_handle_migrate_connected(MainChannelClient *mcc,
>int seamless)
>  {
>  RedClient *client =
>  red_channel_client_get_client(RED_CHANNEL_CLIENT(mcc));
> -spice_printerr("client %p connected: %d seamless %d", client, success,
> seamless);
> +spice_debug("client %p connected: %d seamless %d", client, success,
> seamless);
>  if (mcc->priv->mig_wait_connect) {
>  RedChannel *channel =
>  red_channel_client_get_channel(RED_CHANNEL_CLIENT(mcc));
>  MainChannel *main_channel = MAIN_CHANNEL(channel);
> @@ -455,7 +455,7 @@ void
> main_channel_client_handle_migrate_connected(MainChannelClient *mcc,
>  main_channel_on_migrate_connected(main_channel, success, seamless);
>  } else {
>  if (success) {
> -spice_printerr("client %p MIGRATE_CANCEL", client);
> +spice_debug("client %p MIGRATE_CANCEL", client);
>  red_channel_client_pipe_add_empty_msg(RED_CHANNEL_CLIENT(mcc),
>
> SPICE_MSG_MAIN_MIGRATE_CANCEL);
>  }
> @@ -517,11 +517,11 @@ void main_channel_client_handle_pong(MainChannelClient
> *mcc, SpiceMsgPing *ping,
>  mcc->priv->bitrate_per_sec = (uint64_t)(NET_TEST_BYTES * 8) *
>  100
>  / (roundtrip - mcc->priv->latency);
>  mcc->priv->net_test_stage = NET_TEST_STAGE_COMPLETE;
> -spice_printerr("net test: latency %f ms, bitrate %"PRIu64" bps (%f
> Mbps)%s",
> -   (double)mcc->priv->latency / 1000,
> -   mcc->priv->bitrate_per_sec,
> -   (double)mcc->priv->bitrate_per_sec / 1024 / 1024,
> -   main_channel_client_is_low_bandwidth(mcc) ? " LOW
> BANDWIDTH" : "");
> +spice_debug("net test: latency %f ms, bitrate %"PRIu64" bps (%f
> Mbps)%s",
> +(double)mcc->priv->latency / 1000,
> +mcc->priv->bitrate_per_sec,
> +(double)mcc->priv->bitrate_per_sec / 1024 / 1024,
> +main_channel_client_is_low_bandwidth(mcc) ? " LOW
> BANDWIDTH" : "");
>  
> red_channel_client_start_connectivity_monitoring(RED_CHANNEL_CLIENT(mcc),
>   
> CLIENT_CONNECTIVITY_TIMEOUT);
>  break;

Maybe info ?

> @@ -588,18 +588,18 @@ gboolean
> main_channel_client_migrate_src_complete(MainChannelClient *mcc,
> 
> SPICE_MAIN_CAP_SEMI_SEAMLESS_MIGRATE);
>  if (semi_seamless_support && mcc->priv->mig_connect_ok) {
>  if (success) {
> -spice_printerr("client %p MIGRATE_END", client);
> +spice_debug("client %p MIGRATE_END", client);
>  red_channel_client_pipe_add_empty_msg(rcc,
>
> SPICE_MSG_MAIN_MIGRATE_END);
>  ret = TRUE;
>  } else {
> -spice_printerr("client %p MIGRATE_CANCEL", client);
> +spice_debug("client %p MIGRATE_CANCEL", client);
>  red_channel_client_pipe_add_empty_msg(rcc,
>   

Re: [Spice-devel] [PATCH spice-server 2/4] Do not set TCP_NODELAY flag twice

[Christophe Fergeau]

On Wed, Feb 15, 2017 at 11:31:59AM +, Frediano Ziglio wrote:
> TCP_NODELAY flag is set by default for all connection inside
> reds.c so there's no need to set again for the single
> client channel.

Would have been nice to keep "Do not set not blocking flag twice"
together with this patch, and push them at the same time..

> 
> Note that there are still some call to setsockopt to set this
> option but in this case the flag can reset the flag.

'some calls'
the flag can reset the flag?

Acked-by: Christophe Fergeau 

> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/inputs-channel.c | 15 ---
>  server/spicevmc.c   | 19 ---
>  2 files changed, 34 deletions(-)
> 
> diff --git a/server/inputs-channel.c b/server/inputs-channel.c
> index 3672d7e..223f46f 100644
> --- a/server/inputs-channel.c
> +++ b/server/inputs-channel.c
> @@ -19,11 +19,7 @@
>  #include 
>  #endif
>  
> -#include  // IPPROTO_TCP
> -#include  // TCP_NODELAY
> -#include 
>  #include  // NULL
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -490,17 +486,6 @@ static void inputs_pipe_add_init(RedChannelClient *rcc)
>  
>  static int inputs_channel_config_socket(RedChannelClient *rcc)
>  {
> -int delay_val = 1;
> -RedsStream *stream = red_channel_client_get_stream(rcc);
> -
> -if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY,
> -_val, sizeof(delay_val)) == -1) {
> -if (errno != ENOTSUP && errno != ENOPROTOOPT) {
> -spice_printerr("setsockopt failed, %s", strerror(errno));
> -return FALSE;
> -}
> -}
> -
>  return TRUE;
>  }
>  
> diff --git a/server/spicevmc.c b/server/spicevmc.c
> index 4c9f442..4b46e54 100644
> --- a/server/spicevmc.c
> +++ b/server/spicevmc.c
> @@ -23,10 +23,7 @@
>  #endif
>  
>  #include 
> -#include 
>  #include 
> -#include  // IPPROTO_TCP
> -#include  // TCP_NODELAY
>  #ifdef USE_LZ4
>  #include 
>  #endif
> @@ -445,22 +442,6 @@ static void 
> spicevmc_char_dev_remove_client(RedCharDevice *self,
>  
>  static int spicevmc_red_channel_client_config_socket(RedChannelClient *rcc)
>  {
> -int delay_val = 1;
> -RedsStream *stream = red_channel_client_get_stream(rcc);
> -RedChannel *channel = red_channel_client_get_channel(rcc);
> -uint32_t type;
> -
> -g_object_get(channel, "channel-type", , NULL);
> -if (type == SPICE_CHANNEL_USBREDIR) {
> -if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY,
> -_val, sizeof(delay_val)) != 0) {
> -if (errno != ENOTSUP && errno != ENOPROTOOPT) {
> -spice_printerr("setsockopt failed, %s", strerror(errno));
> -return FALSE;
> -}
> -}
> -}
> -
>  return TRUE;
>  }
>  
> -- 
> 2.9.3
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [PATCH spice-server 4/4] Make RedChannel::config_socket() optional

[Christophe Fergeau]

Acked-by: Christophe Fergeau 

Christophe

On Wed, Feb 15, 2017 at 11:32:01AM +, Frediano Ziglio wrote:
> Most channels don't need to do specific settings for the client socket
> so avoid the need to do this setting making easier to setup the client
> channnel.
> 
> Some improvements and commit subject suggested by Christophe Fergeau.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/inputs-channel.c | 6 --
>  server/main-channel.c   | 6 --
>  server/red-channel.c| 6 +-
>  server/smartcard.c  | 6 --
>  server/spicevmc.c   | 6 --
>  5 files changed, 5 insertions(+), 25 deletions(-)
> 
> diff --git a/server/inputs-channel.c b/server/inputs-channel.c
> index 223f46f..ed92e71 100644
> --- a/server/inputs-channel.c
> +++ b/server/inputs-channel.c
> @@ -484,11 +484,6 @@ static void inputs_pipe_add_init(RedChannelClient *rcc)
>  red_channel_client_pipe_add_push(rcc, >base);
>  }
>  
> -static int inputs_channel_config_socket(RedChannelClient *rcc)
> -{
> -return TRUE;
> -}
> -
>  static void inputs_connect(RedChannel *channel, RedClient *client,
> RedsStream *stream, int migration,
> int num_common_caps, uint32_t *common_caps,
> @@ -634,7 +629,6 @@ inputs_channel_class_init(InputsChannelClass *klass)
>  channel_class->handle_message = inputs_channel_handle_message;
>  
>  /* channel callbacks */
> -channel_class->config_socket = inputs_channel_config_socket;
>  channel_class->on_disconnect = inputs_channel_on_disconnect;
>  channel_class->send_item = inputs_channel_send_item;
>  channel_class->alloc_recv_buf = inputs_channel_alloc_msg_rcv_buf;
> diff --git a/server/main-channel.c b/server/main-channel.c
> index 2d32444..3a6e6cd 100644
> --- a/server/main-channel.c
> +++ b/server/main-channel.c
> @@ -277,11 +277,6 @@ static void 
> main_channel_release_msg_rcv_buf(RedChannelClient *rcc,
>  }
>  }
>  
> -static int main_channel_config_socket(RedChannelClient *rcc)
> -{
> -return TRUE;
> -}
> -
>  static int main_channel_handle_migrate_flush_mark(RedChannelClient *rcc)
>  {
>  RedChannel *channel = red_channel_client_get_channel(rcc);
> @@ -355,7 +350,6 @@ main_channel_class_init(MainChannelClass *klass)
>  channel_class->handle_message = main_channel_handle_message;
>  
>  /* channel callbacks */
> -channel_class->config_socket = main_channel_config_socket;
>  channel_class->on_disconnect = main_channel_client_on_disconnect;
>  channel_class->send_item = main_channel_client_send_item;
>  channel_class->alloc_recv_buf = main_channel_alloc_msg_rcv_buf;
> diff --git a/server/red-channel.c b/server/red-channel.c
> index 2f9173b..67a570d 100644
> --- a/server/red-channel.c
> +++ b/server/red-channel.c
> @@ -210,7 +210,7 @@ red_channel_constructed(GObject *object)
>  
>  G_OBJECT_CLASS(red_channel_parent_class)->constructed(object);
>  
> -spice_assert(klass->config_socket && klass->on_disconnect &&
> +spice_assert(klass->on_disconnect &&
>   klass->alloc_recv_buf && klass->release_recv_buf);
>  spice_assert(klass->handle_migrate_data ||
>   !(self->priv->migration_flags & 
> SPICE_MIGRATE_NEED_DATA_TRANSFER));
> @@ -732,6 +732,10 @@ int red_channel_config_socket(RedChannel *self, 
> RedChannelClient *rcc)
>  {
>  RedChannelClass *klass = RED_CHANNEL_GET_CLASS(self);
>  
> +if (!klass->config_socket) {
> +return TRUE;
> +}
> +
>  return klass->config_socket(rcc);
>  }
>  
> diff --git a/server/smartcard.c b/server/smartcard.c
> index a7cc614..8f12fd9 100644
> --- a/server/smartcard.c
> +++ b/server/smartcard.c
> @@ -394,11 +394,6 @@ void 
> smartcard_char_device_detach_client(RedCharDeviceSmartcard *smartcard,
>  smartcard->priv->scc = NULL;
>  }
>  
> -static int smartcard_channel_client_config_socket(RedChannelClient *rcc)
> -{
> -return TRUE;
> -}
> -
>  SmartCardChannelClient* 
> smartcard_char_device_get_client(RedCharDeviceSmartcard *smartcard)
>  {
>  return smartcard->priv->scc;
> @@ -585,7 +580,6 @@ red_smartcard_channel_class_init(RedSmartcardChannelClass 
> *klass)
>  
>  channel_class->handle_message = smartcard_channel_client_handle_message,
>  
> -channel_class->config_socket = smartcard_channel_client_config_socket;
>  channel_class->on_disconnect = smartcard_channel_client_on_disconnect;
>  channel_class->send_item = smartcard_channel_send_item;
>  channel_class->alloc_recv_buf = 
> smartcard_channel_client_alloc_msg_rcv_buf;
> diff --git a/server/spicevmc.c b/server/spicevmc.c
> index 4b46e54..e705bc7 100644
> --- a/server/spicevmc.c
> +++ b/server/spicevmc.c
> @@ -440,11 +440,6 @@ static void 
> spicevmc_char_dev_remove_client(RedCharDevice *self,
>  red_channel_client_shutdown(channel->rcc);
>  }
>  
> -static int spicevmc_red_channel_client_config_socket(RedChannelClient *rcc)
> -{
> -

Re: [Spice-devel] [PATCH spice-server 3/4] reds-stream: Introduce reds_stream_set_no_delay() helper

[Frediano Ziglio]

> 
> From: Christophe Fergeau 
> 
> The code to enable/disable on a TCP socket is duplicated in multiple
> places in the code base, this commit replaces this duplicated code with
> a helper in RedsStream.
> ---
>  server/common-graphics-channel.c | 13 ++---
>  server/red-channel-client.c  | 17 ++---
>  server/reds-stream.c | 16 
>  server/reds-stream.h |  1 +
>  server/reds.c|  8 +---
>  server/sound.c   |  8 +---
>  6 files changed, 23 insertions(+), 40 deletions(-)
> 
> diff --git a/server/common-graphics-channel.c
> b/server/common-graphics-channel.c
> index bad6e8c..a076111 100644
> --- a/server/common-graphics-channel.c
> +++ b/server/common-graphics-channel.c
> @@ -19,9 +19,6 @@
>  #endif
>  
>  #include 
> -#include 
> -#include 
> -#include 
>  
>  #include "common-graphics-channel.h"
>  #include "dcc.h"
> @@ -117,24 +114,18 @@ int common_channel_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);
> -int delay_val;
>  gboolean is_low_bandwidth;
>  
>  // TODO - this should be dynamic, not one time at channel creation
>  is_low_bandwidth = main_channel_client_is_low_bandwidth(mcc);
> -delay_val = is_low_bandwidth ? 0 : 1;
>  /* FIXME: Using Nagle's Algorithm can lead to apparent delays, depending
>   * on the delayed ack timeout on the other side.
>   * Instead of using Nagle's, we need to implement message buffering on
>   * the application level.
>   * see: http://www.stuartcheshire.org/papers/NagleDelayedAck/
>   */
> -if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY, _val,
> -   sizeof(delay_val)) == -1) {
> -if (errno != ENOTSUP) {
> -spice_warning("setsockopt failed, %s", strerror(errno));
> -}
> -}
> +reds_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,
>  is_low_bandwidth ?
> diff --git a/server/red-channel-client.c b/server/red-channel-client.c
> index 32db186..f79edfc 100644
> --- a/server/red-channel-client.c
> +++ b/server/red-channel-client.c
> @@ -584,13 +584,7 @@ static void
> red_channel_client_send_ping(RedChannelClient *rcc)
>  }  else {
>  rcc->priv->latency_monitor.tcp_nodelay = delay_val;
>  if (!delay_val) {
> -delay_val = 1;
> -if (setsockopt(rcc->priv->stream->socket, IPPROTO_TCP,
> TCP_NODELAY, _val,
> -   sizeof(delay_val)) == -1) {
> -   if (errno != ENOTSUP) {
> -spice_warning("setsockopt failed, %s",
> strerror(errno));
> -}
> -}
> +reds_stream_set_no_delay(rcc->priv->stream, TRUE);
>  }
>  }
>  }
> @@ -1414,14 +1408,7 @@ static void
> red_channel_client_handle_pong(RedChannelClient *rcc, SpiceMsgPing *
>  
>  /* set TCP_NODELAY=0, in case we reverted it for the test*/
>  if (!rcc->priv->latency_monitor.tcp_nodelay) {
> -int delay_val = 0;
> -
> -if (setsockopt(rcc->priv->stream->socket, IPPROTO_TCP, TCP_NODELAY,
> _val,
> -   sizeof(delay_val)) == -1) {
> -if (errno != ENOTSUP) {
> -spice_warning("setsockopt failed, %s", strerror(errno));
> -}
> -}
> +reds_stream_set_no_delay(rcc->priv->stream, FALSE);
>  }
>  
>  /*
> diff --git a/server/reds-stream.c b/server/reds-stream.c
> index d0dadb9..f1952cc 100644
> --- a/server/reds-stream.c
> +++ b/server/reds-stream.c
> @@ -21,6 +21,7 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -256,6 +257,21 @@ int reds_stream_is_plain_unix(const RedsStream *s)
>  
>  }
>  
> +bool reds_stream_set_no_delay(RedsStream *stream, bool no_delay)
> +{
> +int delay_val = no_delay;
> +
> +if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY,
> +   _val, sizeof(delay_val)) != 0) {
> +if (errno != ENOTSUP && errno != ENOPROTOOPT) {
> +spice_printerr("setsockopt failed, %s", strerror(errno));
> +return FALSE;
> +}
> +}
> +
> +return TRUE;
> +}
> +
>  int reds_stream_send_msgfd(RedsStream *stream, int fd)
>  {
>  struct msghdr msgh = { 0, };
> diff --git a/server/reds-stream.h b/server/reds-stream.h
> index a8d1736..c76014b 100644
> --- a/server/reds-stream.h
> +++ b/server/reds-stream.h
> @@ -74,6 +74,7 @@ int reds_stream_enable_ssl(RedsStream *stream, SSL_CTX
> *ctx);
>  void reds_stream_set_info_flag(RedsStream *stream, unsigned int flag);
>  int reds_stream_get_family(const RedsStream 

[Spice-devel] [spice-server v4 5/5] Change some spice_printerr() to spice_debug()

[Christophe Fergeau]

Some debug messages were using spice_printerr(), which should be limited
to actual errors.

Signed-off-by: Christophe Fergeau 
---
 server/inputs-channel.c  |  4 ++--
 server/main-channel-client.c | 28 ++--
 server/main-channel.c| 10 +-
 server/red-channel-client.c  |  4 ++--
 server/red-client.c  |  4 ++--
 server/red-qxl.c | 12 ++--
 server/smartcard.c   |  2 +-
 server/spicevmc.c|  2 +-
 8 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/server/inputs-channel.c b/server/inputs-channel.c
index 3672d7e..89fb90e 100644
--- a/server/inputs-channel.c
+++ b/server/inputs-channel.c
@@ -516,7 +516,7 @@ static void inputs_connect(RedChannel *channel, RedClient 
*client,
 "keyboard channel is insecure");
 }
 
-spice_printerr("inputs channel client create");
+spice_debug("inputs channel client create");
 rcc = inputs_channel_client_create(channel, client, stream, FALSE,
num_common_caps, common_caps,
num_caps, caps);
@@ -714,7 +714,7 @@ int inputs_channel_has_tablet(InputsChannel *inputs)
 
 void inputs_channel_detach_tablet(InputsChannel *inputs, SpiceTabletInstance 
*tablet)
 {
-spice_printerr("");
+spice_debug("");
 inputs->tablet = NULL;
 }
 
diff --git a/server/main-channel-client.c b/server/main-channel-client.c
index 6aace29..44e0b63 100644
--- a/server/main-channel-client.c
+++ b/server/main-channel-client.c
@@ -445,7 +445,7 @@ void 
main_channel_client_handle_migrate_connected(MainChannelClient *mcc,
   int seamless)
 {
 RedClient *client = red_channel_client_get_client(RED_CHANNEL_CLIENT(mcc));
-spice_printerr("client %p connected: %d seamless %d", client, success, 
seamless);
+spice_debug("client %p connected: %d seamless %d", client, success, 
seamless);
 if (mcc->priv->mig_wait_connect) {
 RedChannel *channel = 
red_channel_client_get_channel(RED_CHANNEL_CLIENT(mcc));
 MainChannel *main_channel = MAIN_CHANNEL(channel);
@@ -455,7 +455,7 @@ void 
main_channel_client_handle_migrate_connected(MainChannelClient *mcc,
 main_channel_on_migrate_connected(main_channel, success, seamless);
 } else {
 if (success) {
-spice_printerr("client %p MIGRATE_CANCEL", client);
+spice_debug("client %p MIGRATE_CANCEL", client);
 red_channel_client_pipe_add_empty_msg(RED_CHANNEL_CLIENT(mcc),
   
SPICE_MSG_MAIN_MIGRATE_CANCEL);
 }
@@ -517,11 +517,11 @@ void main_channel_client_handle_pong(MainChannelClient 
*mcc, SpiceMsgPing *ping,
 mcc->priv->bitrate_per_sec = (uint64_t)(NET_TEST_BYTES * 8) * 100
 / (roundtrip - mcc->priv->latency);
 mcc->priv->net_test_stage = NET_TEST_STAGE_COMPLETE;
-spice_printerr("net test: latency %f ms, bitrate %"PRIu64" bps (%f 
Mbps)%s",
-   (double)mcc->priv->latency / 1000,
-   mcc->priv->bitrate_per_sec,
-   (double)mcc->priv->bitrate_per_sec / 1024 / 1024,
-   main_channel_client_is_low_bandwidth(mcc) ? " LOW 
BANDWIDTH" : "");
+spice_debug("net test: latency %f ms, bitrate %"PRIu64" bps (%f 
Mbps)%s",
+(double)mcc->priv->latency / 1000,
+mcc->priv->bitrate_per_sec,
+(double)mcc->priv->bitrate_per_sec / 1024 / 1024,
+main_channel_client_is_low_bandwidth(mcc) ? " LOW 
BANDWIDTH" : "");
 
red_channel_client_start_connectivity_monitoring(RED_CHANNEL_CLIENT(mcc),
  
CLIENT_CONNECTIVITY_TIMEOUT);
 break;
@@ -588,18 +588,18 @@ gboolean 
main_channel_client_migrate_src_complete(MainChannelClient *mcc,

SPICE_MAIN_CAP_SEMI_SEAMLESS_MIGRATE);
 if (semi_seamless_support && mcc->priv->mig_connect_ok) {
 if (success) {
-spice_printerr("client %p MIGRATE_END", client);
+spice_debug("client %p MIGRATE_END", client);
 red_channel_client_pipe_add_empty_msg(rcc,
   SPICE_MSG_MAIN_MIGRATE_END);
 ret = TRUE;
 } else {
-spice_printerr("client %p MIGRATE_CANCEL", client);
+spice_debug("client %p MIGRATE_CANCEL", client);
 red_channel_client_pipe_add_empty_msg(rcc,
   
SPICE_MSG_MAIN_MIGRATE_CANCEL);
 }
 } else {
 if (success) {
-spice_printerr("client %p SWITCH_HOST", client);
+spice_debug("client %p SWITCH_HOST", client);
 red_channel_client_pipe_add_type(rcc,
   

[Spice-devel] [PATCH spice-server 2/4] Do not set TCP_NODELAY flag twice

[Frediano Ziglio]

TCP_NODELAY flag is set by default for all connection inside
reds.c so there's no need to set again for the single
client channel.

Note that there are still some call to setsockopt to set this
option but in this case the flag can reset the flag.

Signed-off-by: Frediano Ziglio 
---
 server/inputs-channel.c | 15 ---
 server/spicevmc.c   | 19 ---
 2 files changed, 34 deletions(-)

diff --git a/server/inputs-channel.c b/server/inputs-channel.c
index 3672d7e..223f46f 100644
--- a/server/inputs-channel.c
+++ b/server/inputs-channel.c
@@ -19,11 +19,7 @@
 #include 
 #endif
 
-#include  // IPPROTO_TCP
-#include  // TCP_NODELAY
-#include 
 #include  // NULL
-#include 
 #include 
 #include 
 #include 
@@ -490,17 +486,6 @@ static void inputs_pipe_add_init(RedChannelClient *rcc)
 
 static int inputs_channel_config_socket(RedChannelClient *rcc)
 {
-int delay_val = 1;
-RedsStream *stream = red_channel_client_get_stream(rcc);
-
-if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY,
-_val, sizeof(delay_val)) == -1) {
-if (errno != ENOTSUP && errno != ENOPROTOOPT) {
-spice_printerr("setsockopt failed, %s", strerror(errno));
-return FALSE;
-}
-}
-
 return TRUE;
 }
 
diff --git a/server/spicevmc.c b/server/spicevmc.c
index 4c9f442..4b46e54 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -23,10 +23,7 @@
 #endif
 
 #include 
-#include 
 #include 
-#include  // IPPROTO_TCP
-#include  // TCP_NODELAY
 #ifdef USE_LZ4
 #include 
 #endif
@@ -445,22 +442,6 @@ static void spicevmc_char_dev_remove_client(RedCharDevice 
*self,
 
 static int spicevmc_red_channel_client_config_socket(RedChannelClient *rcc)
 {
-int delay_val = 1;
-RedsStream *stream = red_channel_client_get_stream(rcc);
-RedChannel *channel = red_channel_client_get_channel(rcc);
-uint32_t type;
-
-g_object_get(channel, "channel-type", , NULL);
-if (type == SPICE_CHANNEL_USBREDIR) {
-if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY,
-_val, sizeof(delay_val)) != 0) {
-if (errno != ENOTSUP && errno != ENOPROTOOPT) {
-spice_printerr("setsockopt failed, %s", strerror(errno));
-return FALSE;
-}
-}
-}
-
 return TRUE;
 }
 
-- 
2.9.3

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

[Spice-devel] [PATCH spice-server 3/4] reds-stream: Introduce reds_stream_set_no_delay() helper

[Frediano Ziglio]

From: Christophe Fergeau 

The code to enable/disable on a TCP socket is duplicated in multiple
places in the code base, this commit replaces this duplicated code with
a helper in RedsStream.
---
 server/common-graphics-channel.c | 13 ++---
 server/red-channel-client.c  | 17 ++---
 server/reds-stream.c | 16 
 server/reds-stream.h |  1 +
 server/reds.c|  8 +---
 server/sound.c   |  8 +---
 6 files changed, 23 insertions(+), 40 deletions(-)

diff --git a/server/common-graphics-channel.c b/server/common-graphics-channel.c
index bad6e8c..a076111 100644
--- a/server/common-graphics-channel.c
+++ b/server/common-graphics-channel.c
@@ -19,9 +19,6 @@
 #endif
 
 #include 
-#include 
-#include 
-#include 
 
 #include "common-graphics-channel.h"
 #include "dcc.h"
@@ -117,24 +114,18 @@ int common_channel_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);
-int delay_val;
 gboolean is_low_bandwidth;
 
 // TODO - this should be dynamic, not one time at channel creation
 is_low_bandwidth = main_channel_client_is_low_bandwidth(mcc);
-delay_val = is_low_bandwidth ? 0 : 1;
 /* FIXME: Using Nagle's Algorithm can lead to apparent delays, depending
  * on the delayed ack timeout on the other side.
  * Instead of using Nagle's, we need to implement message buffering on
  * the application level.
  * see: http://www.stuartcheshire.org/papers/NagleDelayedAck/
  */
-if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY, _val,
-   sizeof(delay_val)) == -1) {
-if (errno != ENOTSUP) {
-spice_warning("setsockopt failed, %s", strerror(errno));
-}
-}
+reds_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,
 is_low_bandwidth ?
diff --git a/server/red-channel-client.c b/server/red-channel-client.c
index 32db186..f79edfc 100644
--- a/server/red-channel-client.c
+++ b/server/red-channel-client.c
@@ -584,13 +584,7 @@ static void red_channel_client_send_ping(RedChannelClient 
*rcc)
 }  else {
 rcc->priv->latency_monitor.tcp_nodelay = delay_val;
 if (!delay_val) {
-delay_val = 1;
-if (setsockopt(rcc->priv->stream->socket, IPPROTO_TCP, 
TCP_NODELAY, _val,
-   sizeof(delay_val)) == -1) {
-   if (errno != ENOTSUP) {
-spice_warning("setsockopt failed, %s", 
strerror(errno));
-}
-}
+reds_stream_set_no_delay(rcc->priv->stream, TRUE);
 }
 }
 }
@@ -1414,14 +1408,7 @@ static void 
red_channel_client_handle_pong(RedChannelClient *rcc, SpiceMsgPing *
 
 /* set TCP_NODELAY=0, in case we reverted it for the test*/
 if (!rcc->priv->latency_monitor.tcp_nodelay) {
-int delay_val = 0;
-
-if (setsockopt(rcc->priv->stream->socket, IPPROTO_TCP, TCP_NODELAY, 
_val,
-   sizeof(delay_val)) == -1) {
-if (errno != ENOTSUP) {
-spice_warning("setsockopt failed, %s", strerror(errno));
-}
-}
+reds_stream_set_no_delay(rcc->priv->stream, FALSE);
 }
 
 /*
diff --git a/server/reds-stream.c b/server/reds-stream.c
index d0dadb9..f1952cc 100644
--- a/server/reds-stream.c
+++ b/server/reds-stream.c
@@ -21,6 +21,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -256,6 +257,21 @@ int reds_stream_is_plain_unix(const RedsStream *s)
 
 }
 
+bool reds_stream_set_no_delay(RedsStream *stream, bool no_delay)
+{
+int delay_val = no_delay;
+
+if (setsockopt(stream->socket, IPPROTO_TCP, TCP_NODELAY,
+   _val, sizeof(delay_val)) != 0) {
+if (errno != ENOTSUP && errno != ENOPROTOOPT) {
+spice_printerr("setsockopt failed, %s", strerror(errno));
+return FALSE;
+}
+}
+
+return TRUE;
+}
+
 int reds_stream_send_msgfd(RedsStream *stream, int fd)
 {
 struct msghdr msgh = { 0, };
diff --git a/server/reds-stream.h b/server/reds-stream.h
index a8d1736..c76014b 100644
--- a/server/reds-stream.h
+++ b/server/reds-stream.h
@@ -74,6 +74,7 @@ int reds_stream_enable_ssl(RedsStream *stream, SSL_CTX *ctx);
 void reds_stream_set_info_flag(RedsStream *stream, unsigned int flag);
 int reds_stream_get_family(const RedsStream *stream);
 int reds_stream_is_plain_unix(const RedsStream *stream);
+bool reds_stream_set_no_delay(RedsStream *stream, bool no_delay);
 int reds_stream_send_msgfd(RedsStream *stream, int fd);
 
 typedef enum {
diff --git a/server/reds.c b/server/reds.c
index 39a7a31..a245f90 

[Spice-devel] [PATCH spice-server 4/4] Make RedChannel::config_socket() optional

[Frediano Ziglio]

Most channels don't need to do specific settings for the client socket
so avoid the need to do this setting making easier to setup the client
channnel.

Some improvements and commit subject suggested by Christophe Fergeau.

Signed-off-by: Frediano Ziglio 
---
 server/inputs-channel.c | 6 --
 server/main-channel.c   | 6 --
 server/red-channel.c| 6 +-
 server/smartcard.c  | 6 --
 server/spicevmc.c   | 6 --
 5 files changed, 5 insertions(+), 25 deletions(-)

diff --git a/server/inputs-channel.c b/server/inputs-channel.c
index 223f46f..ed92e71 100644
--- a/server/inputs-channel.c
+++ b/server/inputs-channel.c
@@ -484,11 +484,6 @@ static void inputs_pipe_add_init(RedChannelClient *rcc)
 red_channel_client_pipe_add_push(rcc, >base);
 }
 
-static int inputs_channel_config_socket(RedChannelClient *rcc)
-{
-return TRUE;
-}
-
 static void inputs_connect(RedChannel *channel, RedClient *client,
RedsStream *stream, int migration,
int num_common_caps, uint32_t *common_caps,
@@ -634,7 +629,6 @@ inputs_channel_class_init(InputsChannelClass *klass)
 channel_class->handle_message = inputs_channel_handle_message;
 
 /* channel callbacks */
-channel_class->config_socket = inputs_channel_config_socket;
 channel_class->on_disconnect = inputs_channel_on_disconnect;
 channel_class->send_item = inputs_channel_send_item;
 channel_class->alloc_recv_buf = inputs_channel_alloc_msg_rcv_buf;
diff --git a/server/main-channel.c b/server/main-channel.c
index 2d32444..3a6e6cd 100644
--- a/server/main-channel.c
+++ b/server/main-channel.c
@@ -277,11 +277,6 @@ static void 
main_channel_release_msg_rcv_buf(RedChannelClient *rcc,
 }
 }
 
-static int main_channel_config_socket(RedChannelClient *rcc)
-{
-return TRUE;
-}
-
 static int main_channel_handle_migrate_flush_mark(RedChannelClient *rcc)
 {
 RedChannel *channel = red_channel_client_get_channel(rcc);
@@ -355,7 +350,6 @@ main_channel_class_init(MainChannelClass *klass)
 channel_class->handle_message = main_channel_handle_message;
 
 /* channel callbacks */
-channel_class->config_socket = main_channel_config_socket;
 channel_class->on_disconnect = main_channel_client_on_disconnect;
 channel_class->send_item = main_channel_client_send_item;
 channel_class->alloc_recv_buf = main_channel_alloc_msg_rcv_buf;
diff --git a/server/red-channel.c b/server/red-channel.c
index 2f9173b..67a570d 100644
--- a/server/red-channel.c
+++ b/server/red-channel.c
@@ -210,7 +210,7 @@ red_channel_constructed(GObject *object)
 
 G_OBJECT_CLASS(red_channel_parent_class)->constructed(object);
 
-spice_assert(klass->config_socket && klass->on_disconnect &&
+spice_assert(klass->on_disconnect &&
  klass->alloc_recv_buf && klass->release_recv_buf);
 spice_assert(klass->handle_migrate_data ||
  !(self->priv->migration_flags & 
SPICE_MIGRATE_NEED_DATA_TRANSFER));
@@ -732,6 +732,10 @@ int red_channel_config_socket(RedChannel *self, 
RedChannelClient *rcc)
 {
 RedChannelClass *klass = RED_CHANNEL_GET_CLASS(self);
 
+if (!klass->config_socket) {
+return TRUE;
+}
+
 return klass->config_socket(rcc);
 }
 
diff --git a/server/smartcard.c b/server/smartcard.c
index a7cc614..8f12fd9 100644
--- a/server/smartcard.c
+++ b/server/smartcard.c
@@ -394,11 +394,6 @@ void 
smartcard_char_device_detach_client(RedCharDeviceSmartcard *smartcard,
 smartcard->priv->scc = NULL;
 }
 
-static int smartcard_channel_client_config_socket(RedChannelClient *rcc)
-{
-return TRUE;
-}
-
 SmartCardChannelClient* 
smartcard_char_device_get_client(RedCharDeviceSmartcard *smartcard)
 {
 return smartcard->priv->scc;
@@ -585,7 +580,6 @@ red_smartcard_channel_class_init(RedSmartcardChannelClass 
*klass)
 
 channel_class->handle_message = smartcard_channel_client_handle_message,
 
-channel_class->config_socket = smartcard_channel_client_config_socket;
 channel_class->on_disconnect = smartcard_channel_client_on_disconnect;
 channel_class->send_item = smartcard_channel_send_item;
 channel_class->alloc_recv_buf = smartcard_channel_client_alloc_msg_rcv_buf;
diff --git a/server/spicevmc.c b/server/spicevmc.c
index 4b46e54..e705bc7 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -440,11 +440,6 @@ static void spicevmc_char_dev_remove_client(RedCharDevice 
*self,
 red_channel_client_shutdown(channel->rcc);
 }
 
-static int spicevmc_red_channel_client_config_socket(RedChannelClient *rcc)
-{
-return TRUE;
-}
-
 static void spicevmc_red_channel_client_on_disconnect(RedChannelClient *rcc)
 {
 RedVmcChannel *channel;
@@ -736,7 +731,6 @@ red_vmc_channel_class_init(RedVmcChannelClass *klass)
 
 channel_class->handle_message = spicevmc_red_channel_client_handle_message;
 
-channel_class->config_socket = spicevmc_red_channel_client_config_socket;
 

[Spice-devel] [PATCH spice-server 1/4] Clear "msg" pointers after releasing

[Frediano Ziglio]

Avoid possible dangling pointers.

Signed-off-by: Frediano Ziglio 
---
 server/red-channel-client.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/server/red-channel-client.c b/server/red-channel-client.c
index 9ab22e4..32db186 100644
--- a/server/red-channel-client.c
+++ b/server/red-channel-client.c
@@ -1277,6 +1277,7 @@ static void 
red_channel_client_handle_incoming(RedChannelClient *rcc)
 if (bytes_read == -1) {
 red_channel_client_release_msg_buf(rcc, msg_type, msg_size,
buffer->msg);
+buffer->msg = NULL;
 red_channel_client_disconnect(rcc);
 return;
 }
@@ -1296,6 +1297,7 @@ static void 
red_channel_client_handle_incoming(RedChannelClient *rcc)
 red_channel_client_release_msg_buf(rcc,
msg_type, msg_size,
buffer->msg);
+buffer->msg = NULL;
 red_channel_client_disconnect(rcc);
 return;
 }
-- 
2.9.3

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

[Spice-devel] [spice-server v2] gstreamer: Add #ifdef around zero_copy()

[Christophe Fergeau]

Since c3d237 "gstreamer: Avoid memory copy if strides are different" is
only needed when zero copy is disabled. This moves the function
definition to an already existing #ifdef block.
---

Changes since v1:
- do not add redundant #ifdef block

 server/gstreamer-encoder.c | 61 +++---
 1 file changed, 31 insertions(+), 30 deletions(-)

diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index dd2926d..991eb51 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -1140,36 +1140,6 @@ static int is_chunk_stride_aligned(const SpiceBitmap 
*bitmap, uint32_t index)
 return TRUE;
 }
 
-/* A helper for push_raw_frame() */
-static inline int line_copy(SpiceGstEncoder *encoder, const SpiceBitmap 
*bitmap,
-uint32_t chunk_offset, uint32_t stream_stride,
-uint32_t height, uint8_t *buffer)
-{
- uint8_t *dst = buffer;
- SpiceChunks *chunks = bitmap->data;
- uint32_t chunk_index = 0;
- for (int l = 0; l < height; l++) {
- /* We may have to move forward by more than one chunk the first
-  * time around. This also protects us against 0-byte chunks.
-  */
- while (chunk_offset >= chunks->chunk[chunk_index].len) {
- if (!is_chunk_stride_aligned(bitmap, chunk_index)) {
- return FALSE;
- }
- chunk_offset -= chunks->chunk[chunk_index].len;
- chunk_index++;
- }
-
- /* Copy the line */
- uint8_t *src = chunks->chunk[chunk_index].data + chunk_offset;
- memcpy(dst, src, stream_stride);
- dst += stream_stride;
- chunk_offset += bitmap->stride;
- }
- spice_return_val_if_fail(dst - buffer == stream_stride * height, FALSE);
- return TRUE;
-}
-
 #ifdef DO_ZERO_COPY
 typedef struct {
 gint refs;
@@ -1265,6 +1235,37 @@ static void clear_zero_copy_queue(SpiceGstEncoder 
*encoder, gboolean unref_queue
 {
 /* Nothing to do */
 }
+
+/* A helper for push_raw_frame() */
+static inline int line_copy(SpiceGstEncoder *encoder, const SpiceBitmap 
*bitmap,
+uint32_t chunk_offset, uint32_t stream_stride,
+uint32_t height, uint8_t *buffer)
+{
+ uint8_t *dst = buffer;
+ SpiceChunks *chunks = bitmap->data;
+ uint32_t chunk_index = 0;
+ for (int l = 0; l < height; l++) {
+ /* We may have to move forward by more than one chunk the first
+  * time around. This also protects us against 0-byte chunks.
+  */
+ while (chunk_offset >= chunks->chunk[chunk_index].len) {
+ if (!is_chunk_stride_aligned(bitmap, chunk_index)) {
+ return FALSE;
+ }
+ chunk_offset -= chunks->chunk[chunk_index].len;
+ chunk_index++;
+ }
+
+ /* Copy the line */
+ uint8_t *src = chunks->chunk[chunk_index].data + chunk_offset;
+ memcpy(dst, src, stream_stride);
+ dst += stream_stride;
+ chunk_offset += bitmap->stride;
+ }
+ spice_return_val_if_fail(dst - buffer == stream_stride * height, FALSE);
+ return TRUE;
+}
+
 #endif
 
 /* A helper for push_raw_frame() */
-- 
2.9.3

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

[Spice-devel] [spice-server v4 2/5] reds: Close sockets when using spice_server_destroy()

[Christophe Fergeau]

Currently, the network sockets opened by reds_init_net() are not closed
on destruction, in other words they are leaked.

Signed-off-by: Christophe Fergeau 
Acked-by: Pavel Grunt 
---
 server/reds.c | 15 +--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/server/reds.c b/server/reds.c
index 39a7a31..b01e037 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -3390,8 +3390,6 @@ static int do_spice_init(RedsState *reds, 
SpiceCoreInterface *core_interface)
 }
 reds->core = core_interface_adapter;
 reds->core.public_interface = core_interface;
-reds->listen_socket = -1;
-reds->secure_listen_socket = -1;
 reds->agent_dev = red_char_device_vdi_port_new(reds);
 reds_update_agent_properties(reds);
 reds->clients = NULL;
@@ -3484,6 +3482,9 @@ SPICE_GNUC_VISIBLE SpiceServer *spice_server_new(void)
 #ifdef RED_STATISTICS
 reds->stat_file = stat_file_new(REDS_MAX_STAT_NODES);
 #endif
+reds->listen_socket = -1;
+reds->secure_listen_socket = -1;
+
 return reds;
 }
 
@@ -3663,6 +3664,16 @@ SPICE_GNUC_VISIBLE void spice_server_destroy(SpiceServer 
*reds)
 if (reds->main_dispatcher) {
 g_object_unref(reds->main_dispatcher);
 }
+if (reds->listen_socket != -1) {
+   reds_core_watch_remove(reds, reds->listen_watch);
+   if (reds->config->spice_listen_socket_fd != reds->listen_socket) {
+  close(reds->listen_socket);
+   }
+}
+if (reds->secure_listen_socket != -1) {
+   reds_core_watch_remove(reds, reds->secure_listen_watch);
+   close(reds->secure_listen_socket);
+}
 
 reds_cleanup(reds);
 #ifdef RED_STATISTICS
-- 
2.9.3

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

[Spice-devel] [spice-server v4 4/5] test-vdagent: Make test case more useful

[Christophe Fergeau]

This switches the test to using the GTest API, and add several tests
related to https://bugzilla.redhat.com/show_bug.cgi?id=1411194

This uses some API not available in glib 2.28, so this checks we have a
new enough glib before building this test, and disables warnings when
using too new glib API when building it.

The "multiple-vmc-devices" is based off code written by Frediano Ziglio.

Signed-off-by: Christophe Fergeau 
---
 configure.ac|  7 
 server/tests/Makefile.am| 20 --
 server/tests/test-vdagent.c | 89 +++--
 3 files changed, 92 insertions(+), 24 deletions(-)

diff --git a/configure.ac b/configure.ac
index f04585f..fd9fbae 100644
--- a/configure.ac
+++ b/configure.ac
@@ -148,6 +148,13 @@ AC_SUBST([SPICE_PROTOCOL_MIN_VER])
 GLIB2_REQUIRED=2.28
 GLIB2_ENCODED_VERSION="GLIB_VERSION_2_28"
 PKG_CHECK_MODULES([GLIB2], [glib-2.0 >= $GLIB2_REQUIRED gio-2.0 >= 
$GLIB2_REQUIRED])
+PKG_CHECK_EXISTS([glib-2.0 >= 2.34], have_glib234="yes", have_glib234="no")
+dnl disable building of some tests if glib is too old
+AS_IF([test x"$have_glib234" = "xyes"], [
+GLIB2_NO_MAXVER_CFLAGS="$GLIB2_CFLAGS"
+AC_SUBST([GLIB2_NO_MAXVER_CFLAGS])
+])
+AM_CONDITIONAL([HAVE_GLIB234], [test x"$have_glib234" = "xyes"])
 GLIB2_CFLAGS="$GLIB2_CFLAGS -DGLIB_VERSION_MIN_REQUIRED=$GLIB2_ENCODED_VERSION 
\
   -DGLIB_VERSION_MAX_ALLOWED=$GLIB2_ENCODED_VERSION"
 AS_VAR_APPEND([SPICE_REQUIRES], [" glib-2.0 >= $GLIB2_REQUIRED gio-2.0 >= 
$GLIB2_REQUIRED"])
diff --git a/server/tests/Makefile.am b/server/tests/Makefile.am
index af0bd20..6196676 100644
--- a/server/tests/Makefile.am
+++ b/server/tests/Makefile.am
@@ -1,19 +1,23 @@
 NULL =
 
-AM_CPPFLAGS =  \
+COMMON_CPPFLAGS =  \
-I$(top_srcdir) \
-I$(top_srcdir)/server  \
-I$(top_builddir)/server\
-I$(top_srcdir)/server/tests\
$(COMMON_CFLAGS)\
-   $(GLIB2_CFLAGS) \
-   $(GOBJECT2_CFLAGS)  \
+   $(GOBJECT2_CFLAGS)  \
$(SMARTCARD_CFLAGS) \
$(SPICE_NONPKGCONFIG_CFLAGS)\
$(SPICE_PROTOCOL_CFLAGS)\
$(WARN_CFLAGS)  \
$(NULL)
 
+AM_CPPFLAGS =  \
+   $(COMMON_CPPFLAGS)  \
+   $(GLIB2_CFLAGS) \
+   $(NULL)
+
 if HAVE_AUTOMATED_TESTS
 AM_CPPFLAGS += -DAUTOMATED_TESTS
 endif
@@ -56,12 +60,20 @@ noinst_PROGRAMS =   \
test-playback   \
test-display-resolution-changes \
test-two-servers\
-   test-vdagent\
test-display-width-stride   \
spice-server-replay \
$(check_PROGRAMS)   \
$(NULL)
 
+if HAVE_GLIB234
+check_PROGRAMS += test-vdagent
+noinst_PROGRAMS += test-vdagent
+test_vdagent_CPPFLAGS =\
+   $(COMMON_CPPFLAGS)  \
+   $(GLIB2_NO_MAXVER_CFLAGS)   \
+   $(NULL)
+endif
+
 if HAVE_GSTREAMER
 noinst_PROGRAMS += test-gst
 endif
diff --git a/server/tests/test-vdagent.c b/server/tests/test-vdagent.c
index e06229e..faf5212 100644
--- a/server/tests/test-vdagent.c
+++ b/server/tests/test-vdagent.c
@@ -37,14 +37,6 @@ int ping_ms = 100;
 #define MIN(a, b) ((a) > (b) ? (b) : (a))
 #endif
 
-static void pinger(SPICE_GNUC_UNUSED void *opaque)
-{
-// show_channels is not thread safe - fails if disconnections / 
connections occur
-//show_channels(server);
-
-core->timer_start(ping_timer, ping_ms);
-}
-
 static int vmc_write(SPICE_GNUC_UNUSED SpiceCharDeviceInstance *sin,
  SPICE_GNUC_UNUSED const uint8_t *buf,
  int len)
@@ -62,6 +54,13 @@ static int vmc_read(SPICE_GNUC_UNUSED 
SpiceCharDeviceInstance *sin,
 static unsigned message_size;
 int ret;
 
+if (pos == sizeof(message)) {
+g_message("sent whole message");
+pos++; /* Only print message once */
+}
+if (pos > sizeof(message)) {
+return 0;
+}
 if (pos == 0) {
 VDIChunkHeader *hdr = (VDIChunkHeader *)message;
 VDAgentMessage *msg = (VDAgentMessage *)[1];
@@ -83,9 +82,6 @@ static int vmc_read(SPICE_GNUC_UNUSED SpiceCharDeviceInstance 
*sin,
 ret = MIN(message_size - pos, len);
 memcpy(buf, [pos], ret);
 pos += ret;
-if (pos == message_size) {
-pos = 0;
-}
 //printf("vmc_read %d (ret %d)\n", len, ret);
 return ret;
 }
@@ -105,24 +101,77 @@ static SpiceCharDeviceInterface vmc_interface = {
 .read   = vmc_read,
 };
 
-SpiceCharDeviceInstance 

Re: [Spice-devel] [PATCH spice-server 4/4] Provide and reuse default implementation for config_socket

[Frediano Ziglio]

> 
> On Wed, Feb 15, 2017 at 06:02:00AM -0500, Frediano Ziglio wrote:
> > > 
> > > On Tue, Feb 14, 2017 at 03:55:25PM +0100, Christophe Fergeau wrote:
> > > > I would not provide any default implementation, and just change
> > > > red_channel_config_socket to
> > > > 
> > > 
> > > Thinking more about this, I actually would provide a default
> > > implementation, and move the setsockopt/fnctl code from
> > > reds_init_client_connection() in the default implementation, and make
> > > sure the child classes properly chain up to it (or call it directly if
> > > chaining up is complicated)
> > > 
> > > Christophe
> > > 
> > 
> > This would introduce possible race conditions.
> > On reds.c the sockets are initialized before any usage.
> > Setting not blocking is quite mandatory to avoid some races using
> > some layers (like TLS). In this case before config_socket there
> > are the headers to tell which channel is this. A client could use
> > the race to cause the Qemu main thread to stop waiting for data
> > (potentially this could happen without authentication).
> 
> Does this include TCP_NODELAY as well?
> 
> Christophe
> 

No, just slows down connection a bit.

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

[Spice-devel] [spice-server v4 1/5] test: Add test_destroy()

[Christophe Fergeau]

This allows to chain several test cases by using
test_new()/test_destroy().

Signed-off-by: Christophe Fergeau 
Acked-by: Pavel Grunt 
---
 server/tests/test-display-base.c | 7 +++
 server/tests/test-display-base.h | 1 +
 2 files changed, 8 insertions(+)

diff --git a/server/tests/test-display-base.c b/server/tests/test-display-base.c
index 55e37a5..636c505 100644
--- a/server/tests/test-display-base.c
+++ b/server/tests/test-display-base.c
@@ -908,6 +908,13 @@ Test *test_new(SpiceCoreInterface *core)
 return test;
 }
 
+void test_destroy(Test *test)
+{
+test->core->timer_remove(test->wakeup_timer);
+spice_server_destroy(test->server);
+free(test);
+}
+
 static void init_automated(void)
 {
 struct sigaction sa;
diff --git a/server/tests/test-display-base.h b/server/tests/test-display-base.h
index 7b5b509..bdf7a11 100644
--- a/server/tests/test-display-base.h
+++ b/server/tests/test-display-base.h
@@ -137,6 +137,7 @@ void test_set_command_list(Test *test, Command *command, 
int num_commands);
 void test_add_display_interface(Test *test);
 void test_add_agent_interface(SpiceServer *server); // TODO - Test *test
 Test* test_new(SpiceCoreInterface* core);
+void test_destroy(Test *test);
 
 uint32_t test_get_width(void);
 uint32_t test_get_height(void);
-- 
2.9.3

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

[Spice-devel] [spice-server v4 0/5] Better test-vdagent test case

[Christophe Fergeau]

Only changes since v3 is the removal of redundant initialization in patch 2/5,
and an additional patch changing some spice_printerr() messages to 
spice_debug().

Christophe

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

[Spice-devel] [spice-server v4 3/5] Use spice_debug rather than spice_info in library code

[Christophe Fergeau]

Using spice_info() gets in the way of tests using
g_test_expect_message() as all the messages emitted using
a non-debug log level must be listed as expected, otherwise we get a
critical about an expected message not having been logged.

Signed-off-by: Christophe Fergeau 
---
 server/cursor-channel.c |  4 +--
 server/dcc.c|  6 ++--
 server/display-channel.c| 22 ++--
 server/image-encoders.c | 12 +++
 server/red-channel-client.c |  4 +--
 server/red-worker.c | 34 +-
 server/reds-stream.c| 40 ++---
 server/reds.c   | 86 ++---
 server/sound.c  |  2 +-
 9 files changed, 105 insertions(+), 105 deletions(-)

diff --git a/server/cursor-channel.c b/server/cursor-channel.c
index 4fe3f8d..fe56098 100644
--- a/server/cursor-channel.c
+++ b/server/cursor-channel.c
@@ -294,7 +294,7 @@ static void cursor_channel_send_item(RedChannelClient *rcc, 
RedPipeItem *pipe_it
 CursorChannel* cursor_channel_new(RedsState *server, QXLInstance *qxl,
   const SpiceCoreInterfaceInternal *core)
 {
-spice_info("create cursor channel");
+spice_debug("create cursor channel");
 return g_object_new(TYPE_CURSOR_CHANNEL,
 "spice-server", server,
 "core-interface", core,
@@ -413,7 +413,7 @@ void cursor_channel_connect(CursorChannel *cursor, 
RedClient *client, RedsStream
 
 spice_return_if_fail(cursor != NULL);
 
-spice_info("add cursor channel client");
+spice_debug("add cursor channel client");
 ccc = cursor_channel_client_new(cursor, client, stream,
 migrate,
 common_caps, num_common_caps,
diff --git a/server/dcc.c b/server/dcc.c
index cf9431a..c3271bb 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -514,7 +514,7 @@ DisplayChannelClient *dcc_new(DisplayChannel *display,
  "jpeg-state", jpeg_state,
  "zlib-glz-state", zlib_glz_state,
  NULL);
-spice_info("New display (client %p) dcc %p stream %p", client, dcc, 
stream);
+spice_debug("New display (client %p) dcc %p stream %p", client, dcc, 
stream);
 
common_graphics_channel_set_during_target_migrate(COMMON_GRAPHICS_CHANNEL(display),
 mig_target);
 dcc->priv->id = 
common_graphics_channel_get_qxl(COMMON_GRAPHICS_CHANNEL(display))->id;
 
@@ -550,7 +550,7 @@ static int 
display_channel_client_wait_for_init(DisplayChannelClient *dcc)
 if (dcc->priv->pixmap_cache && dcc->priv->encoders.glz_dict) {
 dcc->priv->pixmap_cache_generation = 
dcc->priv->pixmap_cache->generation;
 /* TODO: move common.id? if it's used for a per client structure.. 
*/
-spice_info("creating encoder with id == %d", dcc->priv->id);
+spice_debug("creating encoder with id == %d", dcc->priv->id);
 if (!image_encoders_glz_create(>priv->encoders, 
dcc->priv->id)) {
 spice_critical("create global lz failed");
 }
@@ -1052,7 +1052,7 @@ static int dcc_handle_stream_report(DisplayChannelClient 
*dcc,
 
 agent = >priv->stream_agents[report->stream_id];
 if (!agent->video_encoder) {
-spice_info("stream_report: no encoder for stream id %u. "
+spice_debug("stream_report: no encoder for stream id %u. "
"The stream has probably been destroyed",
report->stream_id);
 return TRUE;
diff --git a/server/display-channel.c b/server/display-channel.c
index 288969c..aa71ba4 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -192,13 +192,13 @@ void display_channel_set_stream_video(DisplayChannel 
*display, int stream_video)
 
 switch (stream_video) {
 case SPICE_STREAM_VIDEO_ALL:
-spice_info("sv all");
+spice_debug("sv all");
 break;
 case SPICE_STREAM_VIDEO_FILTER:
-spice_info("sv filter");
+spice_debug("sv filter");
 break;
 case SPICE_STREAM_VIDEO_OFF:
-spice_info("sv off");
+spice_debug("sv off");
 break;
 default:
 spice_warn_if_reached();
@@ -239,7 +239,7 @@ static void stop_streams(DisplayChannel *display)
 if (!stream->current) {
 stream_stop(display, stream);
 } else {
-spice_info("attached stream");
+spice_debug("attached stream");
 }
 }
 
@@ -883,17 +883,17 @@ static bool drawable_can_stream(DisplayChannel *display, 
Drawable *drawable)
 static void display_channel_print_stats(DisplayChannel *display)
 {
 stat_time_t total = display->priv->add_stat.total;
-spice_info("add with shadow count %u",
+spice_debug("add with shadow count %u",
display->priv->add_with_shadow_count);
 display->priv->add_with_shadow_count = 0;
-

Re: [Spice-devel] [PATCH spice-server 4/4] Provide and reuse default implementation for config_socket

[Christophe Fergeau]

On Wed, Feb 15, 2017 at 06:02:00AM -0500, Frediano Ziglio wrote:
> > 
> > On Tue, Feb 14, 2017 at 03:55:25PM +0100, Christophe Fergeau wrote:
> > > I would not provide any default implementation, and just change
> > > red_channel_config_socket to
> > > 
> > 
> > Thinking more about this, I actually would provide a default
> > implementation, and move the setsockopt/fnctl code from
> > reds_init_client_connection() in the default implementation, and make
> > sure the child classes properly chain up to it (or call it directly if
> > chaining up is complicated)
> > 
> > Christophe
> > 
> 
> This would introduce possible race conditions.
> On reds.c the sockets are initialized before any usage.
> Setting not blocking is quite mandatory to avoid some races using
> some layers (like TLS). In this case before config_socket there
> are the headers to tell which channel is this. A client could use
> the race to cause the Qemu main thread to stop waiting for data
> (potentially this could happen without authentication).

Does this include TCP_NODELAY as well?

Christophe


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

Re: [Spice-devel] [PATCH spice-server 4/4] Provide and reuse default implementation for config_socket

[Frediano Ziglio]

> 
> On Tue, Feb 14, 2017 at 03:55:25PM +0100, Christophe Fergeau wrote:
> > I would not provide any default implementation, and just change
> > red_channel_config_socket to
> > 
> 
> Thinking more about this, I actually would provide a default
> implementation, and move the setsockopt/fnctl code from
> reds_init_client_connection() in the default implementation, and make
> sure the child classes properly chain up to it (or call it directly if
> chaining up is complicated)
> 
> Christophe
> 

This would introduce possible race conditions.
On reds.c the sockets are initialized before any usage.
Setting not blocking is quite mandatory to avoid some races using
some layers (like TLS). In this case before config_socket there
are the headers to tell which channel is this. A client could use
the race to cause the Qemu main thread to stop waiting for data
(potentially this could happen without authentication).

> > 
> > @@ -739,6 +739,10 @@ int red_channel_config_socket(RedChannel *self,
> > RedChannelClient *rcc)
> >  {
> >  RedChannelClass *klass = RED_CHANNEL_GET_CLASS(self);
> > 
> > +if (!klass->config_socket) {
> > +return TRUE;
> > +}
> > +
> >  return klass->config_socket(rcc);
> >  }
> > 
> > If you prefer to provide an empty stub as the default impl, I'd name it
> > red_channel_client_default_config_socket()
> > 
> > Reviewed-by: Christophe Fergeau 
> > 
> > Christophe


On the many things reds.c do there are some network stuff (like
TLS/SASL) that perhaps are responsibility of RedsStream.

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

Re: [Spice-devel] [vdagent-linux v8 4/4] vdagentd: Do endian swapping.

[Victor Toso]

On Wed, Feb 15, 2017 at 10:48:07AM +0100, Victor Toso wrote:
> From: Michal Suchanek 
>
> This allows running big endian and little endian guest side by side using
> cut & paste between them.
>
> There is a general design idea that swapping should come as close to
> virtio_read/virtio_write as possible. In particular, the protocol
> between vdagent and vdagentd is guest-specific and in native endian.
> With muliple layers of headers this is a bit tricky. A few message types
> have to be swapped fully before passing through vdagentd.
>
> Signed-off-by: Michal Suchanek 
> Signed-off-by: Victor Toso 
> ---
>  src/vdagentd/vdagentd.c| 109 
> -
>  src/vdagentd/virtio-port.c |  36 +--
>  2 files changed, 120 insertions(+), 25 deletions(-)
>
> diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> index 2329489..954bba1 100644
> --- a/src/vdagentd/vdagentd.c
> +++ b/src/vdagentd/vdagentd.c
> @@ -78,6 +78,34 @@ static int client_connected = 0;
>  static int max_clipboard = -1;
>  
>  /* utility functions */
> +static void virtio_msg_uint32_to_le(uint8_t *_msg, uint32_t size, uint32_t 
> offset)
> +{
> +uint32_t i, *msg = (uint32_t *)(_msg + offset);
> +
> +/* offset - size % 4 should be 0 - extra bytes are ignored */
> +for (i = 0; i < (size - offset) / 4; i++)
> +msg[i] = GUINT32_TO_LE(msg[i]);
> +}
> +
> +static void virtio_msg_uint32_from_le(uint8_t *_msg, uint32_t size, uint32_t 
> offset)
> +{
> +uint32_t i, *msg = (uint32_t *)(_msg + offset);
> +
> +/* offset - size % 4 should be 0 - extra bytes are ignored */
> +for (i = 0; i < (size - offset) / 4; i++)
> +msg[i] = GUINT32_FROM_LE(msg[i]);
> +}
> +
> +static void virtio_msg_uint16_from_le(uint8_t *_msg, uint32_t size, uint32_t 
> offset)
> +{
> +uint32_t i;
> +uint16_t *msg = (uint16_t *)(_msg + offset);
> +
> +/* offset - size % 2 should be 0 - extra bytes are ignored */
> +for (i = 0; i < (size - offset) / 2; i++)
> +msg[i] = GUINT16_FROM_LE(msg[i]);
> +}
> +
>  /* vdagentd <-> spice-client communication handling */
>  static void send_capabilities(struct vdagent_virtio_port *vport,
>  uint32_t request)
> @@ -102,6 +130,7 @@ static void send_capabilities(struct vdagent_virtio_port 
> *vport,
>  VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_GUEST_LINEEND_LF);
>  VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_MAX_CLIPBOARD);
>  VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_AUDIO_VOLUME_SYNC);
> +virtio_msg_uint32_to_le((uint8_t *)caps, size, 0);
>  
>  vdagent_virtio_port_write(vport, VDP_CLIENT_PORT,
>VD_AGENT_ANNOUNCE_CAPABILITIES, 0,
> @@ -174,8 +203,8 @@ static void do_client_monitors(struct vdagent_virtio_port 
> *vport, int port_nr,
>  (uint8_t *)mon_config, size);
>  
>  /* Acknowledge reception of monitors config to spice server / client */
> -reply.type  = VD_AGENT_MONITORS_CONFIG;
> -reply.error = VD_AGENT_SUCCESS;
> +reply.type  = GUINT32_TO_LE(VD_AGENT_MONITORS_CONFIG);
> +reply.error = GUINT32_TO_LE(VD_AGENT_SUCCESS);
>  vdagent_virtio_port_write(vport, port_nr, VD_AGENT_REPLY, 0,
>(uint8_t *), sizeof(reply));
>  }
> @@ -278,8 +307,8 @@ static void send_file_xfer_status(struct 
> vdagent_virtio_port *vport,
>const char *msg, uint32_t id, uint32_t 
> xfer_status)
>  {
>  VDAgentFileXferStatusMessage status = {
> -.id = id,
> -.result = xfer_status,
> +.id = GUINT32_TO_LE(id),
> +.result = GUINT32_TO_LE(xfer_status),
>  };
>  syslog(LOG_WARNING, msg, id);
>  if (vport)
> @@ -361,6 +390,50 @@ static gsize vdagent_message_min_size[] =
>  sizeof(VDAgentAudioVolumeSync), /* VD_AGENT_AUDIO_VOLUME_SYNC */
>  };
>  
> +static void vdagent_message_clipboard_from_le(VDAgentMessage *message_header,
> +uint8_t *data)
> +{
> +gsize min_size = vdagent_message_min_size[message_header->type];
> +uint32_t *data_type = (uint32_t *) data;
> +
> +if (VD_AGENT_HAS_CAPABILITY(capabilities, capabilities_size,
> +VD_AGENT_CAP_CLIPBOARD_SELECTION)) {
> +min_size += 4;
> +data_type++;
> +}
> +
> +switch (message_header->type) {
> +case VD_AGENT_CLIPBOARD_REQUEST:
> +case VD_AGENT_CLIPBOARD:
> +*data_type = GUINT32_FROM_LE(*data_type);
> +break;
> +case VD_AGENT_CLIPBOARD_GRAB:
> +virtio_msg_uint32_from_le(data, message_header->size, min_size);
> +break;
> +default:
> +g_warn_if_reached();
> +}
> +}
> +
> +static void vdagent_message_file_xfer_from_le(VDAgentMessage *message_header,
> +uint8_t *data)
> +{
> +uint32_t *id = (uint32_t *)data;
> +*id = GUINT32_FROM_LE(*id);
> +id++; /* status */
> +
> +

Re: [Spice-devel] [vdagent-linux v8 3/4] Adjust size checks

[Victor Toso]

On Wed, Feb 15, 2017 at 10:48:06AM +0100, Victor Toso wrote:
> From: Christophe Fergeau 

'vdagentd:' prefix here too?

Just a small note, this was also wrong before "vdagentd: early
return on bad message size" patch.

Acked-by: Victor Toso 

>
> ---
>  src/vdagentd/vdagentd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> index 8d43700..2329489 100644
> --- a/src/vdagentd/vdagentd.c
> +++ b/src/vdagentd/vdagentd.c
> @@ -395,8 +395,6 @@ static gboolean vdagent_message_check_size(const 
> VDAgentMessage *message_header)
>  case VD_AGENT_FILE_XFER_DATA:
>  case VD_AGENT_CLIPBOARD:
>  case VD_AGENT_CLIPBOARD_GRAB:
> -case VD_AGENT_CLIPBOARD_REQUEST:
> -case VD_AGENT_CLIPBOARD_RELEASE:
>  case VD_AGENT_AUDIO_VOLUME_SYNC:
>  case VD_AGENT_ANNOUNCE_CAPABILITIES:
>  if (message_header->size < min_size) {
> @@ -409,6 +407,8 @@ static gboolean vdagent_message_check_size(const 
> VDAgentMessage *message_header)
>  case VD_AGENT_FILE_XFER_STATUS:
>  case VD_AGENT_DISPLAY_CONFIG:
>  case VD_AGENT_REPLY:
> +case VD_AGENT_CLIPBOARD_REQUEST:
> +case VD_AGENT_CLIPBOARD_RELEASE:
>  case VD_AGENT_MAX_CLIPBOARD:
>  case VD_AGENT_CLIENT_DISCONNECTED:
>  if (message_header->size != min_size) {
> -- 
> 2.9.3
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [vdagent-linux v8 2/4] Add missing size checks

[Victor Toso]

On Wed, Feb 15, 2017 at 10:48:05AM +0100, Victor Toso wrote:
> From: Christophe Fergeau 

Maybe include vdagentd: prefix in the commit log?
Acked-by: Victor Toso 

>
> ---
>  src/vdagentd/vdagentd.c | 14 ++
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> index ea8482b..8d43700 100644
> --- a/src/vdagentd/vdagentd.c
> +++ b/src/vdagentd/vdagentd.c
> @@ -391,6 +391,8 @@ static gboolean vdagent_message_check_size(const 
> VDAgentMessage *message_header)
>  
>  switch (message_header->type) {
>  case VD_AGENT_MONITORS_CONFIG:
> +case VD_AGENT_FILE_XFER_START:
> +case VD_AGENT_FILE_XFER_DATA:
>  case VD_AGENT_CLIPBOARD:
>  case VD_AGENT_CLIPBOARD_GRAB:
>  case VD_AGENT_CLIPBOARD_REQUEST:
> @@ -404,21 +406,17 @@ static gboolean vdagent_message_check_size(const 
> VDAgentMessage *message_header)
>  }
>  break;
>  case VD_AGENT_MOUSE_STATE:
> +case VD_AGENT_FILE_XFER_STATUS:
> +case VD_AGENT_DISPLAY_CONFIG:
> +case VD_AGENT_REPLY:
>  case VD_AGENT_MAX_CLIPBOARD:
> +case VD_AGENT_CLIENT_DISCONNECTED:
>  if (message_header->size != min_size) {
>  syslog(LOG_ERR, "read: invalid message size: %u for message 
> type: %u",
> message_header->size, message_header->type);
>  return FALSE;
>  }
>  break;
> -case VD_AGENT_FILE_XFER_START:
> -case VD_AGENT_FILE_XFER_DATA:
> -case VD_AGENT_FILE_XFER_STATUS:
> -case VD_AGENT_CLIENT_DISCONNECTED:
> -/* No size checks for these at the moment */
> -break;
> -case VD_AGENT_DISPLAY_CONFIG:
> -case VD_AGENT_REPLY:
>  default:
>  g_warn_if_reached();
>  return FALSE;
> -- 
> 2.9.3
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

Re: [Spice-devel] [vdagent-linux v8 1/4] vdagentd: early return on bad message size

[Victor Toso]

Hi,

On Wed, Feb 15, 2017 at 10:48:04AM +0100, Victor Toso wrote:
> From: Victor Toso 
>
> The payload size for each message should be the size of the expected
> struct or bigger when it contain arrays of no fixed size.
>
> This patch creates the vdagent_message_min_size[] static array with
> the expected size for each message so we can check on
> virtio_port_read_complete().

This is not 100% true anymore. We've also created a
vdagent_message_check_size() which is the one that uses the
vdagent_message_min_size[].

Maybe:

"This patch creates:
 * vdagent_message_min_size[] static array with the
   expected size for each message;
 * vdagent_message_check_size() which checks the size of message's
   payload based on the type of message and by using
   vdagent_message_min_size[] as reference"

(maybe it got worse, comments are welcome :))

>
> Signed-off-by: Victor Toso 
> Signed-off-by: Michal Suchanek 
> ---
>  src/vdagentd/vdagentd.c | 133 
> +---
>  1 file changed, 91 insertions(+), 42 deletions(-)
>
> diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> index 5ca0106..ea8482b 100644
> --- a/src/vdagentd/vdagentd.c
> +++ b/src/vdagentd/vdagentd.c
> @@ -341,34 +341,109 @@ static void do_client_file_xfer(struct 
> vdagent_virtio_port *vport,
>  udscs_write(conn, msg_type, 0, 0, data, message_header->size);
>  }
>
> +static gsize vdagent_message_min_size[] =
> +{
> +-1, /* Does not exist */
> +sizeof(VDAgentMouseState), /* VD_AGENT_MOUSE_STATE */
> +sizeof(VDAgentMonitorsConfig), /* VD_AGENT_MONITORS_CONFIG */
> +sizeof(VDAgentReply), /* VD_AGENT_REPLY */
> +sizeof(VDAgentClipboard), /* VD_AGENT_CLIPBOARD */
> +sizeof(VDAgentDisplayConfig), /* VD_AGENT_DISPLAY_CONFIG */
> +sizeof(VDAgentAnnounceCapabilities), /* VD_AGENT_ANNOUNCE_CAPABILITIES */
> +sizeof(VDAgentClipboardGrab), /* VD_AGENT_CLIPBOARD_GRAB */
> +sizeof(VDAgentClipboardRequest), /* VD_AGENT_CLIPBOARD_REQUEST */
> +sizeof(VDAgentClipboardRelease), /* VD_AGENT_CLIPBOARD_RELEASE */
> +sizeof(VDAgentFileXferStartMessage), /* VD_AGENT_FILE_XFER_START */
> +sizeof(VDAgentFileXferStatusMessage), /* VD_AGENT_FILE_XFER_STATUS */
> +sizeof(VDAgentFileXferDataMessage), /* VD_AGENT_FILE_XFER_DATA */
> +0, /* VD_AGENT_CLIENT_DISCONNECTED */
> +sizeof(VDAgentMaxClipboard), /* VD_AGENT_MAX_CLIPBOARD */
> +sizeof(VDAgentAudioVolumeSync), /* VD_AGENT_AUDIO_VOLUME_SYNC */
> +};
> +
> +static gboolean vdagent_message_check_size(const VDAgentMessage 
> *message_header)
> +{
> +uint32_t min_size = 0;
> +
> +if (message_header->protocol != VD_AGENT_PROTOCOL) {
> +syslog(LOG_ERR, "message with wrong protocol version ignoring");
> +return FALSE;
> +}
> +
> +if (!message_header->type ||
> +message_header->type >= G_N_ELEMENTS(vdagent_message_min_size)) {
> +syslog(LOG_WARNING, "unknown message type %d, ignoring",
> +   message_header->type);
> +return FALSE;
> +}
> +
> +min_size = vdagent_message_min_size[message_header->type];
> +if (VD_AGENT_HAS_CAPABILITY(capabilities, capabilities_size,
> +VD_AGENT_CAP_CLIPBOARD_SELECTION)) {
> +switch (message_header->type) {
> +case VD_AGENT_CLIPBOARD_GRAB:
> +case VD_AGENT_CLIPBOARD_REQUEST:
> +case VD_AGENT_CLIPBOARD:
> +case VD_AGENT_CLIPBOARD_RELEASE:
> +  min_size += 4;
> +}
> +}
> +
> +switch (message_header->type) {
> +case VD_AGENT_MONITORS_CONFIG:
> +case VD_AGENT_CLIPBOARD:
> +case VD_AGENT_CLIPBOARD_GRAB:
> +case VD_AGENT_CLIPBOARD_REQUEST:
> +case VD_AGENT_CLIPBOARD_RELEASE:
> +case VD_AGENT_AUDIO_VOLUME_SYNC:
> +case VD_AGENT_ANNOUNCE_CAPABILITIES:
> +if (message_header->size < min_size) {
> +syslog(LOG_ERR, "read: invalid message size: %u for message 
> type: %u",
> +   message_header->size, message_header->type);
> +return FALSE;
> +}
> +break;
> +case VD_AGENT_MOUSE_STATE:
> +case VD_AGENT_MAX_CLIPBOARD:
> +if (message_header->size != min_size) {
> +syslog(LOG_ERR, "read: invalid message size: %u for message 
> type: %u",
> +   message_header->size, message_header->type);
> +return FALSE;
> +}
> +break;
> +case VD_AGENT_FILE_XFER_START:
> +case VD_AGENT_FILE_XFER_DATA:
> +case VD_AGENT_FILE_XFER_STATUS:
> +case VD_AGENT_CLIENT_DISCONNECTED:
> +/* No size checks for these at the moment */
> +break;
> +case VD_AGENT_DISPLAY_CONFIG:
> +case VD_AGENT_REPLY:
> +default:
> +g_warn_if_reached();
> +return FALSE;
> +}
> +return TRUE;
> +}
> +
>  static int virtio_port_read_complete(
>  struct vdagent_virtio_port *vport,
> 

Re: [Spice-devel] [PATCH spice-server 4/4] Provide and reuse default implementation for config_socket

[Christophe Fergeau]

On Tue, Feb 14, 2017 at 03:55:25PM +0100, Christophe Fergeau wrote:
> I would not provide any default implementation, and just change
> red_channel_config_socket to
> 

Thinking more about this, I actually would provide a default
implementation, and move the setsockopt/fnctl code from
reds_init_client_connection() in the default implementation, and make
sure the child classes properly chain up to it (or call it directly if
chaining up is complicated)

Christophe

> 
> @@ -739,6 +739,10 @@ int red_channel_config_socket(RedChannel *self, 
> RedChannelClient *rcc)
>  {
>  RedChannelClass *klass = RED_CHANNEL_GET_CLASS(self);
> 
> +if (!klass->config_socket) {
> +return TRUE;
> +}
> +
>  return klass->config_socket(rcc);
>  }
> 
> If you prefer to provide an empty stub as the default impl, I'd name it
> red_channel_client_default_config_socket()
> 
> Reviewed-by: Christophe Fergeau 
> 
> Christophe



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



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

[Spice-devel] [vdagent-linux v8 4/4] vdagentd: Do endian swapping.

[Victor Toso]

From: Michal Suchanek 

This allows running big endian and little endian guest side by side using
cut & paste between them.

There is a general design idea that swapping should come as close to
virtio_read/virtio_write as possible. In particular, the protocol
between vdagent and vdagentd is guest-specific and in native endian.
With muliple layers of headers this is a bit tricky. A few message types
have to be swapped fully before passing through vdagentd.

Signed-off-by: Michal Suchanek 
Signed-off-by: Victor Toso 
---
 src/vdagentd/vdagentd.c| 109 -
 src/vdagentd/virtio-port.c |  36 +--
 2 files changed, 120 insertions(+), 25 deletions(-)

diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
index 2329489..954bba1 100644
--- a/src/vdagentd/vdagentd.c
+++ b/src/vdagentd/vdagentd.c
@@ -78,6 +78,34 @@ static int client_connected = 0;
 static int max_clipboard = -1;
 
 /* utility functions */
+static void virtio_msg_uint32_to_le(uint8_t *_msg, uint32_t size, uint32_t 
offset)
+{
+uint32_t i, *msg = (uint32_t *)(_msg + offset);
+
+/* offset - size % 4 should be 0 - extra bytes are ignored */
+for (i = 0; i < (size - offset) / 4; i++)
+msg[i] = GUINT32_TO_LE(msg[i]);
+}
+
+static void virtio_msg_uint32_from_le(uint8_t *_msg, uint32_t size, uint32_t 
offset)
+{
+uint32_t i, *msg = (uint32_t *)(_msg + offset);
+
+/* offset - size % 4 should be 0 - extra bytes are ignored */
+for (i = 0; i < (size - offset) / 4; i++)
+msg[i] = GUINT32_FROM_LE(msg[i]);
+}
+
+static void virtio_msg_uint16_from_le(uint8_t *_msg, uint32_t size, uint32_t 
offset)
+{
+uint32_t i;
+uint16_t *msg = (uint16_t *)(_msg + offset);
+
+/* offset - size % 2 should be 0 - extra bytes are ignored */
+for (i = 0; i < (size - offset) / 2; i++)
+msg[i] = GUINT16_FROM_LE(msg[i]);
+}
+
 /* vdagentd <-> spice-client communication handling */
 static void send_capabilities(struct vdagent_virtio_port *vport,
 uint32_t request)
@@ -102,6 +130,7 @@ static void send_capabilities(struct vdagent_virtio_port 
*vport,
 VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_GUEST_LINEEND_LF);
 VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_MAX_CLIPBOARD);
 VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_AUDIO_VOLUME_SYNC);
+virtio_msg_uint32_to_le((uint8_t *)caps, size, 0);
 
 vdagent_virtio_port_write(vport, VDP_CLIENT_PORT,
   VD_AGENT_ANNOUNCE_CAPABILITIES, 0,
@@ -174,8 +203,8 @@ static void do_client_monitors(struct vdagent_virtio_port 
*vport, int port_nr,
 (uint8_t *)mon_config, size);
 
 /* Acknowledge reception of monitors config to spice server / client */
-reply.type  = VD_AGENT_MONITORS_CONFIG;
-reply.error = VD_AGENT_SUCCESS;
+reply.type  = GUINT32_TO_LE(VD_AGENT_MONITORS_CONFIG);
+reply.error = GUINT32_TO_LE(VD_AGENT_SUCCESS);
 vdagent_virtio_port_write(vport, port_nr, VD_AGENT_REPLY, 0,
   (uint8_t *), sizeof(reply));
 }
@@ -278,8 +307,8 @@ static void send_file_xfer_status(struct 
vdagent_virtio_port *vport,
   const char *msg, uint32_t id, uint32_t 
xfer_status)
 {
 VDAgentFileXferStatusMessage status = {
-.id = id,
-.result = xfer_status,
+.id = GUINT32_TO_LE(id),
+.result = GUINT32_TO_LE(xfer_status),
 };
 syslog(LOG_WARNING, msg, id);
 if (vport)
@@ -361,6 +390,50 @@ static gsize vdagent_message_min_size[] =
 sizeof(VDAgentAudioVolumeSync), /* VD_AGENT_AUDIO_VOLUME_SYNC */
 };
 
+static void vdagent_message_clipboard_from_le(VDAgentMessage *message_header,
+uint8_t *data)
+{
+gsize min_size = vdagent_message_min_size[message_header->type];
+uint32_t *data_type = (uint32_t *) data;
+
+if (VD_AGENT_HAS_CAPABILITY(capabilities, capabilities_size,
+VD_AGENT_CAP_CLIPBOARD_SELECTION)) {
+min_size += 4;
+data_type++;
+}
+
+switch (message_header->type) {
+case VD_AGENT_CLIPBOARD_REQUEST:
+case VD_AGENT_CLIPBOARD:
+*data_type = GUINT32_FROM_LE(*data_type);
+break;
+case VD_AGENT_CLIPBOARD_GRAB:
+virtio_msg_uint32_from_le(data, message_header->size, min_size);
+break;
+default:
+g_warn_if_reached();
+}
+}
+
+static void vdagent_message_file_xfer_from_le(VDAgentMessage *message_header,
+uint8_t *data)
+{
+uint32_t *id = (uint32_t *)data;
+*id = GUINT32_FROM_LE(*id);
+id++; /* status */
+
+switch (message_header->type) {
+case VD_AGENT_FILE_XFER_DATA: {
+   VDAgentFileXferDataMessage *msg = (VDAgentFileXferDataMessage *)data;
+   msg->size = GUINT64_FROM_LE(msg->size);
+   break;
+}
+case VD_AGENT_FILE_XFER_STATUS:
+   *id = GUINT32_FROM_LE(*id); /* status */
+   break;
+

[Spice-devel] [vdagent-linux v8 0/4]

[Victor Toso]

From: Victor Toso 

Hi there!

This patches were also pending for some time and the patches were
distributed in two mail threads and for that reason I'm puting them
here in (another) thread :)

This first three patches were attached to Christophe's email [0], the
last patch is the v7 from Michal [1]. I did not change them in any way
at this moment.

[0] https://lists.freedesktop.org/archives/spice-devel/2017-January/035323.html
[1] https://lists.freedesktop.org/archives/spice-devel/2017-January/035303.html

I've only tested them in LE Fedora 25. I've only small comments to the
patches but IMHO these can be addressed before pushing if nobody
complains.

Cheers,
toso

Christophe Fergeau (2):
  Add missing size checks
  Adjust size checks

Michal Suchanek (1):
  vdagentd: Do endian swapping.

Victor Toso (1):
  vdagentd: early return on bad message size

 src/vdagentd/vdagentd.c| 236 +++--
 src/vdagentd/virtio-port.c |  36 ---
 2 files changed, 207 insertions(+), 65 deletions(-)

-- 
2.9.3

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

[Spice-devel] [vdagent-linux v8 2/4] Add missing size checks

[Victor Toso]

From: Christophe Fergeau 

---
 src/vdagentd/vdagentd.c | 14 ++
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
index ea8482b..8d43700 100644
--- a/src/vdagentd/vdagentd.c
+++ b/src/vdagentd/vdagentd.c
@@ -391,6 +391,8 @@ static gboolean vdagent_message_check_size(const 
VDAgentMessage *message_header)
 
 switch (message_header->type) {
 case VD_AGENT_MONITORS_CONFIG:
+case VD_AGENT_FILE_XFER_START:
+case VD_AGENT_FILE_XFER_DATA:
 case VD_AGENT_CLIPBOARD:
 case VD_AGENT_CLIPBOARD_GRAB:
 case VD_AGENT_CLIPBOARD_REQUEST:
@@ -404,21 +406,17 @@ static gboolean vdagent_message_check_size(const 
VDAgentMessage *message_header)
 }
 break;
 case VD_AGENT_MOUSE_STATE:
+case VD_AGENT_FILE_XFER_STATUS:
+case VD_AGENT_DISPLAY_CONFIG:
+case VD_AGENT_REPLY:
 case VD_AGENT_MAX_CLIPBOARD:
+case VD_AGENT_CLIENT_DISCONNECTED:
 if (message_header->size != min_size) {
 syslog(LOG_ERR, "read: invalid message size: %u for message type: 
%u",
message_header->size, message_header->type);
 return FALSE;
 }
 break;
-case VD_AGENT_FILE_XFER_START:
-case VD_AGENT_FILE_XFER_DATA:
-case VD_AGENT_FILE_XFER_STATUS:
-case VD_AGENT_CLIENT_DISCONNECTED:
-/* No size checks for these at the moment */
-break;
-case VD_AGENT_DISPLAY_CONFIG:
-case VD_AGENT_REPLY:
 default:
 g_warn_if_reached();
 return FALSE;
-- 
2.9.3

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

[Spice-devel] [vdagent-linux v8 1/4] vdagentd: early return on bad message size

[Victor Toso]

From: Victor Toso 

The payload size for each message should be the size of the expected
struct or bigger when it contain arrays of no fixed size.

This patch creates the vdagent_message_min_size[] static array with
the expected size for each message so we can check on
virtio_port_read_complete().

Signed-off-by: Victor Toso 
Signed-off-by: Michal Suchanek 
---
 src/vdagentd/vdagentd.c | 133 +---
 1 file changed, 91 insertions(+), 42 deletions(-)

diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
index 5ca0106..ea8482b 100644
--- a/src/vdagentd/vdagentd.c
+++ b/src/vdagentd/vdagentd.c
@@ -341,34 +341,109 @@ static void do_client_file_xfer(struct 
vdagent_virtio_port *vport,
 udscs_write(conn, msg_type, 0, 0, data, message_header->size);
 }
 
+static gsize vdagent_message_min_size[] =
+{
+-1, /* Does not exist */
+sizeof(VDAgentMouseState), /* VD_AGENT_MOUSE_STATE */
+sizeof(VDAgentMonitorsConfig), /* VD_AGENT_MONITORS_CONFIG */
+sizeof(VDAgentReply), /* VD_AGENT_REPLY */
+sizeof(VDAgentClipboard), /* VD_AGENT_CLIPBOARD */
+sizeof(VDAgentDisplayConfig), /* VD_AGENT_DISPLAY_CONFIG */
+sizeof(VDAgentAnnounceCapabilities), /* VD_AGENT_ANNOUNCE_CAPABILITIES */
+sizeof(VDAgentClipboardGrab), /* VD_AGENT_CLIPBOARD_GRAB */
+sizeof(VDAgentClipboardRequest), /* VD_AGENT_CLIPBOARD_REQUEST */
+sizeof(VDAgentClipboardRelease), /* VD_AGENT_CLIPBOARD_RELEASE */
+sizeof(VDAgentFileXferStartMessage), /* VD_AGENT_FILE_XFER_START */
+sizeof(VDAgentFileXferStatusMessage), /* VD_AGENT_FILE_XFER_STATUS */
+sizeof(VDAgentFileXferDataMessage), /* VD_AGENT_FILE_XFER_DATA */
+0, /* VD_AGENT_CLIENT_DISCONNECTED */
+sizeof(VDAgentMaxClipboard), /* VD_AGENT_MAX_CLIPBOARD */
+sizeof(VDAgentAudioVolumeSync), /* VD_AGENT_AUDIO_VOLUME_SYNC */
+};
+
+static gboolean vdagent_message_check_size(const VDAgentMessage 
*message_header)
+{
+uint32_t min_size = 0;
+
+if (message_header->protocol != VD_AGENT_PROTOCOL) {
+syslog(LOG_ERR, "message with wrong protocol version ignoring");
+return FALSE;
+}
+
+if (!message_header->type ||
+message_header->type >= G_N_ELEMENTS(vdagent_message_min_size)) {
+syslog(LOG_WARNING, "unknown message type %d, ignoring",
+   message_header->type);
+return FALSE;
+}
+
+min_size = vdagent_message_min_size[message_header->type];
+if (VD_AGENT_HAS_CAPABILITY(capabilities, capabilities_size,
+VD_AGENT_CAP_CLIPBOARD_SELECTION)) {
+switch (message_header->type) {
+case VD_AGENT_CLIPBOARD_GRAB:
+case VD_AGENT_CLIPBOARD_REQUEST:
+case VD_AGENT_CLIPBOARD:
+case VD_AGENT_CLIPBOARD_RELEASE:
+  min_size += 4;
+}
+}
+
+switch (message_header->type) {
+case VD_AGENT_MONITORS_CONFIG:
+case VD_AGENT_CLIPBOARD:
+case VD_AGENT_CLIPBOARD_GRAB:
+case VD_AGENT_CLIPBOARD_REQUEST:
+case VD_AGENT_CLIPBOARD_RELEASE:
+case VD_AGENT_AUDIO_VOLUME_SYNC:
+case VD_AGENT_ANNOUNCE_CAPABILITIES:
+if (message_header->size < min_size) {
+syslog(LOG_ERR, "read: invalid message size: %u for message type: 
%u",
+   message_header->size, message_header->type);
+return FALSE;
+}
+break;
+case VD_AGENT_MOUSE_STATE:
+case VD_AGENT_MAX_CLIPBOARD:
+if (message_header->size != min_size) {
+syslog(LOG_ERR, "read: invalid message size: %u for message type: 
%u",
+   message_header->size, message_header->type);
+return FALSE;
+}
+break;
+case VD_AGENT_FILE_XFER_START:
+case VD_AGENT_FILE_XFER_DATA:
+case VD_AGENT_FILE_XFER_STATUS:
+case VD_AGENT_CLIENT_DISCONNECTED:
+/* No size checks for these at the moment */
+break;
+case VD_AGENT_DISPLAY_CONFIG:
+case VD_AGENT_REPLY:
+default:
+g_warn_if_reached();
+return FALSE;
+}
+return TRUE;
+}
+
 static int virtio_port_read_complete(
 struct vdagent_virtio_port *vport,
 int port_nr,
 VDAgentMessage *message_header,
 uint8_t *data)
 {
-uint32_t min_size = 0;
-
-if (message_header->protocol != VD_AGENT_PROTOCOL) {
-syslog(LOG_ERR, "message with wrong protocol version ignoring");
+if (!vdagent_message_check_size(message_header))
 return 0;
-}
 
 switch (message_header->type) {
 case VD_AGENT_MOUSE_STATE:
-if (message_header->size != sizeof(VDAgentMouseState))
-goto size_error;
 do_client_mouse(, (VDAgentMouseState *)data);
 break;
 case VD_AGENT_MONITORS_CONFIG:
-if (message_header->size < sizeof(VDAgentMonitorsConfig))
-goto size_error;
 do_client_monitors(vport, port_nr, message_header,
  

[Spice-devel] [vdagent-linux v8 3/4] Adjust size checks

[Victor Toso]

From: Christophe Fergeau 

---
 src/vdagentd/vdagentd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
index 8d43700..2329489 100644
--- a/src/vdagentd/vdagentd.c
+++ b/src/vdagentd/vdagentd.c
@@ -395,8 +395,6 @@ static gboolean vdagent_message_check_size(const 
VDAgentMessage *message_header)
 case VD_AGENT_FILE_XFER_DATA:
 case VD_AGENT_CLIPBOARD:
 case VD_AGENT_CLIPBOARD_GRAB:
-case VD_AGENT_CLIPBOARD_REQUEST:
-case VD_AGENT_CLIPBOARD_RELEASE:
 case VD_AGENT_AUDIO_VOLUME_SYNC:
 case VD_AGENT_ANNOUNCE_CAPABILITIES:
 if (message_header->size < min_size) {
@@ -409,6 +407,8 @@ static gboolean vdagent_message_check_size(const 
VDAgentMessage *message_header)
 case VD_AGENT_FILE_XFER_STATUS:
 case VD_AGENT_DISPLAY_CONFIG:
 case VD_AGENT_REPLY:
+case VD_AGENT_CLIPBOARD_REQUEST:
+case VD_AGENT_CLIPBOARD_RELEASE:
 case VD_AGENT_MAX_CLIPBOARD:
 case VD_AGENT_CLIENT_DISCONNECTED:
 if (message_header->size != min_size) {
-- 
2.9.3

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

Re: [Spice-devel] [spice-server 2/2] gstreamer: Add #ifdef around zero_copy()

[Pavel Grunt]

Hi,

On Wed, 2017-02-15 at 09:39 +0100, Christophe Fergeau wrote:
> Since c3d237 "gstreamer: Avoid memory copy if strides are different"
> is
> only needed when zero copy is disabled.
> ---
>  server/gstreamer-encoder.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> index dd2926d..bb41425 100644
> --- a/server/gstreamer-encoder.c
> +++ b/server/gstreamer-encoder.c
> @@ -1141,6 +1141,7 @@ static int is_chunk_stride_aligned(const
> SpiceBitmap *bitmap, uint32_t index)
>  }
>  
>  /* A helper for push_raw_frame() */
> +#ifndef DO_ZERO_COPY
>  static inline int line_copy(SpiceGstEncoder *encoder, const
> SpiceBitmap *bitmap,
>  uint32_t chunk_offset, uint32_t
> stream_stride,
>  uint32_t height, uint8_t *buffer)
> @@ -1169,6 +1170,7 @@ static inline int line_copy(SpiceGstEncoder
> *encoder, const SpiceBitmap *bitmap,
>   spice_return_val_if_fail(dst - buffer == stream_stride *
> height, FALSE);
>   return TRUE;
>  }
> +#endif
>  
>  #ifdef DO_ZERO_COPY
Here we can have #else instead of #endif + #ifdef 

Pavel

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

Re: [Spice-devel] [spice-server 1/2] gstreamer: Remove unused function

[Pavel Grunt]

On Wed, 2017-02-15 at 09:39 +0100, Christophe Fergeau wrote:
> rate_control_is_active() is static and not used in gstreamer-
> encoder.c

Yeah, not needed since 97fcad82eb7d1f3bbd8d1163801b5a3db92944c2

Acked-by: Pavel Grunt 

> ---
>  server/gstreamer-encoder.c | 5 -
>  1 file changed, 5 deletions(-)
> 
> diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
> index 5c60a64..dd2926d 100644
> --- a/server/gstreamer-encoder.c
> +++ b/server/gstreamer-encoder.c
> @@ -347,11 +347,6 @@ static uint32_t
> get_network_latency(SpiceGstEncoder *encoder)
>  encoder->cbs.get_roundtrip_ms(encoder->cbs.opaque) / 2 : 0;
>  }
>  
> -static inline int rate_control_is_active(SpiceGstEncoder* encoder)
> -{
> -return encoder->cbs.get_roundtrip_ms != NULL;
> -}
> -
>  static void set_pipeline_changes(SpiceGstEncoder *encoder, uint32_t
> flags)
>  {
>  encoder->set_pipeline |= flags;
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [spice-server 2/2] gstreamer: Add #ifdef around zero_copy()

[Christophe Fergeau]

Since c3d237 "gstreamer: Avoid memory copy if strides are different" is
only needed when zero copy is disabled.
---
 server/gstreamer-encoder.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index dd2926d..bb41425 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -1141,6 +1141,7 @@ static int is_chunk_stride_aligned(const SpiceBitmap 
*bitmap, uint32_t index)
 }
 
 /* A helper for push_raw_frame() */
+#ifndef DO_ZERO_COPY
 static inline int line_copy(SpiceGstEncoder *encoder, const SpiceBitmap 
*bitmap,
 uint32_t chunk_offset, uint32_t stream_stride,
 uint32_t height, uint8_t *buffer)
@@ -1169,6 +1170,7 @@ static inline int line_copy(SpiceGstEncoder *encoder, 
const SpiceBitmap *bitmap,
  spice_return_val_if_fail(dst - buffer == stream_stride * height, FALSE);
  return TRUE;
 }
+#endif
 
 #ifdef DO_ZERO_COPY
 typedef struct {
-- 
2.9.3

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

[Spice-devel] [spice-server 1/2] gstreamer: Remove unused function

[Christophe Fergeau]

rate_control_is_active() is static and not used in gstreamer-encoder.c
---
 server/gstreamer-encoder.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/server/gstreamer-encoder.c b/server/gstreamer-encoder.c
index 5c60a64..dd2926d 100644
--- a/server/gstreamer-encoder.c
+++ b/server/gstreamer-encoder.c
@@ -347,11 +347,6 @@ static uint32_t get_network_latency(SpiceGstEncoder 
*encoder)
 encoder->cbs.get_roundtrip_ms(encoder->cbs.opaque) / 2 : 0;
 }
 
-static inline int rate_control_is_active(SpiceGstEncoder* encoder)
-{
-return encoder->cbs.get_roundtrip_ms != NULL;
-}
-
 static void set_pipeline_changes(SpiceGstEncoder *encoder, uint32_t flags)
 {
 encoder->set_pipeline |= flags;
-- 
2.9.3

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

Re: [Spice-devel] [spice-server v2 14/14] rcc: Consistently name RedChannelClient 'rcc'

[Frediano Ziglio]

> 
> On Tue, Feb 14, 2017 at 12:14:56PM -0600, Jonathon Jongsma wrote:
> > To be honest, I could go either way on this one. I like the
> > consistency, but I don't like the fact that it makes things like "git
> > blame" less useful. I'll leave the decision to others ;)
> 
> I've pushed the series without this patch for now, 'self' is only used
> in GObject boilerplate, so the inconsistency is not too bad (at least
> the patch series itself did not add more inconsistencies this time).
> On the other hand, 'tig blame' and the ',' shortcut help a lot making
> git blame useful even with such commits.
> 
> Christophe
> 

I was commenting out about self myself. Yes, I think that the lines
will affect GObject patch so it's not going to break blame that much.

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

Re: [Spice-devel] [PATCH spice-server] smartcard: Remove a "rename" function

[Christophe Fergeau]

Acked-by: Christophe Fergeau 

On Fri, Feb 10, 2017 at 02:30:57PM +, Frediano Ziglio wrote:
> smartcard_channel_client_pipe_add_push was just calling
> red_channel_client_pipe_add_push without any cast or other
> changes.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/smartcard.c | 15 ---
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/server/smartcard.c b/server/smartcard.c
> index f4bc40d..a7cc614 100644
> --- a/server/smartcard.c
> +++ b/server/smartcard.c
> @@ -124,7 +124,6 @@ typedef struct RedMsgItem {
>  } RedMsgItem;
>  
>  static RedMsgItem *smartcard_get_vsc_msg_item(RedChannelClient *rcc, 
> VSCMsgHeader *vheader);
> -static void smartcard_channel_client_pipe_add_push(RedChannelClient *rcc, 
> RedPipeItem *item);
>  
>  static struct Readers {
>  uint32_t num;
> @@ -186,6 +185,9 @@ static RedPipeItem 
> *smartcard_read_msg_from_device(RedCharDevice *self,
>  return NULL;
>  }
>  
> +/* this is called from both device input and client input. since the device 
> is
> + * a usb device, the context is still the main thread (kvm_main_loop, timers)
> + * so no mutex is required. */
>  static void smartcard_send_msg_to_client(RedCharDevice *self,
>   RedPipeItem *msg,
>   RedClient *client)
> @@ -196,7 +198,7 @@ static void smartcard_send_msg_to_client(RedCharDevice 
> *self,
>  spice_assert(dev->priv->scc &&
>   red_channel_client_get_client(rcc) == client);
>  red_pipe_item_ref(msg);
> -smartcard_channel_client_pipe_add_push(rcc, msg);
> +red_channel_client_pipe_add_push(rcc, msg);
>  }
>  
>  static void smartcard_send_tokens_to_client(RedCharDevice *self,
> @@ -460,15 +462,6 @@ static void smartcard_channel_send_item(RedChannelClient 
> *rcc, RedPipeItem *item
>  red_channel_client_begin_send_message(rcc);
>  }
>  
> -/* this is called from both device input and client input. since the device 
> is
> - * a usb device, the context is still the main thread (kvm_main_loop, timers)
> - * so no mutex is required. */
> -static void smartcard_channel_client_pipe_add_push(RedChannelClient *rcc,
> -   RedPipeItem *item)
> -{
> -red_channel_client_pipe_add_push(rcc, item);
> -}
> -
>  static void smartcard_free_vsc_msg_item(RedPipeItem *base)
>  {
>  RedMsgItem *item = SPICE_UPCAST(RedMsgItem, base);
> -- 
> 2.9.3
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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