Re: [Spice-devel] [PATCH v3 04/11] Rephrase assertion section

[Frediano Ziglio]

> 
> > On 12 Feb 2018, at 13:34, Frediano Ziglio  wrote:
> > 
> >>> 
> >>> On 12 Feb 2018, at 11:22, Frediano Ziglio  wrote:
> >>> 
> > 
> > On 12 Feb 2018, at 09:21, Frediano Ziglio  wrote:
> > 
> >> 
> >> From: Christophe de Dinechin 
> >> 
> >> Signed-off-by: Christophe de Dinechin 
> >> ---
> >> docs/spice_style.txt | 15 ---
> >> 1 file changed, 12 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> >> index 13032df6..eef4880f 100644
> >> --- a/docs/spice_style.txt
> >> +++ b/docs/spice_style.txt
> >> @@ -99,10 +99,19 @@ FIXME and TODO
> >> 
> >> Comments that are prefixed with `FIXME` describe a bug that need to be
> >> fixed. Generally, it is not allowed to commit new code having `FIXME`
> >> comment. Committing  `FIXME` is allowed only for existing bugs.
> >> Comments
> >> that are prefixed with `TODO` describe further features, optimization
> >> or
> >> code improvements, and they are allowed to be committed along with the
> >> relevant code.
> >> 
> >> -ASSERT
> >> ---
> >> +Assertions
> >> +--
> >> +
> >> +Use assertions liberally. Assertions help testing function arguments
> >> and
> >> function results validity. As a result, they make it easier to detect
> >> bugs.
> >> Also, by expressing the intent of the developer, they make the code
> >> easier
> >> to read.
> >> +
> > 
> > I come from the Windows world and I found here in our case the "Use
> > assertions
> > liberally" a bit stronger if they are always used.
>  
>  I’m sorry, I can’t make sense of this sentence. I believe you mean that
>  the
>  recommendation is too strong (not stronger) because we never disable
>  them?
>  
> >>> 
> >>> yes, "too strong”
> >> 
> >> OK. Now I understand what you mean.
> >> 
> >>> 
>  Do you have any performance numbers on the extra cost of assertions?
>  
> >>> 
> >>> Does it matter?
> >> 
> >> Yes it does. When someone refrains from writing an assert because they are
> >> afraid of the extra cost, it generally mean they have no clue and switched
> >> to FUD mode. A typical assert on a modern CPU is a test and a branch, so
> >> we
> >> are talking nanoseconds.
> >> 
> > 
> > If the rule is "never care" that include any path in the code.
> 
> OK. “Liberally” IMO does not mean “don’t care”.
> 
> Liberally means “feel free to add them as you identify conditions that are
> worth checking”. But based on the discussion, I think we need to add things
> to pay attention to, namely:
> 
> - Avoid side effects
> - Don’t use assert as an alternative for error checking
> - Be mindful of timing
> 
> If we add that, does that address your concerns?
> 

Yes.

> > As a practical example take into account Quic code. In encode function
> > there are 4 spice_assert. encode is called for each pixel of the image
> > to million of times for a big image.
> 
> Well, I decided to resort to hard data to figure out how much this case
> really mattered.
> 
> With a reduced test case like this:
> 
> https://pastebin.com/inwJnAmd
> 
> Running 1 iterations with assertions gives: “real 0m11.507s"
> Running without assertions gives: "real   0m0.128s"
> 
> So this is one case where assertions represent 100x the payload!!! Definitely
> worth looking into. If you ever want to use ENABLE_EXTRA_CHECKS anywhere,
> that would be a good place ;-)
> 

Ouch... was not expecting so bad either!

> Also, note that the loss is 10s for 10 billion pixels encoded, so that’s 1s
> per billion pixels, or 1s for 113 full 4K frames. At 60Hz, that suggests
> that the asserts could eat 50% of the CPU. Clearly, if I did not goof up in
> my evaluation, that needs some attention.
> 
> I filed https://spice-redmine.usersys.redhat.com/issues/62.
> 
> Finally, for the record, do not underestimate the compiler either. If you
> leave only the assertions, i.e run https://pastebin.com/DzPY7B1h, then you
> get a time of real0m0.005s, and you will notice that the compiler just
> removed the assertions entirely. It also took me three attempts before
> getting arguments to “encode” where the compiler would not statically remove
> the assertions. My first experiments gave absolutely no effect of
> assertions, because assertions were just optimized away at compile-time.
> 
> 
> > Yes, the test is taking nanoseconds
> > but multiplied by millions turn it to milliseconds multiplied by the images
> > and frames and VMs in a machine they starts to count.
> 
> To me, the only way to decide is to offer hard evidence, as I did above.
> 
> Otherwise, it’s really speculation. In the example above, with variations in
> just the call arguments, I can either have assertions wiped out statically
> or having them count for 100x more than the body of the function…
> 
> 
> > These spice_assert(s) came probably 

Re: [Spice-devel] [PATCH v3 04/11] Rephrase assertion section

[Christophe de Dinechin]

> On 12 Feb 2018, at 13:34, Frediano Ziglio  wrote:
> 
>>> 
>>> On 12 Feb 2018, at 11:22, Frediano Ziglio  wrote:
>>> 
> 
> On 12 Feb 2018, at 09:21, Frediano Ziglio  wrote:
> 
>> 
>> From: Christophe de Dinechin 
>> 
>> Signed-off-by: Christophe de Dinechin 
>> ---
>> docs/spice_style.txt | 15 ---
>> 1 file changed, 12 insertions(+), 3 deletions(-)
>> 
>> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
>> index 13032df6..eef4880f 100644
>> --- a/docs/spice_style.txt
>> +++ b/docs/spice_style.txt
>> @@ -99,10 +99,19 @@ FIXME and TODO
>> 
>> Comments that are prefixed with `FIXME` describe a bug that need to be
>> fixed. Generally, it is not allowed to commit new code having `FIXME`
>> comment. Committing  `FIXME` is allowed only for existing bugs. Comments
>> that are prefixed with `TODO` describe further features, optimization or
>> code improvements, and they are allowed to be committed along with the
>> relevant code.
>> 
>> -ASSERT
>> ---
>> +Assertions
>> +--
>> +
>> +Use assertions liberally. Assertions help testing function arguments
>> and
>> function results validity. As a result, they make it easier to detect
>> bugs.
>> Also, by expressing the intent of the developer, they make the code
>> easier
>> to read.
>> +
> 
> I come from the Windows world and I found here in our case the "Use
> assertions
> liberally" a bit stronger if they are always used.
 
 I’m sorry, I can’t make sense of this sentence. I believe you mean that
 the
 recommendation is too strong (not stronger) because we never disable them?
 
>>> 
>>> yes, "too strong”
>> 
>> OK. Now I understand what you mean.
>> 
>>> 
 Do you have any performance numbers on the extra cost of assertions?
 
>>> 
>>> Does it matter?
>> 
>> Yes it does. When someone refrains from writing an assert because they are
>> afraid of the extra cost, it generally mean they have no clue and switched
>> to FUD mode. A typical assert on a modern CPU is a test and a branch, so we
>> are talking nanoseconds.
>> 
> 
> If the rule is "never care" that include any path in the code.

OK. “Liberally” IMO does not mean “don’t care”.

Liberally means “feel free to add them as you identify conditions that are 
worth checking”. But based on the discussion, I think we need to add things to 
pay attention to, namely:

- Avoid side effects
- Don’t use assert as an alternative for error checking
- Be mindful of timing

If we add that, does that address your concerns?

> As a practical example take into account Quic code. In encode function
> there are 4 spice_assert. encode is called for each pixel of the image
> to million of times for a big image.

Well, I decided to resort to hard data to figure out how much this case really 
mattered.

With a reduced test case like this:

https://pastebin.com/inwJnAmd

Running 1 iterations with assertions gives: “real   0m11.507s"
Running without assertions gives: "real 0m0.128s"

So this is one case where assertions represent 100x the payload!!! Definitely 
worth looking into. If you ever want to use ENABLE_EXTRA_CHECKS anywhere, that 
would be a good place ;-)

Also, note that the loss is 10s for 10 billion pixels encoded, so that’s 1s per 
billion pixels, or 1s for 113 full 4K frames. At 60Hz, that suggests that the 
asserts could eat 50% of the CPU. Clearly, if I did not goof up in my 
evaluation, that needs some attention.

I filed https://spice-redmine.usersys.redhat.com/issues/62.

Finally, for the record, do not underestimate the compiler either. If you leave 
only the assertions, i.e run https://pastebin.com/DzPY7B1h, then you get a time 
of real 0m0.005s, and you will notice that the compiler just removed the 
assertions entirely. It also took me three attempts before getting arguments to 
“encode” where the compiler would not statically remove the assertions. My 
first experiments gave absolutely no effect of assertions, because assertions 
were just optimized away at compile-time.


> Yes, the test is taking nanoseconds
> but multiplied by millions turn it to milliseconds multiplied by the images
> and frames and VMs in a machine they starts to count.

To me, the only way to decide is to offer hard evidence, as I did above.

Otherwise, it’s really speculation. In the example above, with variations in 
just the call arguments, I can either have assertions wiped out statically or 
having them count for 100x more than the body of the function…


> These spice_assert(s) came probably from a long history path where
> ASSERT/assert were used but not compiled in release code.
> Other example is functions like ring_remove which for performance reasons
> somebody decided to define inline but some with all the spice_assert in it
> are enough big to take quite some bytes.

Again, what matters to me is to perform 

Re: [Spice-devel] [PATCH spice-gtk 1/4] tests: add spice+unix:// URI checks

[Marc-André Lureau]

Hi

