Re: [Spice-devel] [PATCH 12/17] Convert message writing from C style to C++ style

> On 23 Feb 2018, at 17:37, Jonathon Jongsma wrote: > > On Fri, 2018-02-23 at 12:36 +0100, Christophe de Dinechin wrote: >>> On 22 Feb 2018, at 19:05, Lukáš Hrázký wrote: >>> >>> On Thu, 2018-02-22 at 18:51 +0100, Christophe de Dinechin wrote: >

Re: [Spice-devel] [PATCH 12/17] Convert message writing from C style to C++ style

On Fri, 2018-02-23 at 12:36 +0100, Christophe de Dinechin wrote: > > On 22 Feb 2018, at 19:05, Lukáš Hrázký wrote: > > > > On Thu, 2018-02-22 at 18:51 +0100, Christophe de Dinechin wrote: > > > > On 21 Feb 2018, at 10:45, Lukáš Hrázký > > > > wrote: > > >

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

Acked-by: Christophe Fergeau 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 > --- > server/tests/test-stream-device.c | 42 >

Re: [Spice-devel] [RFC PATCH spice-server] stream-device: Create channels before first not main connection

On Fri, 2018-02-23 at 12:12 +0100, Christophe de Dinechin wrote: > > On 23 Feb 2018, at 10:24, Frediano Ziglio > > wrote: > > > > > > > > Title of the patch: “first not main” sounds un-english because it > > > mixes > > > adjective and noun-used-as-adjective. Contrast

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

On Sun, Feb 18, 2018 at 06:58:12PM +, Frediano Ziglio wrote: > If data message is followed by another message was theoretically > possible that device looses the sync with the guest. "If the data message is followed by another message, it's theoritically possible that the device loses the

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

On Sun, Feb 18, 2018 at 06:58:10PM +, Frediano Ziglio wrote: > To communicate the error and avoiding to have to read any data the > guest want to write disable the device. I am having troubles parsing this. Is this missing a comma before "disable"? "To communicate the error and avoiding to

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 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. > > Signed-off-by: Frediano

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

Acked-by: Christophe Fergeau 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 spice-streaming-agent 1/1] Adding gstreamer based plugin

Hi, On Fri, Feb 23, 2018 at 01:14:47PM +0100, Christophe de Dinechin wrote: > > +sink = gst_element_factory_make("appsink", "sink"); > > +if (!capture || !convert || !encoder || !sink) { > > +//TODO: check elements existence in build time (and\or improve > > error in runtime) >

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

Cool! > On 22 Feb 2018, at 17:20, Snir Sheriber wrote: > > Gstreamer based plugin utilizing gstreamer elements to capture > screen from X, convert and encode into h264 stream using x264enc > gstreamer plugin. > Configure with --with-gst, will be built as a separate plugin.

Re: [Spice-devel] [PATCH vdagent-win] vdagent: fix loss of mouse movement events

On Fri, Feb 16, 2018 at 03:05:39PM +0300, free.user.name wrote: > send_input() may not be immediately called from handle_mouse_event() on > movement. INPUT structure is generated and stored and a timer may be set > instead. If subsequent call to handle_mouse_event() occurs before timer > expires,

Re: [Spice-devel] [PATCH 12/17] Convert message writing from C style to C++ style

> On 22 Feb 2018, at 19:05, Lukáš Hrázký wrote: > > On Thu, 2018-02-22 at 18:51 +0100, Christophe de Dinechin wrote: >>> On 21 Feb 2018, at 10:45, Lukáš Hrázký wrote: >>> >>> On Tue, 2018-02-20 at 16:41 +0100, Christophe de Dinechin wrote: > On 20

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

> On 23 Feb 2018, at 12:08, Christophe Fergeau wrote: > > On Fri, Feb 23, 2018 at 12:01:59PM +0100, Christophe de Dinechin wrote: >> >> >>> On 23 Feb 2018, at 10:53, Christophe Fergeau wrote: >>> >>> Given the lengthy debate over what is mostly a

Re: [Spice-devel] [RFC PATCH spice-server] stream-device: Create channels before first not main connection

> On 23 Feb 2018, at 10:24, Frediano Ziglio wrote: > >> >> Title of the patch: “first not main” sounds un-english because it mixes >> adjective and noun-used-as-adjective. Contrast “first, not last, connexion” >> (with commas, I believe). Jonathon, any suggestion? Is

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 wrote: > > > > 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

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

> On 23 Feb 2018, at 11:47, Christophe Fergeau wrote: > > 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

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

> On 23 Feb 2018, at 10:53, Christophe Fergeau wrote: > > 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. memset in C++ code is not just a style issue, it’s dangerous. It

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

> On 22 Feb 2018, at 19:03, Frediano Ziglio wrote: > >> >> [Indeed, I had not sent this reply, took time to check standard and >> compilers] >> >>> On 20 Feb 2018, at 12:31, Frediano Ziglio wrote: >>> > On 20 Feb 2018, at 10:39, Lukáš Hrázký

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-streaming-agent v3] log_binary is really a boolean

> On 23 Feb 2018, at 10:58, Christophe Fergeau wrote: > > 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

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

> On 23 Feb 2018, at 11:31, Christophe Fergeau wrote: > > 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

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-protocol] stream-device: Specify how padding shoud be inside new structures

On that topic, curious why the structures are “naturally aligned”? Why aren’t they packed? > On 23 Feb 2018, at 11:11, Frediano Ziglio wrote: > > Depending on how structures are initialised in the code is > possible that implicit padding bytes are not initialised > causing

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

> On 23 Feb 2018, at 11:11, 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

Re: [Spice-devel] [RFC PATCH spice-streaming-agent] Remove paranoid warning

On Fri, Feb 23, 2018 at 10:15:47AM +, 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 generate warning for this. So, s/paranoid/useless/ ? > > --- >

Re: [Spice-devel] [RFC PATCH spice-streaming-agent] Remove paranoid warning

> On 23 Feb 2018, at 11:15, 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 generate warning for this. > > --- > m4/spice-compile-warnings.m4 |

[Spice-devel] [RFC PATCH spice-streaming-agent] Remove paranoid warning

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 generate warning for this. --- m4/spice-compile-warnings.m4 | 1 + src/spice-streaming-agent.cpp | 11 --- 2 files changed, 5 insertions(+), 7

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

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. Signed-off-by: Frediano Ziglio ---

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] [RFC PATCH spice-server] stream-device: Create channels before first not main connection

> > Title of the patch: “first not main” sounds un-english because it mixes > adjective and noun-used-as-adjective. Contrast “first, not last, connexion” > (with commas, I believe). Jonathon, any suggestion? Is “first non-primary” > correct, or does “primary” mean something else for SPICE? >

Re: [Spice-devel] [RFC PATCH spice-server] stream-device: Create channels before first not main connection

Title of the patch: “first not main” sounds un-english because it mixes adjective and noun-used-as-adjective. Contrast “first, not last, connexion” (with commas, I believe). Jonathon, any suggestion? Is “first non-primary” correct, or does “primary” mean something else for SPICE? Overall

[Spice-devel] [RFC PATCH spice-server] stream-device: Create channels before first not main connection

Due to ticket expiration is possible that the streaming channels for the client are created after the ticket expires. This would cause the client to not be able to connect back to a new streaming channel. To avoid this create the channels when the first main channel connection is made. This make