[Spice-devel] [spice-server 1/8] reds: Close sockets when failing to watch them

Currently if we fail to set up the watch waiting for accept() to be called on the socket, we still keep the network socket open even if we are not going to be able to use it. This commit makes sure it's closed a set to -1 when such a failure occurs. --- server/reds.c | 5 + 1 file changed, 5

[Spice-devel] [spice-server 0/8] Add test-listen test case

While working on some bug/new feature for SPICE, I added a test case for our spice_server_set_port/_set_tls/... API. While the work I did this for still needs some work, this test case should be good enough on its own. Christophe ___ Spice-devel

[Spice-devel] [spice-server 4/8] test-listen: Add test case port/address configuration

This test case will be testing the external spice-server API to configure the address/port it's listening on. For now it sets up a listening server, spawns a thread which is going to connect to that port, and check it gets the REDQ magic upon connection. It will be extended to test for Unix

[Spice-devel] [spice-server 2/8] build: Bump glib version

From spice-gtk b312ca08 commit: "At the moment: - Fedora 26 has 2.52 - Fedora 25 has 2.50 - Fedora 24 has 2.48 - CentOS 7 has 2.46 - Debian 9 has 2.50" RHEL6 only have 2.28, but glib 2.32 is only used in a test case at the moment. --- configure.ac | 4 ++-- 1 file

[Spice-devel] [spice-server 5/8] test-listen: Add connection attempt to non-open port

--- server/tests/test-listen.c | 46 +++--- 1 file changed, 43 insertions(+), 3 deletions(-) diff --git a/server/tests/test-listen.c b/server/tests/test-listen.c index 052dc0b8f..562f07487 100644 --- a/server/tests/test-listen.c +++

[Spice-devel] [spice-server 3/8] tests: basic-event-loop: Silence debug message

There is currently a debug printf which is always shown when a mainloop event is triggered. This is unlikely to be useful unless one is debugging the event loop code. --- server/tests/basic-event-loop.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git

[Spice-devel] [spice-server 6/8] test-listen: Add event loop helpers

These factor a bit of common code, and more importantly, help with freeing all event loop related data at the end of each test. --- server/tests/test-listen.c | 120 - 1 file changed, 86 insertions(+), 34 deletions(-) diff --git

[Spice-devel] [spice-server 7/8] test-listen: Add TLS test

--- server/tests/test-listen.c | 106 +++-- 1 file changed, 103 insertions(+), 3 deletions(-) diff --git a/server/tests/test-listen.c b/server/tests/test-listen.c index e88105eea..2a15df1ab 100644 --- a/server/tests/test-listen.c +++

Re: [Spice-devel] [PATCH 12/22] Add exception handling classes