On Thu, Feb 8, 2018 at 2:18 PM, Frediano Ziglio  wrote:
>>
>> From: Marc-André Lureau 
>>
>> For some reason, the URIs test didn't include spice+unix:// checks,
>> probably because they came about the same time.
>>
>> Signed-off-by: Marc-André Lureau 
>> ---
>>  tests/session.c | 28 +---
>>  1 file changed, 25 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/session.c b/tests/session.c
>> index 7ed4a41..413d812 100644
>> --- a/tests/session.c
>> +++ b/tests/session.c
>> @@ -9,6 +9,7 @@ typedef struct {
>>  const gchar *uri_input;
>>  const gchar *uri_output;
>>  const gchar *message;
>> +const gchar *unix_path;
>>  } TestCase;
>>
>>  static void test_session_uri_bad(void)
>> @@ -139,7 +140,7 @@ static void test_session_uri_good(const TestCase *tests,
>> const guint cases)
>>
>>  /* Set URI and check URI, port and tls_port */
>>  for (i = 0; i < cases; i++) {
>> -gchar *uri, *port, *tls_port, *host, *username, *password;
>> +gchar *uri, *port, *tls_port, *host, *username, *password,
>> *unix_path;
>>
>>  s = spice_session_new();
>>  if (tests[i].message != NULL)
>> @@ -152,20 +153,23 @@ static void test_session_uri_good(const TestCase
>> *tests, const guint cases)
>>   "host", &host,
>>   "username", &username,
>>   "password", &password,
>> + "unix-path", &unix_path,
>>NULL);
>> -g_assert_cmpstr(tests[i].uri_output, ==, uri);
>> +g_assert_cmpstr(tests[i].uri_output ?: tests[i].uri_input, ==, uri);
>>  g_assert_cmpstr(tests[i].port, ==, port);
>>  g_assert_cmpstr(tests[i].tls_port, ==, tls_port);
>>  g_assert_cmpstr(tests[i].host, ==, host);
>>  g_assert_cmpstr(tests[i].username, ==, username);
>>  g_assert_cmpstr(tests[i].password, ==, password);
>>  g_test_assert_expected_messages();
>> +g_assert_cmpstr(tests[i].unix_path, ==, unix_path);
>>  g_clear_pointer(&uri, g_free);
>>  g_clear_pointer(&port, g_free);
>>  g_clear_pointer(&tls_port, g_free);
>>  g_clear_pointer(&host, g_free);
>>  g_clear_pointer(&username, g_free);
>>  g_clear_pointer(&password, g_free);
>> +g_clear_pointer(&unix_path, g_free);
>>  g_object_unref(s);
>>  }
>>
>> @@ -180,9 +184,10 @@ static void test_session_uri_good(const TestCase *tests,
>> const guint cases)
>>   "host", tests[i].host,
>>   "username", tests[i].username,
>>   "password", tests[i].password,
>> + "unix-path", tests[i].unix_path,
>>NULL);
>>  g_object_get(s, "uri", &uri, NULL);
>> -g_assert_cmpstr(tests[i].uri_output, ==, uri);
>> +g_assert_cmpstr(tests[i].uri_output ?: tests[i].uri_input, ==, uri);
>>  g_clear_pointer(&uri, g_free);
>>  g_object_unref(s);
>>  }
>> @@ -278,6 +283,22 @@ static void test_session_uri_ipv6_good(void)
>>  test_session_uri_good(tests, G_N_ELEMENTS(tests));
>>  }
>>
>> +static void test_session_uri_unix_good(void)
>> +{
>> +const TestCase tests[] = {
>> +{ .uri_input = "spice+unix:///tmp/foo.sock",
>> +  .unix_path = "/tmp/foo.sock" },
>> +/* perhaps not very clever, but this doesn't raise an error/warning
>> */
>> +{ .uri_input = "spice+unix://",
>> +  .unix_path = "" },
>> +/* unix uri don't support passing password or other kind of options
>> */
>> +{ .uri_input = "spice+unix:///tmp/foo.sock?password=frobnicate",
>> +  .unix_path = "/tmp/foo.sock?password=frobnicate" },
>> +};
>> +
>> +test_session_uri_good(tests, G_N_ELEMENTS(tests));
>> +}
>> +
>>  int main(int argc, char* argv[])
>>  {
>>  g_test_init(&argc, &argv, NULL);
>> @@ -285,6 +306,7 @@ int main(int argc, char* argv[])
>>  g_test_add_func("/session/bad-uri", test_session_uri_bad);
>>  g_test_add_func("/session/good-ipv4-uri", test_session_uri_ipv4_good);
>>  g_test_add_func("/session/good-ipv6-uri", test_session_uri_ipv6_good);
>> +g_test_add_func("/session/good-unix", test_session_uri_unix_good);
>>
>>  return g_test_run();
>>  }
>
> Looks good (still to review better).
> Can we consider this patch separate from the rest of the series
> (that is merge even separately) ?

Sure, it was just in the same area of code, and thus added dependency,
but we can review & merge this one right away I think.
thanks


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

Re: [Spice-devel] [PATCH spice-server v2] reds: Disable TLS 1.0

[Victor Toso]

Hey,

- Original Message -
> TLS 1.0 is considered now insecure.
> TLS 1.1 was introduced in 2006.
> Our SPICE clients uses OpenSSL to use TLS and the support for TLS 1.1
> in OpenSSL was introduced in 2006 too so even in systems like
> Windows XP which are not officially supporting TLS 1.0 will work
> with SPICE and TLS 1.1.
> This fixes https://bugzilla.redhat.com/show_bug.cgi?id=1521053.

Yes, this should be fine. Ack.

> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/reds.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Changes since v1:
> - update comment to be more precise.
> 
> diff --git a/server/reds.c b/server/reds.c
> index fa5e838a..a31ed4e9 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -2728,10 +2728,10 @@ static int reds_init_ssl(RedsState *reds)
>  static GOnce openssl_once = G_ONCE_INIT;
>  const SSL_METHOD *ssl_method;
>  int return_code;
> -/* Limit connection to TLSv1 only.
> +/* Limit connection to TLSv1.1 or newer.
>   * When some other SSL/TLS version becomes obsolete, add it to this
>   * variable. */
> -long ssl_options = SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3 |
> SSL_OP_NO_COMPRESSION;
> +long ssl_options = SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3 |
> SSL_OP_NO_COMPRESSION | SSL_OP_NO_TLSv1;
>  
>  /* Global system initialization*/
>  g_once(&openssl_once, openssl_global_init, NULL);
> --
> 2.14.3
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH spice-server v2] reds: Disable TLS 1.0

[Frediano Ziglio]

TLS 1.0 is considered now insecure.
TLS 1.1 was introduced in 2006.
Our SPICE clients uses OpenSSL to use TLS and the support for TLS 1.1
in OpenSSL was introduced in 2006 too so even in systems like
Windows XP which are not officially supporting TLS 1.0 will work
with SPICE and TLS 1.1.
This fixes https://bugzilla.redhat.com/show_bug.cgi?id=1521053.

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

Changes since v1:
- update comment to be more precise.

diff --git a/server/reds.c b/server/reds.c
index fa5e838a..a31ed4e9 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -2728,10 +2728,10 @@ static int reds_init_ssl(RedsState *reds)
 static GOnce openssl_once = G_ONCE_INIT;
 const SSL_METHOD *ssl_method;
 int return_code;
-/* Limit connection to TLSv1 only.
+/* Limit connection to TLSv1.1 or newer.
  * When some other SSL/TLS version becomes obsolete, add it to this
  * variable. */
-long ssl_options = SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3 | 
SSL_OP_NO_COMPRESSION;
+long ssl_options = SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3 | 
SSL_OP_NO_COMPRESSION | SSL_OP_NO_TLSv1;
 
 /* Global system initialization*/
 g_once(&openssl_once, openssl_global_init, NULL);
-- 
2.14.3

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

Re: [Spice-devel] [PATCH vdagent 2/2] vdagent: handle clipboard using GTK+

[Victor Toso]

Hi,