On Fri, Mar 02, 2018 at 06:06:32PM +0100, Christophe de Dinechin wrote: > > FWIW, I also find myself in agreement with Frediano and Lukas on this point. > > OK. Maybe I misunderstood something. > > Do we agree that the case Frediano raised is someone trying: > > throw Error(“The value of

Re: [Spice-devel] [PATCH spice-streaming-agent v5 2/2] Implement handling of error messages from the server

On Mon, Mar 05, 2018 at 05:58:15PM +0100, Christophe de Dinechin wrote: > > Perhaps you lost the mails saying that the protocol structure don't and > > won't have internal padding. > > Only on x86. It has padding on any ABI with a natural 64-bit alignment. > > I don’t have an Itanium handy, but

Re: [Spice-devel] [PATCH spice-common] build: Remove FIXME_SERVER_SMARTCARD hack

On Mon, Mar 05, 2018 at 11:19:05AM -0300, Eduardo Lima (Etrunko) wrote: > On 05/03/18 08:03, Christophe Fergeau wrote: > > On Fri, Mar 02, 2018 at 12:14:29PM -0300, Eduardo Lima (Etrunko) wrote: > >> When we remove the hacks in configure.ac and common/Makefile.am, two &

Re: [Spice-devel] [PATCH spice-server v2 6/6] stream-channel: Send the full frame in a single message

On Wed, Feb 28, 2018 at 08:30:33AM +, Frediano Ziglio wrote: > The current implementation of server and client assume that a single > data message contains an encoded frame. > This is not a problem for most encoding but for MJPEG this causes > the client to fail decoding. This commit really

Re: [Spice-devel] [PATCH spice-server v2 0/6] stream-device tests and stability patches

I still feel some of the logs could be a bit more detailed, but ack for patches 1 to 5. Christophe On Wed, Feb 28, 2018 at 08:30:27AM +, Frediano Ziglio wrote: > These patches fix some problem with streaming device adding also tests > for different issues. > Most of the patches was sent

Re: [Spice-devel] [PATCH spice-server 3/8] stream-device: Implements properly device reset on open/close

On Tue, Feb 27, 2018 at 06:20:32AM -0500, Frediano Ziglio wrote: > > > > On Sun, Feb 18, 2018 at 06:58:09PM +, Frediano Ziglio wrote: > > > Due to the way Qemu handle the device we must consume all pending > > > data inside the callback to read data from the device. > > > > "handles the

[Spice-devel] [spice-streaming-agent v2] build: Don't use -fvisibility when building the agent

In my testing (x86_64/gcc), this had no impact on the resulting binary, building with/without it gives the same stripped binary save for its buildid. The check for -fvisibility in configure.ac is kept as this is going to be useful when we ship dlopen'ed plugins. Signed-off-by: Christophe Fergeau

Re: [Spice-devel] [PATCH spice-common] build: Remove FIXME_SERVER_SMARTCARD hack

On Fri, Mar 02, 2018 at 12:14:29PM -0300, Eduardo Lima (Etrunko) wrote: > When we remove the hacks in configure.ac and common/Makefile.am, two > errors pop out: > > generated_server_demarshallers.c: In function > ‘parse_msgc_smartcard_reader_add’: > generated_server_demarshallers.c:1985:30:

Re: [Spice-devel] [PATCH 12/22] Add exception handling classes

On Fri, Mar 02, 2018 at 02:00:23PM +0100, Christophe de Dinechin wrote: > > > > On 2 Mar 2018, at 09:03, Frediano Ziglio wrote: > > > >>> > >>> On 1 Mar 2018, at 13:13, Frediano Ziglio wrote: > >>> > > From: Christophe de Dinechin

Re: [Spice-devel] [spice-streaming-agent 3/6] build: Remove -fvisibility detection from configure.ac

On Fri, Mar 02, 2018 at 02:27:57AM -0500, Frediano Ziglio wrote: > > > > In my testing (x86_64/gcc), this had no impact on the resulting binary, > > building with/without it gives the same stripped binary save for its > > buildid. > > > > Signed-off-by: Chr

Re: [Spice-devel] [PATCH 10/22] Remove client_codecs global variable, moved inside the 'Stream' class

On Thu, Mar 01, 2018 at 08:28:12PM +0100, Christophe de Dinechin wrote: > > > > On 1 Mar 2018, at 11:51, Lukáš Hrázký wrote: > > > > On Wed, 2018-02-28 at 16:43 +0100, Christophe de Dinechin wrote: > >> From: Christophe de Dinechin > >> > >> It makes

Re: [Spice-devel] [spice-streaming-agent 4/6] build: Improve BINDIR doc string

On Thu, Mar 01, 2018 at 10:03:13AM -0600, Jonathon Jongsma wrote: > On Thu, 2018-03-01 at 16:41 +0100, Christophe de Dinechin wrote: > > > On 1 Mar 2018, at 16:27, Christophe Fergeau <cferg...@redhat.com> > > > wrote: > > > > > > Signed-of

[Spice-devel] [spice-streaming-agent v2] build: Add --enable-tests

Tests require 'catch' to be installed, one might want to disable them if catch is not available. This patch adds a --disable-tests switch. By default, tests are enabled depending on 'catch' availability. Signed-off-by: Christophe Fergeau <cferg...@redhat.com> --- configure.ac

Re: [Spice-devel] [PATCH] build: Add autogen.sh convenience script

Hey, I sent https://lists.freedesktop.org/archives/spice-devel/2018-March/042479.html (which is based on spice-gtk's autogen.sh) which should address the issues below. Christophe On Fri, Feb 16, 2018 at 03:28:41PM +, Daniel P. Berrangé wrote: > On Fri, Feb 16, 2018 at 04:18:19PM +0100,

[Spice-devel] [spice-streaming-agent 2/6] build: Use pkgconfig to detect libjpeg

Upstream provides a .pc file, we can use it rather than doing the detection manually. Signed-off-by: Christophe Fergeau <cferg...@redhat.com> --- configure.ac | 14 +- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/configure.ac b/configure.ac index 5aab662..e

[Spice-devel] [spice-streaming-agent 1/6] build: Fix VPATH build

The .desktop file is generated, so will be in $(builddir), not $(srcdir) Signed-off-by: Christophe Fergeau <cferg...@redhat.com> --- Makefile.am | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile.am b/Makefile.am index 74ec16a..94ad7aa 100644 --- a/Makefile.am

[Spice-devel] [spice-streaming-agent 4/6] build: Improve BINDIR doc string

Signed-off-by: Christophe Fergeau <cferg...@redhat.com> --- configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 2ab14da..ce83153 100644 --- a/configure.ac +++ b/configure.ac @@ -50,7 +50,7 @@ LIBVIRT_LINKER_NO_INDIRECT AC

[Spice-devel] [spice-streaming-agent 6/6] build: Add autogen.sh script

Convenience script to avoid running autoreconf/configure manually. It can be run from a directory out of the source tree for VPATH builds. This script is based off the one in spice-gtk. Signed-off-by: Christophe Fergeau <cferg...@redhat.com> --- autogen.sh | 18 ++ 1 file c

[Spice-devel] [spice-streaming-agent 0/6] build: Misc configure.ac improvements

Hey, while looking at adding a --disable-tests argument as discussed recently on the mailing list, I piled up a couple of unrelated changes to configure.ac/Makefile.am. Christophe ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org

[Spice-devel] [spice-streaming-agent 3/6] build: Remove -fvisibility detection from configure.ac

In my testing (x86_64/gcc), this had no impact on the resulting binary, building with/without it gives the same stripped binary save for its buildid. Signed-off-by: Christophe Fergeau <cferg...@redhat.com> --- configure.ac| 19 --- src/Makefile.am | 2 -- 2 files chang

[Spice-devel] [spice-streaming-agent 5/6] build: Add --enable-tests

Tests are enabled by default, but are expensive to compile, so they can be disabled if that's what one wants. They will also be enabled/disabled by default depending on the availability of 'catch' Signed-off-by: Christophe Fergeau <cferg...@redhat.com> --- configure.ac

Re: [Spice-devel] [PATCH spice-streaming-agent] Only build unit tests when running make check

On Wed, Feb 28, 2018 at 01:33:35PM +0100, Lukáš Hrázký wrote: > On Wed, 2018-02-28 at 12:19 +0100, Christophe Fergeau wrote: > > On Wed, Feb 28, 2018 at 11:56:07AM +0100, Lukáš Hrázký wrote: > > > On Tue, 2018-02-27 at 18:02 +0100, Christophe Fergeau wrote: > > > >

Re: [Spice-devel] [PATCH 02/22] Reorder headers according to style guide

The style guide also recommends to put config.h there. Christophe On Wed, Feb 28, 2018 at 04:43:05PM +0100, Christophe de Dinechin wrote: > From: Christophe de Dinechin > > Signed-off-by: Christophe de Dinechin > --- > src/spice-streaming-agent.cpp |

Re: [Spice-devel] [PATCH spice-streaming-agent v2] Remove useless warning

Acked-by: Christophe Fergeau <cferg...@redhat.com> On Wed, Feb 28, 2018 at 10:02:27AM +, Frediano Ziglio wrote: > Fields are initialized to zero if not explicitly specified which is > actually what you usually want so there's no reason to have the > compiler to g

Re: [Spice-devel] [PATCH 12/22] Add exception handling classes

On Wed, Feb 28, 2018 at 04:43:15PM +0100, Christophe de Dinechin wrote: > From: Christophe de Dinechin > > Throwing 'runtime_error' directly should be reserved for the support > library. Add an 'Error' class as a base class for all errors thrown > by the streaming agent, as

Re: [Spice-devel] [PATCH 01/22] Eliminate signed/unsigned warning

https://lists.freedesktop.org/archives/spice-devel/2018-February/042080.html On Wed, Feb 28, 2018 at 04:43:04PM +0100, Christophe de Dinechin wrote: > From: Christophe de Dinechin > > Signed-off-by: Christophe de Dinechin > --- >

Re: [Spice-devel] [PATCH 06/22] Get rid of C-style memset initializations, use C++ style aggregates

My understanding is that the previous iteration was quite controversial, I would just drop it from the series unless you get acks from everyone involved this time. Christophe On Wed, Feb 28, 2018 at 04:43:09PM +0100, Christophe de Dinechin wrote: > From: Christophe de Dinechin

Re: [Spice-devel] [PATCH 03/22] Reformat 'if' statments according to style guide

s/statments/statements in the short log. On Wed, Feb 28, 2018 at 04:43:06PM +0100, Christophe de Dinechin wrote: > From: Christophe de Dinechin > > This patch ensures that all 'if' statements have braces, according to > style guide, so that when the follow-up patches

Re: [Spice-devel] [PATCH spice-streaming-agent] Only build unit tests when running make check

On Wed, Feb 28, 2018 at 11:56:07AM +0100, Lukáš Hrázký wrote: > On Tue, 2018-02-27 at 18:02 +0100, Christophe Fergeau wrote: > > On Tue, Feb 27, 2018 at 01:04:31PM +0100, Lukáš Hrázký wrote: > > > On Tue, 2018-02-27 at 06:33 -0500, Frediano Ziglio wrote: > > &

Re: [Spice-devel] [PATCH spice-streaming-agent] Only build unit tests when running make check

On Tue, Feb 27, 2018 at 01:04:31PM +0100, Lukáš Hrázký wrote: > On Tue, 2018-02-27 at 06:33 -0500, Frediano Ziglio wrote: > > > > > > Removing noinst_PROGRAMS from src/unittests/Makefile.am will cause the > > > unit tests not be built on a regular build (`make`), they will only be > > > built

Re: [Spice-devel] [PATCH spice-server 7/8] test-stream-device: Check we don't read past data message

Acked-by: Christophe Fergeau <cferg...@redhat.com> On Sun, Feb 18, 2018 at 06:58:13PM +, Frediano Ziglio wrote: > Test case for the issue fixed by previous commit. > > Signed-off-by: Frediano Ziglio <fzig...@redhat.com> > --- > server/test

Re: [Spice-devel] [PATCH spice-server 6/8] stream-device: Do not read past data message

evice loses the sync with the guest." > The actual Qemu and agent implementation avoids it but better > to avoid it. "The actual Qemu and streaming agent implementations avoid it, but better to make sure this can't happen in the server code too." Acked-by: Christophe Fergeau &

Re: [Spice-devel] [PATCH spice-server 5/8] stream-device: Workaround Qemu bug closing device

On Sun, Feb 18, 2018 at 06:58:11PM +, Frediano Ziglio wrote: > Previous patch causes a bug into Qemu if the patch "in Qemu" rather than "into" > 46764fe09ca2e0f15c0981a672c166ed8cf57e72 ("virtio-serial: fix segfault > on disconnect") is not include in that version of Qemu. "included". I'd

Re: [Spice-devel] [PATCH spice-server 4/8] stream-device: Disable guest device on errors

ar_device_reset(char_dev); > reset_channels(dev); > + > +// enable the device again. We try to enable it even on close as > +// this could prevent a possible race condition where we open the > +// device from the guest and it take some time to enable causing > +

Re: [Spice-devel] [PATCH spice-server 5/8] stream-device: Workaround Qemu bug closing device

On Sun, Feb 18, 2018 at 06:58:11PM +, Frediano Ziglio wrote: > Previous patch causes a bug into Qemu if the patch > 46764fe09ca2e0f15c0981a672c166ed8cf57e72 ("virtio-serial: fix segfault > on disconnect") is not include in that version of Qemu. > This crash happens when device is closed inside

Re: [Spice-devel] [PATCH spice-server 3/8] stream-device: Implements properly device reset on open/close

On Sun, Feb 18, 2018 at 06:58:09PM +, Frediano Ziglio wrote: > Due to the way Qemu handle the device we must consume all pending > data inside the callback to read data from the device. "handles the device, we must .." "... the callback which reads data ..." ? I would add "when an error

Re: [Spice-devel] [PATCH spice-server 2/8] test-stream-device: Test batched multiple messages

Ok, some of the same comments from previous patch apply. Acked-by: Christophe Fergeau <cferg...@redhat.com> On Sun, Feb 18, 2018 at 06:58:08PM +, Frediano Ziglio wrote: > Test all batched (send together) messages are handled correctly > and device is not stuck. > > Signe

Re: [Spice-devel] [PATCH spice-server 1/8] test-stream-device: Better Qemu emulation for data reading

Acked-by: Christophe Fergeau <cferg...@redhat.com> couple of comments on the comments below On Sun, Feb 18, 2018 at 06:58:07PM +, Frediano Ziglio wrote: > Qemu does not trigger a new data read if we don't read all data in > the buffer. > > Signed-off-by: Frediano Ziglio &

Re: [Spice-devel] [PATCH 07/17] Get rid of C-style memset initializations, use C++ style aggregates

On Fri, Feb 23, 2018 at 12:01:59PM +0100, Christophe de Dinechin wrote: > > > > On 23 Feb 2018, at 10:53, Christophe Fergeau <cferg...@redhat.com> wrote: > > > > Given the lengthy debate over what is mostly a small cosmetic patch, I > > suggest that we postpo

Re: [Spice-devel] [PATCH spice-streaming-agent v3] log_binary is really a boolean

On Fri, Feb 23, 2018 at 11:42:42AM +0100, Christophe de Dinechin wrote: > The suggestion about nacking commits that don’t have a long log seems a bit > extreme for one-line cleanups. This patch is not a one-line cleanup, of course I would not blindly nack trivial/self explanatory patches, but I

Re: [Spice-devel] [PATCH spice-protocol] stream-device: Specify how padding shoud be inside new structures

On Fri, Feb 23, 2018 at 10:11:46AM +, Frediano Ziglio wrote: > Depending on how structures are initialised in the code is > possible that implicit padding bytes are not initialised > causing possible information leaks as the entire structure > with all padding is sent through device/network. >

Re: [Spice-devel] [PATCH spice-streaming-agent v3] log_binary is really a boolean

On Fri, Feb 23, 2018 at 08:18:52AM +0100, Christophe de Dinechin wrote: > > > On Feb 23, 2018, at 8:07 AM, Frediano Ziglio wrote: > > > > From: Christophe de Dinechin > > > > Signed-off-by: Christophe de Dinechin > > --- > >

Re: [Spice-devel] [PATCH 07/17] Get rid of C-style memset initializations, use C++ style aggregates

Given the lengthy debate over what is mostly a small cosmetic patch, I suggest that we postpone this one for now and drop it from the series. Christophe On Fri, Feb 16, 2018 at 05:15:37PM +0100, Christophe de Dinechin wrote: > From: Christophe de Dinechin > >

Re: [Spice-devel] [PATCH spice-streaming-agent 0/1] Adding gstreamer based plugin

On Thu, Feb 22, 2018 at 06:20:50PM +0200, Snir Sheriber wrote: > This is gstreamer based plugin for spice streaming agent. This plugin > is considered experimental but can be very useful for testing purposes. > > Notes: > *It is currently uses sw h264 encoding (using x264enc plugin), may not >

Re: [Spice-devel] SUSE 91 / SUSE 93 supported xf86 Version

Hey, On Mon, Jan 29, 2018 at 08:24:59AM +0100, ralfbuhrm...@bundeswehr.org wrote: > Dear Spice-Developer, > > i'm a civilian Employer in the the German Navy and have a little Problem. > > We are using a Linux-Based Trainer Software using SUSE 9.1 and 9.3. With > our new Hardware, we have

Re: [Spice-devel] [PATCH spice-server v2 1/2] stream-device: handle cursor from device

I'm late to the party, but a one-line commit log for a patch adding 160 lines of code is *not* acceptable. Christophe On Fri, Feb 09, 2018 at 09:10:48AM +, Frediano Ziglio wrote: > Signed-off-by: Frediano Ziglio > --- > server/stream-device.c | 169 >

Re: [Spice-devel] broken spice dependancy

Hey, For what it's worth, inflamatory emails such as this one are not welcome on this list, and usually not the best way of getting help/feedback. gtk-doc should not be required for building from tarballs if you pass --disable-gtk-doc to configure. It even seems to be disabled by default on my

Re: [Spice-devel] Gitlab - 2018!

On Mon, Feb 19, 2018 at 04:15:07PM +0100, Victor Toso wrote: > Hi, > > There was a thread in 2016 about moving to gitlab [0] which it > was not complete done. New projects have been started in gitlab > instead of freedesktop and without notice, some users are even > filling bugs there already

Re: [Spice-devel] [spice-gtk] Add --spice-disable-clipboard option

On Tue, Feb 20, 2018 at 12:48:48PM +0100, Victor Toso wrote: > On Thu, Jan 18, 2018 at 10:31:26AM +0100, Christophe Fergeau wrote: > > At least on X.org, malicious code could run the equivalent of "watch > > xsel -o --clipboard" in a VM, and would then be able to tr

Re: [Spice-devel] [PATCH spice-streaming-agent v3 3/3] mjpeg-fallback: unittest for the options parsing

On Mon, Feb 19, 2018 at 09:19:18PM +0200, Uri Lublin wrote: > If users prefer to not run autogen.sh that's ok. > It provides defaults options for developers. > For example, I do not expect users to run configure with > --enable-maintainer-mode too. NB: My understanding of

Re: [Spice-devel] [spice-gtk] Add --spice-disable-clipboard option

On Thu, Jan 18, 2018 at 12:13:45PM +0100, Christophe Fergeau wrote: > On Thu, Jan 18, 2018 at 12:06:34PM +0100, Marc-André Lureau wrote: > > Hi > > > > On Thu, Jan 18, 2018 at 10:31 AM, Christophe Fergeau > > <cferg...@redhat.com> wrote: > > > A

Re: [Spice-devel] [PATCH 2/2] Eliminate signed/unsigned warning

On Mon, Feb 19, 2018 at 03:06:40PM +0100, Christophe de Dinechin wrote: > > > > On 19 Feb 2018, at 13:24, Christophe Fergeau <cferg...@redhat.com> wrote: > > > > On Fri, Feb 16, 2018 at 04:23:06PM +0100, Christophe de Dinechin wrote: > >> From: Chri

Re: [Spice-devel] [PATCH spice-server v3 0/2] Implement cursor for streaming device

On Tue, Feb 13, 2018 at 01:58:07PM +, Frediano Ziglio wrote: > Changes since v2: > - shrink buffer after the message is handled. > > Frediano Ziglio (2): > stream-device: handle cursor from device > stream-device: Implement mouse movement > > server/stream-device.c | 201 >

Re: [Spice-devel] [spice-gtk v1 2/2] channel-display: use libva to set preferred video codecs

On Thu, Feb 15, 2018 at 10:05:05AM +0100, Victor Toso wrote: > From: Victor Toso > > By using the detection of which video codecs (and profiles!) the > hardware supports with VA-API capable driver in previous commit. > > Signed-off-by: Victor Toso >

Re: [Spice-devel] [spice-gtk v1 1/2] Use libva to check video hardware accel capabilities

Hey, On Thu, Feb 15, 2018 at 10:05:04AM +0100, Victor Toso wrote: > From: Victor Toso > > Libva is an implementation for VA-API. > > This can be used to automatically send to the server the > preferred video codecs as the client would prefer streams > with video codecs

Re: [Spice-devel] #include "canvas_base.c" ?

On Thu, Feb 15, 2018 at 03:28:42PM +0100, Christophe de Dinechin wrote: > While looking at some warnings, I came across this in sw_canvas.c: > > #include “canvas_base.c" > > So we include a .c in another one. Apparently, this was inherited from some > Cairo canvas. Is that something we care to

Re: [Spice-devel] [RFC PATCH 1/1] separate and encapsulate the agent business code

On Wed, Feb 14, 2018 at 10:13:41PM +0100, Christophe de Dinechin wrote: > >> I’d write “config.h”. No reason to ever look config.h in system headers. > > > > The reason for the <> is described in [1], 4th paragraph. I've > > mentioned it during the previous discussion and didn't get any comment >

Re: [Spice-devel] [PATCH spice-streaming-agent v2 2/4] Remove clang warning on missing 'override'

On Thu, Feb 15, 2018 at 03:58:15PM +0100, Christophe de Dinechin wrote: > Again, I really don’t want to have to post-process my patches manually > to split-out whitespace corrections. I see that as “punishment for > doing the right thing”. This is also a matter of differing workflows, I mostly

Re: [Spice-devel] [PATCH spice-gtk v2 2/4] uri: learn to parse spice+tls:// form

On Fri, Feb 16, 2018 at 10:30:18AM +, Daniel P. Berrangé wrote: > On Fri, Feb 16, 2018 at 11:13:06AM +0100, marcandre.lur...@redhat.com wrote: > > From: Marc-André Lureau > > > > spice:// has a weird scheme encoding, where it can accept both plain > > and tls

Re: [Spice-devel] [PATCH 2/2] Eliminate signed/unsigned warning

On Fri, Feb 16, 2018 at 04:23:06PM +0100, Christophe de Dinechin wrote: > From: Christophe de Dinechin > > Without this, GCC complains about signed / unsigned comparisons: > > mjpeg-fallback.cpp:121:24: warning: comparison between signed and unsigned > integer expressions

Re: [Spice-devel] [PATCH 03/14] streaming_requested is really a bool

On Wed, Feb 14, 2018 at 06:52:11PM +0100, Christophe de Dinechin wrote: > From: Christophe de Dinechin > > Signed-off-by: Christophe de Dinechin > --- > src/spice-streaming-agent.cpp | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > >

Re: [Spice-devel] [PATCH v4 04/12] Rephrase assertion section

On Thu, Feb 15, 2018 at 05:06:17PM +0100, Christophe de Dinechin wrote: > From: Christophe de Dinechin > > Changes since v3: > - Integrate comments about performance impact and solution > - Integrate comments about impact of a failed assertion > - Add prohibition against

Re: [Spice-devel] [PATCH v4 04/12] Rephrase assertion section

On Fri, Feb 16, 2018 at 10:40:58AM +0100, Christophe de Dinechin wrote: > > > > On 16 Feb 2018, at 10:12, Christophe Fergeau <cferg...@redhat.com> wrote: > > > > On Thu, Feb 15, 2018 at 05:06:17PM +0100, Christophe de Dinechin wrote: > >> From: Chri

Re: [Spice-devel] [PATCH v4 12/12] Whitespace adjustment guideline

On Thu, Feb 15, 2018 at 05:06:25PM +0100, Christophe de Dinechin wrote: > From: Christophe de Dinechin > > Change since v3: > - This particular aspect is controversial, so it was put in a separate patch. This has been nack'ed by multiple people already, please don't resend

Re: [Spice-devel] [PATCH v4 10/12] Integrate suggestions from Christophe Fergeau

The shortlog is not really good.. On Thu, Feb 15, 2018 at 05:06:23PM +0100, Christophe de Dinechin wrote: > From: Christophe de Dinechin > > This attempts to find a wording that takes into account the existing > practice, notably in the server. > > Signed-off-by:

Re: [Spice-devel] [PATCH v4 08/12] Add guidelines about warnings

On Thu, Feb 15, 2018 at 05:06:21PM +0100, Christophe de Dinechin wrote: > From: Christophe de Dinechin > > The objective of these guidelines is that: > - We avoid introducing new warnings > - We know how to fix old ones > > Changes since v3: > - Put 'controversial'

Re: [Spice-devel] [PATCH v4 04/12] Rephrase assertion section

On Thu, Feb 15, 2018 at 05:06:17PM +0100, Christophe de Dinechin wrote: > From: Christophe de Dinechin > > Changes since v3: > - Integrate comments about performance impact and solution > - Integrate comments about impact of a failed assertion > - Add prohibition against

Re: [Spice-devel] [PATCH v4 01/12] Add .clang-format with defaults matching what's specified in the style guide

On Thu, Feb 15, 2018 at 05:06:14PM +0100, Christophe de Dinechin wrote: > From: Christophe de Dinechin > > There are two use cases for this .clang-format: > > 1. Local reformatting of source code, e.g. using Emacs clang-format.el > package. >This is basically a wawy to

Re: [Spice-devel] [PATCH v4 06/12] Rephrase section on short functions for readability

On Thu, Feb 15, 2018 at 05:06:19PM +0100, Christophe de Dinechin wrote: > From: Christophe de Dinechin > > Signed-off-by: Christophe de Dinechin > --- > docs/spice_style.txt | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git

Re: [Spice-devel] [spice-server] style: Slight tweak to the header guard section

On Thu, Feb 15, 2018 at 04:04:57PM +0100, Christophe de Dinechin wrote: > > This style guide only indicates what we aim to achieve. It does not > necessarily reflect the current state of the code. > > What about adding: > > Consistency matters. It may be preferable to ignore a

Re: [Spice-devel] [PATCH v3 11/11] Rewrite the style guide for headers

On Thu, Feb 15, 2018 at 03:25:08PM +0100, Christophe de Dinechin wrote: > > And then I made concrete suggestions > > as how I would move forward with this patch… > > Sorry, I saw your push back, but I did not see the concrete suggestion for > moving forward… Would you please be kind enough to

Re: [Spice-devel] [spice-server] style: Slight tweak to the header guard section

On Thu, Feb 15, 2018 at 03:25:23PM +0100, Christophe de Dinechin wrote: > > > > On 15 Feb 2018, at 13:41, Christophe Fergeau <cferg...@redhat.com> wrote: > > > > On Thu, Feb 15, 2018 at 11:55:44AM +0100, Christophe de Dinechin wrote: > >> Now, Christoph

Re: [Spice-devel] [PATCH v3 11/11] Rewrite the style guide for headers

On Thu, Feb 15, 2018 at 01:14:43PM +0100, Christophe de Dinechin wrote: > > > > On 15 Feb 2018, at 10:19, Christophe Fergeau <cferg...@redhat.com> wrote: > > > > On Wed, Feb 14, 2018 at 11:24:50PM +0100, Christophe de Dinechin wrote: > >> > >

Re: [Spice-devel] [spice-server] style: Slight tweak to the header guard section

On Thu, Feb 15, 2018 at 11:55:44AM +0100, Christophe de Dinechin wrote: > Now, Christophe’s arguments are that > > 1) we should not write guidelines that are inconsistent with existing code. > 2) this is in the server codebase, so we should server rules > > Problem is with 2, really. > > We