On Sun, Jan 21, 2018 at 09:03:14PM +0100, Jakub Janků wrote:
> From: Jakub Janků 
> 
> Place the code that handles clipboard
> into a separate file - clipboard.c
> ---
>  Makefile.am |   2 +
>  src/vdagent/clipboard.c | 401 
> 
>  src/vdagent/clipboard.h |  42 +
>  src/vdagent/vdagent.c   |  31 +++-
>  4 files changed, 471 insertions(+), 5 deletions(-)
>  create mode 100644 src/vdagent/clipboard.c
>  create mode 100644 src/vdagent/clipboard.h
> 
> diff --git a/Makefile.am b/Makefile.am
> index c4bd3dd..88550c6 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -33,6 +33,8 @@ src_spice_vdagent_SOURCES = \
>   $(common_sources)   \
>   src/vdagent/audio.c \
>   src/vdagent/audio.h \
> + src/vdagent/clipboard.c \
> + src/vdagent/clipboard.h \
>   src/vdagent/file-xfers.c\
>   src/vdagent/file-xfers.h\
>   src/vdagent/x11-priv.h  \
> diff --git a/src/vdagent/clipboard.c b/src/vdagent/clipboard.c
> new file mode 100644
> index 000..657a6b4
> --- /dev/null
> +++ b/src/vdagent/clipboard.c
> @@ -0,0 +1,401 @@
> +/*  clipboard.c - vdagent clipboard handling code
> +
> +Copyright 2017 Red Hat, Inc.
> +
> +This program is free software: you can redistribute it and/or modify
> +it under the terms of the GNU General Public License as published by
> +the Free Software Foundation, either version 3 of the License, or
> +(at your option) any later version.
> +
> +This program is distributed in the hope that it will be useful,
> +but WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +GNU General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with this program.  If not, see .
> +*/
> +
> +#ifdef HAVE_CONFIG_H
> +# include 
> +#endif
> +
> +#include 
> +#include 
> +
> +#include "vdagentd-proto.h"
> +#include "spice/vd_agent.h"
> +#include "udscs.h"
> +#include "clipboard.h"
> +
> +/* 2 selections supported - _SELECTION_CLIPBOARD = 0, _SELECTION_PRIMARY = 1 
> */
> +#define SELECTION_COUNT (VD_AGENT_CLIPBOARD_SELECTION_PRIMARY + 1)
> +#define TYPE_COUNT  (VD_AGENT_CLIPBOARD_IMAGE_JPG + 1)
> +
> +#define sel_from_clip(clipboard) \
> +GPOINTER_TO_UINT(g_object_get_data(G_OBJECT(clipboard), 
> "vdagent-selection"))
> +
> +enum {
> +OWNER_NONE,
> +OWNER_GUEST,
> +OWNER_CLIENT
> +};
> +
> +typedef struct {
> +GMainLoop*loop;
> +GtkSelectionData *sel_data;
> +} DataRetrieval;
> +
> +struct VDAgentClipboards {
> +struct udscs_connection*conn;
> +
> +GtkClipboard   *clipboard[SELECTION_COUNT];
> +guint   owner[SELECTION_COUNT];
> +
> +GList  *data_retrievals[SELECTION_COUNT];
> +GList  *data_requests[SELECTION_COUNT];

I think we could use something more clear instead of
'data_retrievals' and 'data_requests' but I'm terrible with
names myself so I suggest 'pending_set_request' and
'pending_get_requests' or just 'set_requests' and 'get_requests'.

A comment would help too, like /* Client -> Guest */ and vice
versa.

> +gpointer   *last_targets_req[SELECTION_COUNT];
> +
> +GdkAtom targets[SELECTION_COUNT][TYPE_COUNT];

Maybe wrap them in inner struct { ... } selection[SELECTION_TYPE] ?

e.g.
- c->data_requests[sel] = ...
+ c->selection[id].data_request = ...

> +};
> +
> +static const struct {
> +guint type;
> +const gchar  *atom_name;
> +} atom2agent[] = {
> +{VD_AGENT_CLIPBOARD_UTF8_TEXT, "UTF8_STRING"},
> +{VD_AGENT_CLIPBOARD_UTF8_TEXT, "text/plain;charset=utf-8"},
> +{VD_AGENT_CLIPBOARD_UTF8_TEXT, "STRING"},
> +{VD_AGENT_CLIPBOARD_UTF8_TEXT, "TEXT"},
> +{VD_AGENT_CLIPBOARD_UTF8_TEXT, "text/plain"},
> +{VD_AGENT_CLIPBOARD_IMAGE_PNG, "image/png"},
> +{VD_AGENT_CLIPBOARD_IMAGE_BMP, "image/bmp"},
> +{VD_AGENT_CLIPBOARD_IMAGE_BMP, "image/x-bmp"},
> +{VD_AGENT_CLIPBOARD_IMAGE_BMP, "image/x-MS-bmp"},
> +{VD_AGENT_CLIPBOARD_IMAGE_BMP, "image/x-win-bitmap"},
> +{VD_AGENT_CLIPBOARD_IMAGE_TIFF,"image/tiff"},
> +{VD_AGENT_CLIPBOARD_IMAGE_JPG, "image/jpeg"},
> +};
> +
> +static guint get_type_from_atom(GdkAtom atom)
> +{
> +int i;
> +gchar *name = gdk_atom_name(atom);
> +for (i = 0; i < G_N_ELEMENTS(atom2agent); i++)
> +if (!g_ascii_strcasecmp(name, atom2agent[i].atom_name))
> +break;
> +g_free(name);
> +return i < G_N_ELEMENTS(atom2agent) ? atom2agent[i].type
> +: VD_AGENT_CLIPBOARD_NONE;
> +}
> +
> +/* gtk_clipboard_request_(, callback, user_data

Re: [Spice-devel] [PATCH spice-server v4 0/4] Better handling reset for streaming device

[Lukáš Hrázký]

On Sun, 2018-02-11 at 17:19 +0200, Uri Lublin wrote:
> On 01/30/2018 04:33 PM, Christophe de Dinechin wrote:
> > 
> > Christophe de Dinechin writes:
> > 
> > > > On 30 Jan 2018, at 12:56, Frediano Ziglio  wrote:
> > > > 
> > > > > 
> > > > > Hi Frediano,
> > > > > 
> > > > > 
> > > > > 
> > > > > > On 30 Jan 2018, at 11:50, Frediano Ziglio  
> > > > > > wrote:
> > > > > > 
> > > > > > ping the series
> > > > > 
> > > > > I’m currently looking at it. Is it supposed to fix the read errors I 
> > > > > had when
> > > > > restarting the streaming agent?
> > > > > 
> > > > 
> > > > Yes, make the reset more stable.
> > > > When you close the device the state will be more consistent allowing
> > > > basically to kill the process using the device in any state and opening
> > > > again. Obviously if you continue to send wrong commands the device will
> > > > keep rejecting them.
> > > > 
> > > > I tried to reproduce the issues reported on IRC and these helped me,
> > > > now I avoid entirely to reboot the guest.
> > > 
> > > OK, right now I get a QEMU crash whenever I do any kind of activity
> > > (the keyboard seems to be what triggers it).
> > > 
> > > I’m trying to reproduce on master to see if your patch is the cause.
> > > That host has gone through some unusual nastiness, and may be
> > > in a geborked state.
> > 
> > Reverting the server to master, I am back to the behavior I had before,
> > where the same series of events leads to
> > 
> > DISPLAY=:1 spice-streaming-agent -c noblock=yes
> > spice-streaming-agent[2240]: UNKNOWN msg of type 5
> > spice-streaming-agent[2240]: BAD VERSION 0 (expected is 1)
> > spice-streaming-agent[2240]: BAD VERSION 108 (expected is 1)
> > spice-streaming-agent[2240]: BAD VERSION 97 (expected is 1)
> > spice-streaming-agent[2240]: read command from device FAILED -- read 1 
> > expected 8
> > spice-streaming-agent[2240]: FAILED to read command
> 
> 
> Hi Christophe,
> 
> There are some problems here:
> 1. (I guess) your spice-streaming-agent sends CURSOR messages
> which currently the spice-server does not know to handle.
> (Frediano sent patches for that but still no review).
> 
> For now try to comment out the code in spice-streaming-agent
> that sends CURSOR commands.
> 
> 2. spice-server gets an unknown command. It sends a message to the
> spice-streaming-agent to let it know the server read an invalid
> message. spice-streaming-agent is missing a code to handle such
> a message.
> 
> This should be fixed.

I've got the patch for this. Just wanted to write a unit test for it
and that's where I hit several issues which I'm still trying to
resolve... I might post the patch without a test for now...

Lukas

> 3. When such messages are in play, they are not fully read (code
> breaks after reading the header). This makes the spice-server
> and spice-streaming-agent go out of sync).
> 
> This may be fixed. I'm not sure we can recover
> all errors, and perhaps the right thing to do is sync
> with close/open of the virtio-serial-port.
> However reading the whole message (if it's not too big) should
> at least keep the server/agent synchronized, even if an unknown
> message encountered.
> 
> Regards,
>  Uri.
> 
> ___
> 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] [RFC PATCH spice-streaming-agent v2] separate and encapsulate the agent business code

[Lukáš Hrázký]

On Mon, 2018-02-12 at 03:53 -0500, Frediano Ziglio wrote:
> > 
> > Create an AgentRunner (TODO: needs a better name) class to encapsulate
> > the streaming and communication with the server. The basic setup (cmd
> 
> Also encapsulate command line interface (quite odd, does it make still
> much sense?)

Well, yes.. if I understand what you mean, that is the other half that
is separated... Want me to mention it explicitly in the commit message?

> > arg parsing, signal handling, ...) is moved to main.cpp. The rest of the
> > functions is moved to the AgentRunner class and modified as little as
> > possible:
> > - The cursor updating code is moved into a functor called CursorThread
> > - Some initialization and cleanup is moved to AgentRunner's constructor
> >   and destructor
> > - Some error handling moved over to exceptions, mainly what was in
> >   main() and do_capture()
> > - A couple of variables gently renamed.
> > 
> > Signed-off-by: Lukáš Hrázký 
> > ---
> > Changes since v1:
> > - update according to Frediano's namespace changes
> > 
> >  src/Makefile.am   |   2 +
> >  src/main.cpp  | 126 
> >  src/spice-streaming-agent.cpp | 458
> >  +-
> >  src/spice-streaming-agent.hpp |  57 ++
> >  4 files changed, 372 insertions(+), 271 deletions(-)
> >  create mode 100644 src/main.cpp
> >  create mode 100644 src/spice-streaming-agent.hpp
> > 
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index 6d37274..42bb10f 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -49,6 +49,7 @@ spice_streaming_agent_LDADD = \
> >  
> >  spice_streaming_agent_SOURCES = \
> > spice-streaming-agent.cpp \
> > +   spice-streaming-agent.hpp \
> > static-plugin.cpp \
> > static-plugin.hpp \
> > concrete-agent.cpp \
> > @@ -57,4 +58,5 @@ spice_streaming_agent_SOURCES = \
> > mjpeg-fallback.hpp \
> > jpeg.cpp \
> > jpeg.hpp \
> > +   main.cpp \
> > $(NULL)
> > diff --git a/src/main.cpp b/src/main.cpp
> > new file mode 100644
> > index 000..46eda85
> > --- /dev/null
> > +++ b/src/main.cpp
> > @@ -0,0 +1,126 @@
> > +/* An implementation of a SPICE streaming agent
> > + *
> > + * \copyright
> > + * Copyright 2016-2018 Red Hat Inc. All rights reserved.
> > + */
> > +
> > +#include 
> > +#include "spice-streaming-agent.hpp"
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +
> > +namespace ssa = spice::streaming_agent;
> > +
> > +static void usage(const char *progname)
> > +{
> > +printf("usage: %s \n", progname);
> > +printf("options are:\n");
> > +printf("\t-p portname  -- virtio-serial port to use\n");
> > +printf("\t-i accept commands from stdin\n");
> > +printf("\t-l file -- log frames to file\n");
> > +printf("\t--log-binary -- log binary frames (following -l)\n");
> > +printf("\t-d -- enable debug logs\n");
> > +printf("\t-c variable=value -- change settings\n");
> > +printf("\t\tframerate = 1-100 (check 10,20,30,40,50,60)\n");
> > +printf("\n");
> > +printf("\t-h or --help -- print this help message\n");
> > +
> > +exit(1);
> > +}
> > +
> > +void handle_interrupt(int intr)
> > +{
> > +syslog(LOG_INFO, "Got signal %d, exiting", intr);
> > +ssa::AgentRunner::quit = true;
> > +}
> > +
> > +void register_interrupts(void)
> > +{
> > +struct sigaction sa;
> > +
> > +memset(&sa, 0, sizeof(sa));
> > +sa.sa_handler = handle_interrupt;
> > +if ((sigaction(SIGINT, &sa, NULL) != 0) &&
> > +(sigaction(SIGTERM, &sa, NULL) != 0)) {
> > +syslog(LOG_WARNING, "failed to register signal handler %m");
> > +}
> > +}
> > +
> > +int main(int argc, char* argv[])
> > +{
> > +std::string stream_port = "/dev/virtio-ports/com.redhat.stream.0";
> > +char opt;
> > +std::string log_filename;
> > +int log_binary = 0;
> > +bool stdin_ok = false;
> > +int logmask = LOG_UPTO(LOG_WARNING);
> > +struct option long_options[] = {
> > +{ "log-binary", no_argument, &log_binary, 1},
> > +{ "help", no_argument, NULL, 'h'},
> > +{ 0, 0, 0, 0}
> > +};
> > +
> > +if (isatty(fileno(stderr)) && isatty(fileno(stdin))) {
> > +stdin_ok = true;
> > +}
> > +
> > +openlog("spice-streaming-agent", stdin_ok? (LOG_PERROR|LOG_PID) :
> > LOG_PID, LOG_USER);
> > +setlogmask(logmask);
> > +
> > +std::vector> options;
> > +
> > +while ((opt = getopt_long(argc, argv, "hip:c:l:d", long_options, NULL))
> > != -1) {
> > +switch (opt) {
> > +case 0:
> > +/* Handle long options if needed */
> > +break;
> > +case 'i':
> > +stdin_ok = true;
> > +openlog("spice-streaming-agent", LOG_PERROR|LOG_PID, LOG_USER);
> > +break;
> > +case 'p':
> > +stream_port = optarg;
> > +break;
> > +case 'c': {
> > +char *p = str

Re: [Spice-devel] [PATCH v3 04/11] Rephrase assertion section

[Frediano Ziglio]

> 
> > On 12 Feb 2018, at 11:22, Frediano Ziglio  wrote:
> > 
> >>> 
> >>> On 12 Feb 2018, at 09:21, Frediano Ziglio  wrote:
> >>> 
>  
>  From: Christophe de Dinechin 
>  
>  Signed-off-by: Christophe de Dinechin 
>  ---
>  docs/spice_style.txt | 15 ---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>  
>  diff --git a/docs/spice_style.txt b/docs/spice_style.txt
>  index 13032df6..eef4880f 100644
>  --- a/docs/spice_style.txt
>  +++ b/docs/spice_style.txt
>  @@ -99,10 +99,19 @@ FIXME and TODO
>  
>  Comments that are prefixed with `FIXME` describe a bug that need to be
>  fixed. Generally, it is not allowed to commit new code having `FIXME`
>  comment. Committing  `FIXME` is allowed only for existing bugs. Comments
>  that are prefixed with `TODO` describe further features, optimization or
>  code improvements, and they are allowed to be committed along with the
>  relevant code.
>  
>  -ASSERT
>  ---
>  +Assertions
>  +--
>  +
>  +Use assertions liberally. Assertions help testing function arguments
>  and
>  function results validity. As a result, they make it easier to detect
>  bugs.
>  Also, by expressing the intent of the developer, they make the code
>  easier
>  to read.
>  +
> >>> 
> >>> I come from the Windows world and I found here in our case the "Use
> >>> assertions
> >>> liberally" a bit stronger if they are always used.
> >> 
> >> I’m sorry, I can’t make sense of this sentence. I believe you mean that
> >> the
> >> recommendation is too strong (not stronger) because we never disable them?
> >> 
> > 
> > yes, "too strong”
> 
> OK. Now I understand what you mean.
> 
> > 
> >> Do you have any performance numbers on the extra cost of assertions?
> >> 
> > 
> > Does it matter?
> 
> Yes it does. When someone refrains from writing an assert because they are
> afraid of the extra cost, it generally mean they have no clue and switched
> to FUD mode. A typical assert on a modern CPU is a test and a branch, so we
> are talking nanoseconds.
> 

If the rule is "never care" that include any path in the code.
As a practical example take into account Quic code. In encode function
there are 4 spice_assert. encode is called for each pixel of the image
to million of times for a big image. Yes, the test is taking nanoseconds
but multiplied by millions turn it to milliseconds multiplied by the images
and frames and VMs in a machine they starts to count.
These spice_assert(s) came probably from a long history path where
ASSERT/assert were used but not compiled in release code.
Other example is functions like ring_remove which for performance reasons
somebody decided to define inline but some with all the spice_assert in it
are enough big to take quite some bytes.

> 
> > Depends on many thing, but the "use liberally" seems to
> > indicate that we never care about the costs.
> 
> No, it means that in general, we do not care about the cost of an assert.
> 
>  Of course, don’t be stupid and write code like:
> 
>   assert(recursive_fibonacci(50) > 0)
> 
> because then the assert might cost a little more than a few nanoseconds
> (about 36 seconds on my laptop).
> 
> But in reality, asserts are generally used to test very simple conditions.

In Windows people expect these checks to disappear in release so people
do more expensive checks.
This "small" setting change make a very big difference.

> Bad cases of an expensive assert in performance critical code are extremely
> rare. If this happens, I expect code review to catch it, and even so, I
> would not even consider the removal of an assert for that reason without
> hard numbers. Which is why I asked you about hard numbers above :-)
> 

But the style apply also to any future code so it catches also the addition
case, not only removal. And as code was moved from ASSERT to 
spice_assert/g_assert
implicitly (maybe without people realizing) turned out in many additions.

> Remember, I recently did some measurements to prove that a record entry would
> not be too expensive to be left in the code all the time. Assertions are
> generally even less expensive than a flight recorder entry.
> 
> If you want, what I could do is add a rule that in general, the expression
> inside an assert should make no function calls and have no side effects.
> That would have two benefits:
> 
> - By avoiding function calls, you restrict expressions to things that can be
> computed locally, which generally means a few CPU cycles at most

But is hard to enforce in case of inline functions.

> - By avoiding side effects, you guarantee that the assert has no impact on
> the behavior of the program.
> 

I think no side effects (beside timing) are mandatory for asserts.

> Also, for C++ code, consider C++G I.6, i.e. thinking about writing
> contract-like stuff, with “expect” for assertions that check pre-conditions,
> etc.
> 

I.6 a

[Spice-devel] [PATCH spice-server] reds: Disable TLS 1.0

[Frediano Ziglio]

TLS 1.0 is considered now insecure.
TLS 1.1 was introduced in 2006.
Our SPICE clients uses OpenSSL to use TLS and the support for TLS 1.1
in OpenSSL was introduced in 2006 too so even in systems like
Windows XP which are not officially supporting TLS 1.0 will work
with SPICE and TLS 1.1.
This fixes https://bugzilla.redhat.com/show_bug.cgi?id=1521053.

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

diff --git a/server/reds.c b/server/reds.c
index fa5e838a..1895b225 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -2728,10 +2728,10 @@ static int reds_init_ssl(RedsState *reds)
 static GOnce openssl_once = G_ONCE_INIT;
 const SSL_METHOD *ssl_method;
 int return_code;
-/* Limit connection to TLSv1 only.
+/* Limit connection to TLSv1.1 only.
  * When some other SSL/TLS version becomes obsolete, add it to this
  * variable. */
-long ssl_options = SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3 | 
SSL_OP_NO_COMPRESSION;
+long ssl_options = SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3 | 
SSL_OP_NO_COMPRESSION | SSL_OP_NO_TLSv1;
 
 /* Global system initialization*/
 g_once(&openssl_once, openssl_global_init, NULL);
-- 
2.14.3

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

Re: [Spice-devel] [PATCH v3 04/11] Rephrase assertion section

[Christophe de Dinechin]

> On 12 Feb 2018, at 11:22, Frediano Ziglio  wrote:
> 
>>> 
>>> On 12 Feb 2018, at 09:21, Frediano Ziglio  wrote:
>>> 
 
 From: Christophe de Dinechin 
 
 Signed-off-by: Christophe de Dinechin 
 ---
 docs/spice_style.txt | 15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)
 
 diff --git a/docs/spice_style.txt b/docs/spice_style.txt
 index 13032df6..eef4880f 100644
 --- a/docs/spice_style.txt
 +++ b/docs/spice_style.txt
 @@ -99,10 +99,19 @@ FIXME and TODO
 
 Comments that are prefixed with `FIXME` describe a bug that need to be
 fixed. Generally, it is not allowed to commit new code having `FIXME`
 comment. Committing  `FIXME` is allowed only for existing bugs. Comments
 that are prefixed with `TODO` describe further features, optimization or
 code improvements, and they are allowed to be committed along with the
 relevant code.
 
 -ASSERT
 ---
 +Assertions
 +--
 +
 +Use assertions liberally. Assertions help testing function arguments and
 function results validity. As a result, they make it easier to detect
 bugs.
 Also, by expressing the intent of the developer, they make the code easier
 to read.
 +
>>> 
>>> I come from the Windows world and I found here in our case the "Use
>>> assertions
>>> liberally" a bit stronger if they are always used.
>> 
>> I’m sorry, I can’t make sense of this sentence. I believe you mean that the
>> recommendation is too strong (not stronger) because we never disable them?
>> 
> 
> yes, "too strong”

OK. Now I understand what you mean.

> 
>> Do you have any performance numbers on the extra cost of assertions?
>> 
> 
> Does it matter?

Yes it does. When someone refrains from writing an assert because they are 
afraid of the extra cost, it generally mean they have no clue and switched to 
FUD mode. A typical assert on a modern CPU is a test and a branch, so we are 
talking nanoseconds.


> Depends on many thing, but the "use liberally" seems to
> indicate that we never care about the costs.

No, it means that in general, we do not care about the cost of an assert.

 Of course, don’t be stupid and write code like:

assert(recursive_fibonacci(50) > 0)

because then the assert might cost a little more than a few nanoseconds (about 
36 seconds on my laptop).

But in reality, asserts are generally used to test very simple conditions. Bad 
cases of an expensive assert in performance critical code are extremely rare. 
If this happens, I expect code review to catch it, and even so, I would not 
even consider the removal of an assert for that reason without hard numbers. 
Which is why I asked you about hard numbers above :-)

Remember, I recently did some measurements to prove that a record entry would 
not be too expensive to be left in the code all the time. Assertions are 
generally even less expensive than a flight recorder entry.

If you want, what I could do is add a rule that in general, the expression 
inside an assert should make no function calls and have no side effects. That 
would have two benefits:

- By avoiding function calls, you restrict expressions to things that can be 
computed locally, which generally means a few CPU cycles at most
- By avoiding side effects, you guarantee that the assert has no impact on the 
behavior of the program.

Also, for C++ code, consider C++G I.6, i.e. thinking about writing 
contract-like stuff, with “expect” for assertions that check pre-conditions, 
etc.


> 
>> 
>>> Kind of always using
>>> address sanitizer compiled in even in production. Recently in spice-server
>>> we added code like:
>>> 
>>>   if (ENABLE_EXTRA_CHECKS) {
>>>   spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
>>>   spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT);
>>>   }
>>> 
>>> the ENABLE_EXTRA_CHECKS is always true/false and is a compiled in constant
>>> and is true only if explicitly enabled so the compiler will strip the
>>> entire
>>> checks if not enabled (but still checking for syntax).
>> 
>> Yes. This kind of code is the obvious result of being a bit confused on the
>> usage of assertions.
>> 
>> That specific example is somewhat ridiculous, by the way. The two tests are
>> one machine instruction each, I doubt they are even noticeable on any
>> performance benchmark.
>> 
> 
> One machine instruction? In which architecture?

We are not going to play that game again, are we? You know exactly what I mean.

Any architecture I know of can do an integer compare for equality or >= in one 
machine instruction. That’s what I’m talking about.

Then obviously, depending on the surrounding context, you will also probably 
have a load (likely to be from hot cache, unless you are asserting for 
something that is not related to surrounding code), and a never-taken 
conditional branch, which the compiler is likely to mark statically as not 
taken because the assert macros nowadays have hi

Re: [Spice-devel] [PATCH spice-server v4 0/4] Better handling reset for streaming device

[Frediano Ziglio]

> 
> > On 12 Feb 2018, at 09:04, Frediano Ziglio  wrote:
> > 
> >> 
> >> On 01/30/2018 04:33 PM, Christophe de Dinechin wrote:
> >>> 
> >>> Christophe de Dinechin writes:
> >>> 
> > On 30 Jan 2018, at 12:56, Frediano Ziglio  wrote:
> > 
> >> 
> >> Hi Frediano,
> >> 
> >> 
> >> 
> >>> On 30 Jan 2018, at 11:50, Frediano Ziglio  wrote:
> >>> 
> >>> ping the series
> >> 
> >> I’m currently looking at it. Is it supposed to fix the read errors I
> >> had
> >> when
> >> restarting the streaming agent?
> >> 
> > 
> > Yes, make the reset more stable.
> > When you close the device the state will be more consistent allowing
> > basically to kill the process using the device in any state and opening
> > again. Obviously if you continue to send wrong commands the device will
> > keep rejecting them.
> > 
> > I tried to reproduce the issues reported on IRC and these helped me,
> > now I avoid entirely to reboot the guest.
>  
>  OK, right now I get a QEMU crash whenever I do any kind of activity
>  (the keyboard seems to be what triggers it).
>  
>  I’m trying to reproduce on master to see if your patch is the cause.
>  That host has gone through some unusual nastiness, and may be
>  in a geborked state.
> >>> 
> >>> Reverting the server to master, I am back to the behavior I had before,
> >>> where the same series of events leads to
> >>> 
> >>> DISPLAY=:1 spice-streaming-agent -c noblock=yes
> >>> spice-streaming-agent[2240]: UNKNOWN msg of type 5
> >>> spice-streaming-agent[2240]: BAD VERSION 0 (expected is 1)
> >>> spice-streaming-agent[2240]: BAD VERSION 108 (expected is 1)
> >>> spice-streaming-agent[2240]: BAD VERSION 97 (expected is 1)
> >>> spice-streaming-agent[2240]: read command from device FAILED -- read 1
> >>> expected 8
> >>> spice-streaming-agent[2240]: FAILED to read command
> >> 
> >> 
> >> Hi Christophe,
> >> 
> >> There are some problems here:
> >> 1. (I guess) your spice-streaming-agent sends CURSOR messages
> >>which currently the spice-server does not know to handle.
> >>(Frediano sent patches for that but still no review).
> >> 
> >>For now try to comment out the code in spice-streaming-agent
> >>that sends CURSOR commands.
> >> 
> > 
> > Would not make sense to review the patches instead of having
> > to write another workaround also taking into account that
> > the review process is taking more than 6 months?
> > 
> >> 2. spice-server gets an unknown command. It sends a message to the
> >>spice-streaming-agent to let it know the server read an invalid
> >>message. spice-streaming-agent is missing a code to handle such
> >>a message.
> >> 
> >>This should be fixed.
> >> 
> >> 3. When such messages are in play, they are not fully read (code
> >>breaks after reading the header). This makes the spice-server
> >>and spice-streaming-agent go out of sync).
> >> 
> >>This may be fixed. I'm not sure we can recover
> >>all errors, and perhaps the right thing to do is sync
> >>with close/open of the virtio-serial-port.
> > 
> > This is what this patch series is doing.
> 
> And indeed, that patch series improves things if you have a recent enough
> QEMU. But it triggers a but in 2.9.1, which crashes QEMU and kills the VM. I
> saw that on my test machine, Frediano now reproduced.