Re: [Spice-devel] [spice-server] style: Slight tweak to the header guard section

On Thu, Feb 15, 2018 at 10:56:49AM +0100, Lukáš Hrázký wrote: > On Wed, 2018-02-14 at 22:43 +0100, Christophe de Dinechin wrote: > > > On 14 Feb 2018, at 17:29, Christophe Fergeau <cferg...@redhat.com> wrote: > > > > > > This changes the suggested style to w

Re: [Spice-devel] [spice-server] style: Slight tweak to the header guard section

On Wed, Feb 14, 2018 at 10:43:26PM +0100, Christophe de Dinechin wrote: > > > > On 14 Feb 2018, at 17:29, Christophe Fergeau <cferg...@redhat.com> wrote: > > > > This changes the suggested style to what is currently used in > > spice-server codebase. Thi

Re: [Spice-devel] [PATCH v3 11/11] Rewrite the style guide for headers

On Wed, Feb 14, 2018 at 11:24:50PM +0100, Christophe de Dinechin wrote: > > > > On 14 Feb 2018, at 14:35, Christophe Fergeau <cferg...@redhat.com> wrote: > > > > This one sounds more like an RFC to me > > Well, this is really a bug fix in the documentati

Re: [Spice-devel] [PATCH v3 01/11] Add .clang-format with defaults matching what's specified in the style guide

On Wed, Feb 14, 2018 at 10:29:25PM +0100, Christophe de Dinechin wrote: > > > > On 14 Feb 2018, at 17:34, Christophe Fergeau <cferg...@redhat.com> wrote: > > > > On Wed, Feb 14, 2018 at 10:45:56AM -0500, Frediano Ziglio wrote: > >>> > >>&g

Re: [Spice-devel] [PATCH v3 10/11] Add guidelines about warnings and whitespaces

On Wed, Feb 14, 2018 at 10:53:04PM +0100, Christophe de Dinechin wrote: > > > > On 14 Feb 2018, at 14:37, Christophe Fergeau <cferg...@redhat.com> wrote: > > > > On Thu, Feb 08, 2018 at 12:25:30PM +0100, Christophe de Dinechin wrote: > >> From: Chri

Re: [Spice-devel] [PATCH v2 12/13] Add guidelines about warnings and whitespaces

On Thu, Feb 08, 2018 at 11:09:35AM +0100, Victor Toso wrote: > Hi, > > On Thu, Feb 08, 2018 at 05:01:05AM -0500, Frediano Ziglio wrote: > > > > Depends on many cases. You don't want spurious changes to make harder to > > > > look at the history for instance (that is a point for Nack). > > > > The

Re: [Spice-devel] [PATCH spice-streaming-agent v3 3/3] mjpeg-fallback: unittest for the options parsing