but -> bug (if is not obvious).

Yes, is a bug recognized and fixed in recent Qemu.
Today I was checking the RHEL 7.5 package (they have 1800 patches on top!)
and the fix is not included.
Is not clear how to handle this bug.
Detecting if runtime Qemu has this bug is not easy (is a NULL check inside
a function). Also there are cases where spice-server is not used inside Qemu.
Detecting the Qemu version at compile time is not easy either as it adds a
reverse build dependency at build time (consider the RPM build) which
is a bad thing and also would not cover easily cases where an old
version has the fix for this bug.
Also we have to consider different distributions.
I can split the patch in 2 to avoid to trigger the bug on buggy versions
but this is not optimal, the device will keep working and potentially
the agent will ignore the error.
The Qemu bug is triggered when the device is closed inside a write
operation. The way character device works in case of spice is that Qemu
calls spice_server_char_device_wakeup which try to read and write.
In the patch I propose I call state(sin, 0) which closes the device
inside read, inside the Qemu write.
One possible workaround would be to call state(sin, 0) not in read
but the way I see would be to schedule a timer event that would
check if device has a pending error and in this case attempting to
close the device. Quite ugly code potentially not that easy to maintain.
I'll try to code and test this last workaround.

> > 
> >>However reading the whole message (if it's not too big) should
> >>at least keep th

Re: [Spice-devel] [PATCH v3 04/11] Rephrase assertion section

[Frediano Ziglio]

> 
> > On 12 Feb 2018, at 09:21, Frediano Ziglio  wrote:
> > 
> >> 
> >> From: Christophe de Dinechin 
> >> 
> >> Signed-off-by: Christophe de Dinechin 
> >> ---
> >> docs/spice_style.txt | 15 ---
> >> 1 file changed, 12 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> >> index 13032df6..eef4880f 100644
> >> --- a/docs/spice_style.txt
> >> +++ b/docs/spice_style.txt
> >> @@ -99,10 +99,19 @@ FIXME and TODO
> >> 
> >> Comments that are prefixed with `FIXME` describe a bug that need to be
> >> fixed. Generally, it is not allowed to commit new code having `FIXME`
> >> comment. Committing  `FIXME` is allowed only for existing bugs. Comments
> >> that are prefixed with `TODO` describe further features, optimization or
> >> code improvements, and they are allowed to be committed along with the
> >> relevant code.
> >> 
> >> -ASSERT
> >> ---
> >> +Assertions
> >> +--
> >> +
> >> +Use assertions liberally. Assertions help testing function arguments and
> >> function results validity. As a result, they make it easier to detect
> >> bugs.
> >> Also, by expressing the intent of the developer, they make the code easier
> >> to read.
> >> +
> > 
> > I come from the Windows world and I found here in our case the "Use
> > assertions
> > liberally" a bit stronger if they are always used.
> 
> I’m sorry, I can’t make sense of this sentence. I believe you mean that the
> recommendation is too strong (not stronger) because we never disable them?
> 

yes, "too strong"

> Do you have any performance numbers on the extra cost of assertions?
> 

Does it matter? Depends on many thing, but the "use liberally" seems to
indicate that we never care about the costs.

> 
> > Kind of always using
> > address sanitizer compiled in even in production. Recently in spice-server
> > we added code like:
> > 
> >if (ENABLE_EXTRA_CHECKS) {
> >spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
> >spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT);
> >}
> > 
> > the ENABLE_EXTRA_CHECKS is always true/false and is a compiled in constant
> > and is true only if explicitly enabled so the compiler will strip the
> > entire
> > checks if not enabled (but still checking for syntax).
> 
> Yes. This kind of code is the obvious result of being a bit confused on the
> usage of assertions.
> 
> That specific example is somewhat ridiculous, by the way. The two tests are
> one machine instruction each, I doubt they are even noticeable on any
> performance benchmark.
> 

One machine instruction? In which architecture?
Yes, this is not really hot path but I think there's a balance between
probability that the event happens, cost of not having the check and
costs having it. The boundaries are personal opinions. 

> ENABLE_EXTRA_CHECKS is also used for additional checks that are somewhat
> expensive at run-time, like in display_channel_finalize. OK, but then why
> not make that a run-time flag instead of a compile-time flag? Unless the
> additional test code makes your code larger by 50%, but I doubt this is the
> case here…
> 

Yes, as I said are opinions. And as I said ENABLE_EXTRA_CHECKS is a compile
time constant true or false so has no costs at runtime.

> > 
> >> +Several form of assertion exist. SPICE does not use the language `assert`
> >> much, but instead relies on the following variants:
> >> +
> >> +- `spice_assert`, defined in `common/log.h`, is never disabled at
> >> run-time
> >> and prints an error if the assertion fails.
> >> +
> >> +- `g_assert` and other variants defined in glib such as `g_assert_null`,
> >> can
> >> be diabled by setting `G_DISABLE_ASSERT` (which is never done in practice
> >> for SPICE code), and emits a message through the g_assertion_message
> >> function and its variants.
> >> +
> > 
> > typo: "diabled" -> "disabled"
> > No, our code as far as I remember should NEVER be compiled and used with
> > G_DISABLE_ASSERT enabled.
> 
> I mention in the text that we don’t disable them in practice.
> 
> But why should that NEVER be done? (in uppercase, no less)? If some distro
> has a policy of building with assertions disabled, that’s their choice.
> 

If you call g_assert to check runtime thing that can happen instead
of developer error and you remove the check at runtime this lead to
potential security issues so the NEVER that is we should ignore the
policy and build with assertions enabled even if the build policies
disable them. At this point is not their choice anymore.

> > Or maybe I'm confused by G_DISABLE_CHECKS ?
> > OT: maybe we should check this in the code (I think better place is
> > common/log.c).
> 
> Check what? That G_DISABLE_ASSERT is not set?
> 

Yes

> > 
> >> +The guidelines for selecting one or the other are not very clear from the
> >> existing code. The `spice_assert` variant may be preferred for SPICE-only
> >> code, as it makes it clearer that this assertion is coming from SPICE. The
> >> `g_assert` and var

Re: [Spice-devel] [PATCH spice-server v4 0/4] Better handling reset for streaming device

[Christophe de Dinechin]

> On 12 Feb 2018, at 09:04, Frediano Ziglio  wrote:
> 
>> 
>> On 01/30/2018 04:33 PM, Christophe de Dinechin wrote:
>>> 
>>> Christophe de Dinechin writes:
>>> 
> On 30 Jan 2018, at 12:56, Frediano Ziglio  wrote:
> 
>> 
>> Hi Frediano,
>> 
>> 
>> 
>>> On 30 Jan 2018, at 11:50, Frediano Ziglio  wrote:
>>> 
>>> ping the series
>> 
>> I’m currently looking at it. Is it supposed to fix the read errors I had
>> when
>> restarting the streaming agent?
>> 
> 
> Yes, make the reset more stable.
> When you close the device the state will be more consistent allowing
> basically to kill the process using the device in any state and opening
> again. Obviously if you continue to send wrong commands the device will
> keep rejecting them.
> 
> I tried to reproduce the issues reported on IRC and these helped me,
> now I avoid entirely to reboot the guest.
 
 OK, right now I get a QEMU crash whenever I do any kind of activity
 (the keyboard seems to be what triggers it).
 
 I’m trying to reproduce on master to see if your patch is the cause.
 That host has gone through some unusual nastiness, and may be
 in a geborked state.
>>> 
>>> Reverting the server to master, I am back to the behavior I had before,
>>> where the same series of events leads to
>>> 
>>> DISPLAY=:1 spice-streaming-agent -c noblock=yes
>>> spice-streaming-agent[2240]: UNKNOWN msg of type 5
>>> spice-streaming-agent[2240]: BAD VERSION 0 (expected is 1)
>>> spice-streaming-agent[2240]: BAD VERSION 108 (expected is 1)
>>> spice-streaming-agent[2240]: BAD VERSION 97 (expected is 1)
>>> spice-streaming-agent[2240]: read command from device FAILED -- read 1
>>> expected 8
>>> spice-streaming-agent[2240]: FAILED to read command
>> 
>> 
>> Hi Christophe,
>> 
>> There are some problems here:
>> 1. (I guess) your spice-streaming-agent sends CURSOR messages
>>which currently the spice-server does not know to handle.
>>(Frediano sent patches for that but still no review).
>> 
>>For now try to comment out the code in spice-streaming-agent
>>that sends CURSOR commands.
>> 
> 
> Would not make sense to review the patches instead of having
> to write another workaround also taking into account that 
> the review process is taking more than 6 months?
> 
>> 2. spice-server gets an unknown command. It sends a message to the
>>spice-streaming-agent to let it know the server read an invalid
>>message. spice-streaming-agent is missing a code to handle such
>>a message.
>> 
>>This should be fixed.
>> 
>> 3. When such messages are in play, they are not fully read (code
>>breaks after reading the header). This makes the spice-server
>>and spice-streaming-agent go out of sync).
>> 
>>This may be fixed. I'm not sure we can recover
>>all errors, and perhaps the right thing to do is sync
>>with close/open of the virtio-serial-port.
> 
> This is what this patch series is doing.

And indeed, that patch series improves things if you have a recent enough QEMU. 
But it triggers a but in 2.9.1, which crashes QEMU and kills the VM. I saw that 
on my test machine, Frediano now reproduced.

> 
>>However reading the whole message (if it's not too big) should
>>at least keep the server/agent synchronized, even if an unknown
>>message encountered.
>> 
> 
> No, is not enough. If the message is expected to change the server
> state discarding the message will make the dialog out of sync anyway.
> Would make sense if the server could send back an error for a specific
> message (currently if you send a batch of messages you don't know
> the relationship between error and message). Also would mean that
> the client MUST handle the error from a message and as the messages
> replies are async to keep a queue of messages.
> To me it seems that at the end would be just more complicated than
> expected instead of graceful.
> 
>> Regards,
>> Uri.
>> 
>> 
> 
> 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 v3 04/11] Rephrase assertion section

[Christophe de Dinechin]

> On 12 Feb 2018, at 09:21, Frediano Ziglio  wrote:
> 
>> 
>> From: Christophe de Dinechin 
>> 
>> Signed-off-by: Christophe de Dinechin 
>> ---
>> docs/spice_style.txt | 15 ---
>> 1 file changed, 12 insertions(+), 3 deletions(-)
>> 
>> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
>> index 13032df6..eef4880f 100644
>> --- a/docs/spice_style.txt
>> +++ b/docs/spice_style.txt
>> @@ -99,10 +99,19 @@ FIXME and TODO
>> 
>> Comments that are prefixed with `FIXME` describe a bug that need to be
>> fixed. Generally, it is not allowed to commit new code having `FIXME`
>> comment. Committing  `FIXME` is allowed only for existing bugs. Comments
>> that are prefixed with `TODO` describe further features, optimization or
>> code improvements, and they are allowed to be committed along with the
>> relevant code.
>> 
>> -ASSERT
>> ---
>> +Assertions
>> +--
>> +
>> +Use assertions liberally. Assertions help testing function arguments and
>> function results validity. As a result, they make it easier to detect bugs.
>> Also, by expressing the intent of the developer, they make the code easier
>> to read.
>> +
> 
> I come from the Windows world and I found here in our case the "Use assertions
> liberally" a bit stronger if they are always used.

I’m sorry, I can’t make sense of this sentence. I believe you mean that the 
recommendation is too strong (not stronger) because we never disable them?

Do you have any performance numbers on the extra cost of assertions?


> Kind of always using
> address sanitizer compiled in even in production. Recently in spice-server
> we added code like:
> 
>if (ENABLE_EXTRA_CHECKS) {
>spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
>spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT);
>}
> 
> the ENABLE_EXTRA_CHECKS is always true/false and is a compiled in constant
> and is true only if explicitly enabled so the compiler will strip the entire
> checks if not enabled (but still checking for syntax).

Yes. This kind of code is the obvious result of being a bit confused on the 
usage of assertions.

That specific example is somewhat ridiculous, by the way. The two tests are one 
machine instruction each, I doubt they are even noticeable on any performance 
benchmark.

ENABLE_EXTRA_CHECKS is also used for additional checks that are somewhat 
expensive at run-time, like in display_channel_finalize. OK, but then why not 
make that a run-time flag instead of a compile-time flag? Unless the additional 
test code makes your code larger by 50%, but I doubt this is the case here…

> 
>> +Several form of assertion exist. SPICE does not use the language `assert`
>> much, but instead relies on the following variants:
>> +
>> +- `spice_assert`, defined in `common/log.h`, is never disabled at run-time
>> and prints an error if the assertion fails.
>> +
>> +- `g_assert` and other variants defined in glib such as `g_assert_null`, can
>> be diabled by setting `G_DISABLE_ASSERT` (which is never done in practice
>> for SPICE code), and emits a message through the g_assertion_message
>> function and its variants.
>> +
> 
> typo: "diabled" -> "disabled"
> No, our code as far as I remember should NEVER be compiled and used with
> G_DISABLE_ASSERT enabled.

I mention in the text that we don’t disable them in practice.

But why should that NEVER be done? (in uppercase, no less)? If some distro has 
a policy of building with assertions disabled, that’s their choice.

> Or maybe I'm confused by G_DISABLE_CHECKS ?
> OT: maybe we should check this in the code (I think better place is 
> common/log.c).

Check what? That G_DISABLE_ASSERT is not set?

> 
>> +The guidelines for selecting one or the other are not very clear from the
>> existing code. The `spice_assert` variant may be preferred for SPICE-only
>> code, as it makes it clearer that this assertion is coming from SPICE. The
>> `g_assert` and variants may be preferred for assertions related to the use
>> of glib.
>> 
>> -Use it freely. ASSERT helps testing function arguments and function results
>> validity.  ASSERT helps detecting bugs and improve code readability and
>> stability.
>> 
>> sizeof
>> --
> 
> Frediano

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

Re: [Spice-devel] [PATCH vdagent 1/2] vdagent-x11: remove clipboard handling

[Victor Toso]

Hi,

Follow up after the comment in the cover letter.