On Wed, Feb 14, 2018 at 10:40:58AM -0500, Frediano Ziglio wrote: > > > > > On 14 Feb 2018, at 13:34, Lukáš Hrázký wrote: > > > > > > Introduce a unit test framework (Catch) to the codebase and a simple > > > unit test for parsing the options of the mjpeg plugin. > > > > > >

Re: [Spice-devel] [PATCH v3 01/11] Add .clang-format with defaults matching what's specified in the style guide

On Wed, Feb 14, 2018 at 10:45:56AM -0500, Frediano Ziglio wrote: > > > > Shouldn't this go with a Makefile rule? A few lines in the log what this > > is about, what is the goal for having this file, ... would not hurt. > > > > Christophe > > > > I think this file is supposed to just help

[Spice-devel] [spice-server] style: Slight tweak to the header guard section

This changes the suggested style to what is currently used in spice-server codebase. This also removes a few sentences which are not really related to how one should format their header guards. Signed-off-by: Christophe Fergeau <cferg...@redhat.com> --- docs/spice_style.txt | 6 ++

Re: [Spice-devel] [PATCH v3 09/11] Add mention of header guards

On Thu, Feb 08, 2018 at 12:25:29PM +0100, Christophe de Dinechin wrote: > From: Christophe de Dinechin > > Signed-off-by: Christophe de Dinechin > --- > docs/spice_style.txt | 19 +++ > 1 file changed, 19 insertions(+) > > diff --git

Re: [Spice-devel] [PATCH spice-streaming-agent v2 3/4] use std::string where it's easy to replace

On Thu, Feb 08, 2018 at 08:16:37AM +0100, Christophe de Dinechin wrote: > Using std::string by default is *not* considered a good practice in > C++. The reference for C++ good practices are probably best summarized > by the C++ Core Guidelines: >

Re: [Spice-devel] [PATCH spice-streaming-agent] build: Do not use top_srcdir if not necessary

On Mon, Jan 29, 2018 at 05:16:12PM -0500, Frediano Ziglio wrote: > > > > > > > On 29 Jan 2018, at 12:00, Frediano Ziglio wrote: > > > > > > Signed-off-by: Frediano Ziglio > > > --- > > > Makefile.am | 4 ++-- > > > 1 file changed, 2 insertions(+), 2

Re: [Spice-devel] [PATCH v3 10/11] Add guidelines about warnings and whitespaces

On Thu, Feb 08, 2018 at 12:25:30PM +0100, Christophe de Dinechin wrote: > From: Christophe de Dinechin > > The objective of these guidelines is that: > - We avoid introducing new warnings > - We know how to fix old ones > - We don't have to isolate whitespace changes when

Re: [Spice-devel] [PATCH v3 11/11] Rewrite the style guide for headers