On Sun, Jan 21, 2018 at 09:03:13PM +0100, Jakub Janků wrote:
> From: Jakub Janků 
> 
> The code will be replaced by GTK+ in the following patch.
> ---
>  src/vdagent/vdagent.c  |7 -
>  src/vdagent/x11-priv.h |   91 
>  src/vdagent/x11.c  | 1074 
> +---
>  src/vdagent/x11.h  |   10 -
>  4 files changed, 1 insertion(+), 1181 deletions(-)
> 
> diff --git a/src/vdagent/vdagent.c b/src/vdagent/vdagent.c
> index d86ee25..92ffcf3 100644
> --- a/src/vdagent/vdagent.c
> +++ b/src/vdagent/vdagent.c
> @@ -164,18 +164,12 @@ static void daemon_read_complete(struct 
> udscs_connection **connp,
>  vdagent_x11_set_monitor_config(agent->x11, (VDAgentMonitorsConfig 
> *)data, 0);
>  break;
>  case VDAGENTD_CLIPBOARD_REQUEST:
> -vdagent_x11_clipboard_request(agent->x11, header->arg1, 
> header->arg2);
>  break;
>  case VDAGENTD_CLIPBOARD_GRAB:
> -vdagent_x11_clipboard_grab(agent->x11, header->arg1, (uint32_t 
> *)data,
> -   header->size / sizeof(uint32_t));
>  break;
>  case VDAGENTD_CLIPBOARD_DATA:
> -vdagent_x11_clipboard_data(agent->x11, header->arg1, header->arg2,
> -   data, header->size);
>  break;
>  case VDAGENTD_CLIPBOARD_RELEASE:
> -vdagent_x11_clipboard_release(agent->x11, header->arg1);
>  break;
>  case VDAGENTD_VERSION:
>  if (strcmp((char *)data, VERSION) != 0) {
> @@ -228,7 +222,6 @@ static void daemon_read_complete(struct udscs_connection 
> **connp,
>  }
>  break;
>  case VDAGENTD_CLIENT_DISCONNECTED:
> -vdagent_x11_client_disconnected(agent->x11);

So, instead of removing the vdagent_x11 calls, you could
introduce the clipboard.[ch] and use the renamed calls here that
will call the x11 code for now.

Reviewed-by: Victor Toso 

toso

>  if (vdagent_finalize_file_xfer(agent)) {
>  vdagent_init_file_xfer(agent);
>  }
> diff --git a/src/vdagent/x11-priv.h b/src/vdagent/x11-priv.h
> index 3776098..627e0ed 100644
> --- a/src/vdagent/x11-priv.h
> +++ b/src/vdagent/x11-priv.h
> @@ -1,118 +1,27 @@
>  #ifndef VDAGENT_X11_PRIV
>  #define VDAGENT_X11_PRIV
>  
> -#include 
> -#include 
> -
> -#include 
> -
>  #include 
>  
> -/* Macros to print a message to the logfile prefixed by the selection */
> -#define SELPRINTF(format, ...) \
> -syslog(LOG_ERR, "%s: " format, \
> -vdagent_x11_sel_to_str(selection), ##__VA_ARGS__)
> -
> -#define VSELPRINTF(format, ...) \
> -do { \
> -if (x11->debug) { \
> -syslog(LOG_DEBUG, "%s: " format, \
> -vdagent_x11_sel_to_str(selection), ##__VA_ARGS__); \
> -} \
> -} while (0)
> -
>  #define MAX_SCREENS 16
>  /* Same as qxl_dev.h client_monitors_config.heads count */
>  #define MONITOR_SIZE_COUNT 64
>  
> -enum { owner_none, owner_guest, owner_client };
> -
> -/* X11 terminology is confusing a selection request is a request from an
> -   app to get clipboard data from us, so iow from the spice client through
> -   the vdagent channel. We handle these one at a time and queue any which
> -   come in while we are still handling the current one. */
> -struct vdagent_x11_selection_request {
> -XEvent event;
> -uint8_t selection;
> -struct vdagent_x11_selection_request *next;
> -};
> -
> -/* A conversion request is X11 speak for asking another app to give its
> -   clipboard data to us, we do these on behalf of the spice client to copy
> -   data from the guest to the client. Like selection requests we process
> -   these one at a time. */
> -struct vdagent_x11_conversion_request {
> -Atom target;
> -uint8_t selection;
> -struct vdagent_x11_conversion_request *next;
> -};
> -
> -struct clipboard_format_tmpl {
> -uint32_t type;
> -const char *atom_names[16];
> -};
> -
> -struct clipboard_format_info {
> -uint32_t type;
> -Atom atoms[16];
> -int atom_count;
> -};
> -
>  struct monitor_size {
>  int width;
>  int height;
>  };
>  
> -static const struct clipboard_format_tmpl clipboard_format_templates[] = {
> -{ VD_AGENT_CLIPBOARD_UTF8_TEXT, { "UTF8_STRING", 
> "text/plain;charset=UTF-8",
> -  "text/plain;charset=utf-8", "STRING", NULL }, },
> -{ VD_AGENT_CLIPBOARD_IMAGE_PNG, { "image/png", NULL }, },
> -{ VD_AGENT_CLIPBOARD_IMAGE_BMP, { "image/bmp", "image/x-bmp",
> -  "image/x-MS-bmp", "image/x-win-bitmap", NULL }, },
> -{ VD_AGENT_CLIPBOARD_IMAGE_TIFF, { "image/tiff", NULL }, },
> -{ VD_AGENT_CLIPBOARD_IMAGE_JPG, { "image/jpeg", NULL }, },
> -};
> -
> -#define clipboard_format_count 
> (sizeof(clipboard_format_templates)/sizeof(clipboard_format_templates[0]))
> -
>  struct vdagent_x11 {
> -struct clipboard_format_info clipboard_formats[clipboard_format_count];
>  Display *display;
> -Atom 

Re: [Spice-devel] [PATCH vdagent 0/2] clipboard system redesign

[Victor Toso]

Hey,

Sorry for taking some time to review your patches!

On Sun, Jan 21, 2018 at 09:03:12PM +0100, Jakub Janků wrote:
> Hi,
> these two patches replace the current clipboard handling code,
> which uses Xlib, with code utilizing GTK+. The code was moved
> from x11.c to clipboard.c
>
> Although GTK+ can work on Wayland natively, this code currently
> works only under X. gdk_set_allowed_backends("x11") in
> vdagent.c is called to force GTK+ to always use X11 backend.
> 
> The implementation is partially based on the code in spice-gtk
> (spice-gtk-session.c).
> 
> Cheers,

Awesome work. Tested and seems to work as expected on F26 guest.
I'll be reviewing this inline but I'd like to suggest a different
approach to the patches.

For the clipboard, GTK+ is going to replace x11 fairly well and I
don't expect major problems but we might have some, we never
know. I would like to do one last vdagent release with x11
clipboard to explicit say that we will replace x11 with gtk+
after that. I think we can do a release as soon as this gets in.

So, removing x11 calls would be a problem.

Another issue is that, it is hard to review/compare when one
patch removes the feature and the other add it back besides the
fact that bisecting would be broken to that feature (although its
not a big problem in this case, I think).

My suggestion is to have a ./configure --with-gtk or --enable-gtk
to wrap new code around #ifdef GTK_ENABLED for the new code
otherwise it calls the x11 code as before.

I think the clipboard function functions exported by x11.h should
be moved to clipboard.[ch] and together with the proposed
VDAgentClipboards (interface) changes.

What do you think?
Cheers,
toso

> 
>  Makefile.am |2 +
>  src/vdagent/clipboard.c |  401 ++
>  src/vdagent/clipboard.h |   42 ++
>  src/vdagent/vdagent.c   |   38 +-
>  src/vdagent/x11-priv.h  |   91 
>  src/vdagent/x11.c   | 1074 
> +--
>  src/vdagent/x11.h   |   10 -
>  7 files changed, 472 insertions(+), 1186 deletions(-)
>  create mode 100644 src/vdagent/clipboard.c
>  create mode 100644 src/vdagent/clipboard.h
> 
> -- 
> 2.14.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] [RFC PATCH spice-streaming-agent v2] separate and encapsulate the agent business code

[Frediano Ziglio]

> 
> Create an AgentRunner (TODO: needs a better name) class to encapsulate
> the streaming and communication with the server. The basic setup (cmd

Also encapsulate command line interface (quite odd, does it make still
much sense?)

> arg parsing, signal handling, ...) is moved to main.cpp. The rest of the
> functions is moved to the AgentRunner class and modified as little as
> possible:
> - The cursor updating code is moved into a functor called CursorThread
> - Some initialization and cleanup is moved to AgentRunner's constructor
>   and destructor
> - Some error handling moved over to exceptions, mainly what was in
>   main() and do_capture()
> - A couple of variables gently renamed.
> 
> Signed-off-by: Lukáš Hrázký 
> ---
> Changes since v1:
> - update according to Frediano's namespace changes
> 
>  src/Makefile.am   |   2 +
>  src/main.cpp  | 126 
>  src/spice-streaming-agent.cpp | 458
>  +-
>  src/spice-streaming-agent.hpp |  57 ++
>  4 files changed, 372 insertions(+), 271 deletions(-)
>  create mode 100644 src/main.cpp
>  create mode 100644 src/spice-streaming-agent.hpp
> 
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 6d37274..42bb10f 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -49,6 +49,7 @@ spice_streaming_agent_LDADD = \
>  
>  spice_streaming_agent_SOURCES = \
>   spice-streaming-agent.cpp \
> + spice-streaming-agent.hpp \
>   static-plugin.cpp \
>   static-plugin.hpp \
>   concrete-agent.cpp \
> @@ -57,4 +58,5 @@ spice_streaming_agent_SOURCES = \
>   mjpeg-fallback.hpp \
>   jpeg.cpp \
>   jpeg.hpp \
> + main.cpp \
>   $(NULL)
> diff --git a/src/main.cpp b/src/main.cpp
> new file mode 100644
> index 000..46eda85
> --- /dev/null
> +++ b/src/main.cpp
> @@ -0,0 +1,126 @@
> +/* An implementation of a SPICE streaming agent
> + *
> + * \copyright
> + * Copyright 2016-2018 Red Hat Inc. All rights reserved.
> + */
> +
> +#include 
> +#include "spice-streaming-agent.hpp"
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +
> +namespace ssa = spice::streaming_agent;
> +
> +static void usage(const char *progname)
> +{
> +printf("usage: %s \n", progname);
> +printf("options are:\n");
> +printf("\t-p portname  -- virtio-serial port to use\n");
> +printf("\t-i accept commands from stdin\n");
> +printf("\t-l file -- log frames to file\n");
> +printf("\t--log-binary -- log binary frames (following -l)\n");
> +printf("\t-d -- enable debug logs\n");
> +printf("\t-c variable=value -- change settings\n");
> +printf("\t\tframerate = 1-100 (check 10,20,30,40,50,60)\n");
> +printf("\n");
> +printf("\t-h or --help -- print this help message\n");
> +
> +exit(1);
> +}
> +
> +void handle_interrupt(int intr)
> +{
> +syslog(LOG_INFO, "Got signal %d, exiting", intr);
> +ssa::AgentRunner::quit = true;
> +}
> +
> +void register_interrupts(void)
> +{
> +struct sigaction sa;
> +
> +memset(&sa, 0, sizeof(sa));
> +sa.sa_handler = handle_interrupt;
> +if ((sigaction(SIGINT, &sa, NULL) != 0) &&
> +(sigaction(SIGTERM, &sa, NULL) != 0)) {
> +syslog(LOG_WARNING, "failed to register signal handler %m");
> +}
> +}
> +
> +int main(int argc, char* argv[])
> +{
> +std::string stream_port = "/dev/virtio-ports/com.redhat.stream.0";
> +char opt;
> +std::string log_filename;
> +int log_binary = 0;
> +bool stdin_ok = false;
> +int logmask = LOG_UPTO(LOG_WARNING);
> +struct option long_options[] = {
> +{ "log-binary", no_argument, &log_binary, 1},
> +{ "help", no_argument, NULL, 'h'},
> +{ 0, 0, 0, 0}
> +};
> +
> +if (isatty(fileno(stderr)) && isatty(fileno(stdin))) {
> +stdin_ok = true;
> +}
> +
> +openlog("spice-streaming-agent", stdin_ok? (LOG_PERROR|LOG_PID) :
> LOG_PID, LOG_USER);
> +setlogmask(logmask);
> +
> +std::vector> options;
> +
> +while ((opt = getopt_long(argc, argv, "hip:c:l:d", long_options, NULL))
> != -1) {
> +switch (opt) {
> +case 0:
> +/* Handle long options if needed */
> +break;
> +case 'i':
> +stdin_ok = true;
> +openlog("spice-streaming-agent", LOG_PERROR|LOG_PID, LOG_USER);
> +break;
> +case 'p':
> +stream_port = optarg;
> +break;
> +case 'c': {
> +char *p = strchr(optarg, '=');
> +if (p == NULL) {
> +syslog(LOG_ERR, "wrong 'c' argument %s\n", optarg);
> +usage(argv[0]);
> +}
> +*p++ = '\0';
> +options.push_back(std::make_pair(optarg, p));
> +break;
> +}
> +case 'l':
> +log_filename = optarg;
> +break;
> +case 'd':
> +logmask = LOG_UPTO(LOG_DEBUG);
> +setlogmask(log

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

[Frediano Ziglio]

See https://lists.freedesktop.org/archives/spice-devel/2018-February/041724.html

> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  docs/spice_style.txt | 11 ++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index 10bfbc9a..13032df6 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -34,7 +34,16 @@ Names
>  
>  Use lower case and separate words using dashes (e.g., file-name.c,
>  header.h).
>  
> -Use standard file extension for C source and header files.
> +The file extensions used in the SPICE project are:
> +- .c for C source
> +- .cpp for C++ sources
> +- .h for headers that can be included from C code
> +- .hpp for headers that are strictly reserved to C++
> +- .m for Objective-C source files (currently not properly enforced)
> +
> +Note that .h headers may contain C++ code as long as the header can
> +sucessfully be included from a C source file.
> +
>  
>  Line width
>  ~~
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v3 07/11] Remove confusing example

[Frediano Ziglio]

See https://lists.freedesktop.org/archives/spice-devel/2018-February/041727.html

> From: Christophe de Dinechin 
> 
> The sentence explaining that example makes no real sense, and
> the coding style suggestion is horrendous (not to mention flies in the
> face of all automatic indentation tools)
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  docs/spice_style.txt | 10 +-
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index 486efbc9..68dbeeef 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -279,15 +279,7 @@ if (condition) {
>  +
>  In case of long condition statement, prefer having additional temporary
>  variable over multiple line condition statement.
>  +
> -In case of new line in condition statement.
> -+
> -[source,c]
> -
> -if (long_name && very_long_name && very_long ||
> -   var_name) {
> -
> -+
> -or indent under the round bracket using spaces
> +Indent long conditionals under the opening parenthesis using spaces
>  +
>  [source,c]
>  
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v3 04/11] Rephrase assertion section

[Frediano Ziglio]

> 
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  docs/spice_style.txt | 15 ---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index 13032df6..eef4880f 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -99,10 +99,19 @@ FIXME and TODO
>  
>  Comments that are prefixed with `FIXME` describe a bug that need to be
>  fixed. Generally, it is not allowed to commit new code having `FIXME`
>  comment. Committing  `FIXME` is allowed only for existing bugs. Comments
>  that are prefixed with `TODO` describe further features, optimization or
>  code improvements, and they are allowed to be committed along with the
>  relevant code.
>  
> -ASSERT
> ---
> +Assertions
> +--
> +
> +Use assertions liberally. Assertions help testing function arguments and
> function results validity. As a result, they make it easier to detect bugs.
> Also, by expressing the intent of the developer, they make the code easier
> to read.
> +

I come from the Windows world and I found here in our case the "Use assertions
liberally" a bit stronger if they are always used. Kind of always using
address sanitizer compiled in even in production. Recently in spice-server
we added code like:

if (ENABLE_EXTRA_CHECKS) {
spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT);
}

the ENABLE_EXTRA_CHECKS is always true/false and is a compiled in constant
and is true only if explicitly enabled so the compiler will strip the entire
checks if not enabled (but still checking for syntax).

> +Several form of assertion exist. SPICE does not use the language `assert`
> much, but instead relies on the following variants:
> +
> +- `spice_assert`, defined in `common/log.h`, is never disabled at run-time
> and prints an error if the assertion fails.
> +
> +- `g_assert` and other variants defined in glib such as `g_assert_null`, can
> be diabled by setting `G_DISABLE_ASSERT` (which is never done in practice
> for SPICE code), and emits a message through the g_assertion_message
> function and its variants.
> +

typo: "diabled" -> "disabled"
No, our code as far as I remember should NEVER be compiled and used with
G_DISABLE_ASSERT enabled. Or maybe I'm confused by G_DISABLE_CHECKS ?
OT: maybe we should check this in the code (I think better place is 
common/log.c).

> +The guidelines for selecting one or the other are not very clear from the
> existing code. The `spice_assert` variant may be preferred for SPICE-only
> code, as it makes it clearer that this assertion is coming from SPICE. The
> `g_assert` and variants may be preferred for assertions related to the use
> of glib.
>  
> -Use it freely. ASSERT helps testing function arguments and function results
> validity.  ASSERT helps detecting bugs and improve code readability and
> stability.
>  
>  sizeof
>  --

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

Re: [Spice-devel] [PATCH spice-server v4 0/4] Better handling reset for streaming device

[Frediano Ziglio]

> 
> On 01/30/2018 04:33 PM, Christophe de Dinechin wrote:
> > 
> > Christophe de Dinechin writes:
> > 
> >>> On 30 Jan 2018, at 12:56, Frediano Ziglio  wrote:
> >>>
> 
>  Hi Frediano,
> 
> 
> 
> > On 30 Jan 2018, at 11:50, Frediano Ziglio  wrote:
> >
> > ping the series
> 
>  I’m currently looking at it. Is it supposed to fix the read errors I had
>  when
>  restarting the streaming agent?
> 
> >>>
> >>> Yes, make the reset more stable.
> >>> When you close the device the state will be more consistent allowing
> >>> basically to kill the process using the device in any state and opening
> >>> again. Obviously if you continue to send wrong commands the device will
> >>> keep rejecting them.
> >>>
> >>> I tried to reproduce the issues reported on IRC and these helped me,
> >>> now I avoid entirely to reboot the guest.
> >>
> >> OK, right now I get a QEMU crash whenever I do any kind of activity
> >> (the keyboard seems to be what triggers it).
> >>
> >> I’m trying to reproduce on master to see if your patch is the cause.
> >> That host has gone through some unusual nastiness, and may be
> >> in a geborked state.
> > 
> > Reverting the server to master, I am back to the behavior I had before,
> > where the same series of events leads to
> > 
> > DISPLAY=:1 spice-streaming-agent -c noblock=yes
> > spice-streaming-agent[2240]: UNKNOWN msg of type 5
> > spice-streaming-agent[2240]: BAD VERSION 0 (expected is 1)
> > spice-streaming-agent[2240]: BAD VERSION 108 (expected is 1)
> > spice-streaming-agent[2240]: BAD VERSION 97 (expected is 1)
> > spice-streaming-agent[2240]: read command from device FAILED -- read 1
> > expected 8
> > spice-streaming-agent[2240]: FAILED to read command
> 
> 
> Hi Christophe,
> 
> There are some problems here:
> 1. (I guess) your spice-streaming-agent sends CURSOR messages
> which currently the spice-server does not know to handle.
> (Frediano sent patches for that but still no review).
> 
> For now try to comment out the code in spice-streaming-agent
> that sends CURSOR commands.
> 

Would not make sense to review the patches instead of having
to write another workaround also taking into account that 
the review process is taking more than 6 months?

> 2. spice-server gets an unknown command. It sends a message to the
> spice-streaming-agent to let it know the server read an invalid
> message. spice-streaming-agent is missing a code to handle such
> a message.
> 
> This should be fixed.
> 
> 3. When such messages are in play, they are not fully read (code
> breaks after reading the header). This makes the spice-server
> and spice-streaming-agent go out of sync).
> 
> This may be fixed. I'm not sure we can recover
> all errors, and perhaps the right thing to do is sync
> with close/open of the virtio-serial-port.

This is what this patch series is doing.

> However reading the whole message (if it's not too big) should
> at least keep the server/agent synchronized, even if an unknown
> message encountered.
> 

No, is not enough. If the message is expected to change the server
state discarding the message will make the dialog out of sync anyway.
Would make sense if the server could send back an error for a specific
message (currently if you send a batch of messages you don't know
the relationship between error and message). Also would mean that
the client MUST handle the error from a message and as the messages
replies are async to keep a queue of messages.
To me it seems that at the end would be just more complicated than
expected instead of graceful.

> Regards,
>  Uri.
> 
> 

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