This one sounds more like an RFC to me, as from a quick look in server/, this is not the style currently in use. Christophe On Thu, Feb 08, 2018 at 12:25:31PM +0100, Christophe de Dinechin wrote: > From: Christophe de Dinechin > > As written, the headers style guide looks

Re: [Spice-devel] [PATCH v3 03/11] Specify file extensions for C++ and Obj-C

On Mon, Feb 12, 2018 at 03:23:06AM -0500, Frediano Ziglio wrote: > See > https://lists.freedesktop.org/archives/spice-devel/2018-February/041724.html > > > From: Christophe de Dinechin > > > > Signed-off-by: Christophe de Dinechin > > --- > >

Re: [Spice-devel] [PATCH v2 11/13] Add mention of header guards

On Thu, Feb 08, 2018 at 03:58:46AM -0500, Frediano Ziglio wrote: > > + > > +[source,h] > > +--- > > +#ifndef MY_MODULE_H > > +#define MY_MODULE_H > > + > > +... > > + > > +#endif /* MY_MODULE_H */ > > +--- > > + > > Are we suggesting C style only comment or is clear the is not important? > (I'm

Re: [Spice-devel] [PATCH v2 06/13] Rephrase section on short functions for readability

On Thu, Feb 08, 2018 at 03:50:37AM -0500, Frediano Ziglio wrote: > > > > From: Christophe de Dinechin > > > > Signed-off-by: Christophe de Dinechin > > --- > > docs/spice_style.txt | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > >

Re: [Spice-devel] [PATCH v2 05/13] Rephrase section about constants

On Thu, Feb 08, 2018 at 03:48:46AM -0500, Frediano Ziglio wrote: > I think that the "Alternatively, use global `const` variables." > is quite an ancient comment when spice-server was in C++ > (much before I join and I think even older than the git repository!) There was a C++ *client* in

<    5   6   7   8   9   10   11   12   13   14   >