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

[Christophe Fergeau]

On Thu, Feb 15, 2018 at 05:06:19PM +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  docs/spice_style.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index 996c5cd9..9e6536e4 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -178,7 +178,7 @@ If multiple related constants are to be defined, consider 
> the use of enumeration
>  Short functions
>  ---
>  
> -Try to split code to short functions, each having simple functionality, in 
> order to improve code readability and re-usability. Prefix with inline short 
> functions or functions that were splitted for readability reason.
> +Try to split code to short functions, each having simple functionality, in 
> order to improve code readability and re-usability. Prefix with `inline` 
> functions that were splitted for readability reason or that are very short.

I'd just drop the second sentence. The compiler usually knows better.

Christophe


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

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

[Christophe Fergeau]

On Thu, Feb 15, 2018 at 05:06:21PM +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin 
> 
> The objective of these guidelines is that:
> - We avoid introducing new warnings
> - We know how to fix old ones
> 
> Changes since v3:
> - Put 'controversial' whitespace proposal in separate patch
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  docs/spice_style.txt | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index 977ce154..90ad577d 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -523,3 +523,10 @@ using spice::streaming_agent::some_class;
>  +
>  [source,cpp]
>  namespace ssa = spice::streaming_agent;
> +
> +Compilation
> +---
> +
> +The source code should compile without warnings on all variants of GCC and 
> clang available.
> +A patch may be rejected if it introduces new warnings.
> +Warnings that appear over time due to improvements in compilers should be 
> fixed in dedicated patches. A patch should not mix warning fixes and other 
> changes.

As said earlier, I don't think this brings much to explicitly states
this. And this is both too specific, while leaving some cases undefined,
this does not define a process to fix warnings which were not noticed at
the time the patch was ack'ed. But really, I would just drop this patch.
This could even be frightening to newcomers ("what, I have to test my
patch on dozens of compilers??"), while no regular committer is actually
going to do it.

Christophe


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

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

[Frediano Ziglio]

> > On 15 Feb 2018, at 11:00, Frediano Ziglio  wrote:
> > 
> > From: Christophe de Dinechin 
> > 
> > Signed-off-by: Christophe de Dinechin 
> > ---
> > Change since v1:
> > - do not clash with possible short 'b' option.
> > ---
> > src/spice-streaming-agent.cpp | 14 ++
> > 1 file changed, 10 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> > index 4ec5e42..aa73826 100644
> > --- a/src/spice-streaming-agent.cpp
> > +++ b/src/spice-streaming-agent.cpp
> > @@ -55,9 +55,9 @@ struct SpiceStreamDataMessage
> > 
> > static bool streaming_requested = false;
> > static bool quit_requested = false;
> > +static bool log_binary = false;
> > static std::set client_codecs;
> > static int streamfd = -1;
> > -static int log_binary = 0;
> > static std::mutex stream_mtx;
> > 
> > static int have_something_to_read(int timeout)
> > @@ -405,11 +405,14 @@ done:
> > int main(int argc, char* argv[])
> > {
> > std::string streamport = "/dev/virtio-ports/com.redhat.stream.0";
> > -char opt;
> > +int opt;
> > const char *log_filename = NULL;
> > int logmask = LOG_UPTO(LOG_WARNING);
> > -struct option long_options[] = {
> > -{ "log-binary", no_argument, _binary, 1},
> > +enum {
> > +OPT_LOG_BINARY = UCHAR_MAX+1
> > +};
> > +static const struct option long_options[] = {
> > +{ "log-binary", no_argument, NULL, OPT_LOG_BINARY},
> 
> Wouldn’t it be simpler to add the -b option for binary log, since we don’t
> use it?
> 
> That was my intent, and then it slipped my mind…
> 

This patch is about style and type, adding an option is a public
interface change.
I don't think that this option that requires a short option.

> 
> > { "help", no_argument, NULL, 'h'},
> > { 0, 0, 0, 0}
> > };
> > @@ -437,6 +440,9 @@ int main(int argc, char* argv[])
> > agent.AddOption(optarg, p);
> > break;
> > }
> > +case OPT_LOG_BINARY:
> > +log_binary = true;
> > +break;
> > case 'l':
> > log_filename = optarg;
> > break;

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

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

[Christophe Fergeau]

The shortlog is not really good..

On Thu, Feb 15, 2018 at 05:06:23PM +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin 
> 
> This attempts to find a wording that takes into account the existing
> practice, notably in the server.
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  docs/spice_style.txt | 16 +---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index c28d7be3..4c29a410 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -432,12 +432,22 @@ Headers should be protected against multiple inclusion 
> using a macro that contai
>  
>  ...
>  
> -#endif // MY_MODULE_H
> +#endif /* MY_MODULE_H */
>  
>  
> -The macro may include additional information, e.g. a component. For example 
> a file generally referenced as foo/bar.h could use a FOO_BAR_H macro.
> +The macro may include additional information, e.g. a component. For
> +example a file generally referenced as foo/bar.h could use a FOO_BAR_H
> +macro.
> +
> +Historically, some headers added underscores liberally,
> +e.g. MY_MODULE_H_. This is neither necessary nor discouraged, although
> +as a reminder, a leading underscore followed by a capital letter is
> +reserved for the implementation and should not be used, so
> +_MY_MODULE_H is, technically speaking, invalid C.

Just drop all of this, we want a guideline which says
"your header guards should be MY_MODULE_H".
We don't want a  guideline which says "Your header guard should be
MY_MODULE_H. Or FOO_MY_MODULE_H. Or if you prefer _MY_MODULE_H even if
it's wrong".

Christophe

> +
> +C++ headers and headers in new components should use // for the #endif 
> comment.
> +The server code consistently uses /* */ for the #endif comment, keep it that 
> way.
>  
> -Historically, some headers added underscores liberally, e.g. MY_MODULE_H_. 
> This is neither necessary nor discouraged, although as a reminder, a leading 
> underscore followed by a capital letter is reserved for the implementation 
> and should not be used, so _MY_MODULE_H is, technically speaking, invalid C.
>  
>  Header inclusion
>  
> -- 
> 2.13.5 (Apple Git-94)
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

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

[Frediano Ziglio]

> 
> > On 9 Feb 2018, at 17:59, Frediano Ziglio  wrote:
> > 
>  
>  
> > +
> > +// read part of the message till we get all
> > +if (dev->msg_len < dev->hdr.size) {
> > +dev->msg = g_realloc(dev->msg, dev->hdr.size);
> > +dev->msg_len = dev->hdr.size;
> > +}
> > +int n = sif->read(sin, dev->msg->buf + dev->msg_pos, dev->hdr.size
> > -
> > dev->msg_pos);
> > +if (n <= 0) {
> > +return false;
> > +}
> > +dev->msg_pos += n;
> > +if (dev->msg_pos != dev->hdr.size) {
>  
>  Is it assumed you can all read in one go? I’m surprized there is a
>  “while”
>  loop for reading the header (which is small) but no while loop for
>  reading
>  the payload which is larger).
>  
> >>> 
> >>> When we return false is not an error, just there's no data left next time
> >>> will add more data. So there's no while but there's a loop anyway.
> >>> I suppose similarly we could avoid the while in the other function.
> >> 
> >> Oh, so you are saying that you think it’s OK to loop ouside, truncate the
> >> data that you g_realloc’d, and then start over?
> >> 
> >> Does not work. Say you started with 1K in buffer, then go to this inner
> >> read
> >> here, which does a g_realloc and reads 10K. Then we re-enter the above
> >> function, so I believe we will truncate to 1K again, and then re-extend to
> >> 10K and start reading at 10K. We lost 9K of data, unless I’m mistaken.
> >> 
> > 
> > No, the header is not read again until all message is processed.
> > There are also tests for this.
> 
> This is not what I am talking about. I’m saying you read this, where H is
> header and D is data.
> 
> You first read header, let’s say four items.
> 
> []
> 
> Then you realloc, which puts some undefined garbage in the area the data:
> 
> [][]
> 
> Then you read data, get 4 elements:
> 
> [][]
> 
> So you restart in the outer loop, where you truncate the header
> 

why truncate the header?

The 

while (dev->hdr_pos < sizeof(dev->hdr)) {

condition is not taken and we get back reading the partial message.

> []
> 
> and reallocate
> 
> [][]
> 
> and restart reading, not sure if we have reset msg_pos or not, so you get
> either
> 
> [][]
> 
> or
> 
> [][]
> 
> 
> None of which is correct. I’ve not seen in the code how we prevent this if we
> don’t loop in the inner read.
> 
> Maybe I missed the part that makes us restart correctly, but where did we
> keep the data?
> 
> 
> Regards
> Christophe
> 
> 

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

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

[Christophe Fergeau]

On Thu, Feb 15, 2018 at 05:06:17PM +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin 
> 
> Changes since v3:
> - Integrate comments about performance impact and solution
> - Integrate comments about impact of a failed assertion
> - Add prohibition against side effects
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  docs/spice_style.txt | 36 +---
>  1 file changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index 5dc41cb1..3bc70570 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -113,10 +113,40 @@ 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, but pay attention to performance impact. 
> Assertions checks have a significant performance impact should be placed 
> under the `ENABLE_EXTRA_CHECKS` conditional.

[snip]

> +
> +Be mindful of the impact of a failing assertion. For example, assertions in 
> the server should not abort it, as this would kill the QEMU process and the 
> virtual machine.

For me, "assertion" implies abortion of the process, so the first
sentence ("use assertions liberally") and this one are confusing.
In particular, g_assert() which you mention in the snipped part does
abort.

Christophe


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

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

[Christophe Fergeau]

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

'way'

>heavy editing or refactoring.
> 
> 2. Global reformatting of the source code, e.g. using the 'make reformat'
>rule in the 'build' makefiles.
> 
> Presently, our source code is not entirely ready for this kind of
> refactoring yet, mostly because several of our headers are not
> self-contained, so you end up with errors like:
> 
> In file included from agent-msg-filter.c:25:
> In file included from ./red-common.h:37:
> In file included from ./spice.h:27:
> ./spice-migration.h:47:31: error: unknown type name 'SpiceServer'

My understanding was also that this .clang-format file got the
formatting wrong in some cases when applied over the codebase?



> Changes since v3:
> - Documented rationale and use cases in commit log
> - Some setting changes to more closely match existing server practice

NB: This part belongs below the -- line as this usually should not be pushed
to the upstream repository


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

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

[Christophe Fergeau]

On Thu, Feb 15, 2018 at 05:06:25PM +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin 
> 
> Change since v3:
> - This particular aspect is controversial, so it was put in a separate patch.

This has been nack'ed by multiple people already, please don't resend it
unchanged.

Christophe

> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  docs/spice_style.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index 13bd3809..0cbd3570 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -559,3 +559,5 @@ Compilation
>  The source code should compile without warnings on all variants of GCC and 
> clang available.
>  A patch may be rejected if it introduces new warnings.
>  Warnings that appear over time due to improvements in compilers should be 
> fixed in dedicated patches. A patch should not mix warning fixes and other 
> changes.
> +
> +Any patch may adjust whitespace (e.g. eliminate trailing whitespace). 
> Whitespace adjustments do not require specific patches
> -- 
> 2.13.5 (Apple Git-94)
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel


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

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

[Christophe de Dinechin]

> On 16 Feb 2018, at 10:07, Christophe Fergeau  wrote:
> 
> On Thu, Feb 15, 2018 at 05:06:14PM +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin 
>> 
>> There are two use cases for this .clang-format:
>> 
>> 1. Local reformatting of source code, e.g. using Emacs clang-format.el 
>> package.
>>   This is basically a wawy to accelerate routine reformatting actions during
> 
> 'way'
> 
>>   heavy editing or refactoring.
>> 
>> 2. Global reformatting of the source code, e.g. using the 'make reformat'
>>   rule in the 'build' makefiles.
>> 
>> Presently, our source code is not entirely ready for this kind of
>> refactoring yet, mostly because several of our headers are not
>> self-contained, so you end up with errors like:
>> 
>>In file included from agent-msg-filter.c:25:
>>In file included from ./red-common.h:37:
>>In file included from ./spice.h:27:
>>./spice-migration.h:47:31: error: unknown type name 'SpiceServer'
> 
> My understanding was also that this .clang-format file got the
> formatting wrong in some cases when applied over the codebase?

I did improve the settings, see below “some setting changes”.

Frediano’s comment was based on a push where I had not yet noticed that

1. LLVM defaults to aligning decls, so I added AlignConsecutiveAssignments: 
false and AlignConsecutiveDeclarations: false.
2. The ‘Linux’ brace alignment was not correct for structs, which prompted me 
to change it that setting to ‘Mozilla'

The result after these changes is here: 
https://gitlab.com/c3d/spice-server/commit/19e7c5bb97b0ce9d489c16229ab9caf39080145d

There are still issues, but they seem to be more related to:

- Misinterpretation of some macro code, e.g. 
https://gitlab.com/c3d/spice-server/commit/19e7c5bb97b0ce9d489c16229ab9caf39080145d#c75ac8e2c001e819bae810b126f19ba02f1bac2a_35_35

- Special manual formatting, e.g. 
https://gitlab.com/c3d/spice-server/commit/19e7c5bb97b0ce9d489c16229ab9caf39080145d#1f4f5fab2bf5fd2a2296b15eba820fc32802b59d_239_235.
 Those can be protected with a "// clang-format off” comment.

- The return type on a separate line is systematically removed. I don’t know if 
we want that or not. We could set AlwaysBreakAfterDefinitionReturnType: 
TopLevel.

For the rest, I think it’s doing a rather good job, and there are many places 
where it markedly improves things.


> 
> 
> 
>> Changes since v3:
>> - Documented rationale and use cases in commit log
>> - Some setting changes to more closely match existing server practice
> 
> NB: This part belongs below the -- line as this usually should not be pushed
> to the upstream repository

Ah, yes. Thanks.

> ___
> 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-gtk v2 0/4] Add spice+tls:// uri form

[marcandre . lureau]

From: Marc-André Lureau 

Hi,

Here are a few patches related to spice schemes URI handling. They
introduce a spice+tls:// form (see related man page update and patch
for details).

cheers

v2:
- add a patch to report warning with original uri for clarity
- accept query string in spice+tls:// form, but reject 'port' or
  'tls-port' that are only for spice://.
- update tests
- rebased

Marc-André Lureau (4):
  session: warn with the original URI
  uri: learn to parse spice+tls:// form
  uri: generate spice://host:port or spice+tls://host:port
  tests: add spice+tls:// tests

 man/spice-client.pod | 29 +++---
 src/spice-session.c  | 58 +++-
 tests/session.c  | 44 +--
 3 files changed, 94 insertions(+), 37 deletions(-)

-- 
2.16.1.73.g5832b7e9f2

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

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

[marcandre . lureau]

From: Marc-André Lureau 

spice:// has a weird scheme encoding, where it can accept both plain
and tls ports with URI query parameters. However, it's not very
convenient nor very common to use (who really want to mix plain & tls
channels?).

Instead, let's introduce the more readable form spice+tls://host:port
This form will not accept 'port' or 'tls-port' query string parameter.

Signed-off-by: Marc-André Lureau 
---
 man/spice-client.pod | 29 +
 src/spice-session.c  | 23 +++
 2 files changed, 36 insertions(+), 16 deletions(-)

diff --git a/man/spice-client.pod b/man/spice-client.pod
index 7288b84..459e5f1 100644
--- a/man/spice-client.pod
+++ b/man/spice-client.pod
@@ -12,23 +12,24 @@ can be used to tweak some SPICE-specific option.
 
 =head1 URI
 
-The most basic SPICE URI which can be used is in the form
+To initiate a plain SPICE connection (the connection will be
+unencrypted) to hostname.example.com and port 5900, use the following
+URI:
+
   spice://hostname.example.com:5900
 
-This will try to initiate a SPICE connection to hostname.example.com
-to port 5900. This connection will be unencrypted. This URI is
-equivalent to
-  spice://hostname.example.com?port=5900
+In order to start a TLS connection, one would use:
 
-In order to start a TLS connection, one would use
-  spice://hostname.example.com?tls-port=5900
+  spice+tls://hostname.example.com:5900
 
-Other valid URI parameters are 'username' and 'password'. Be careful that
-passing a password through a SPICE URI might cause the password to be
-visible by any local user through 'ps'.
+Note: 'spice+tls' is available since v0.35, you have to use the
+spice:// query string with the 'tls-port' parameter before that.
+
+=head1 URI query string
+
+spice URI accepts query string. Several parameters can be specified at
+once if they are separated by & or ;
 
-Several parameters can be specified at once if they are separated
-by & or ;
   spice://hostname.example.com?port=5900;tls-port=5901
 
 When using 'tls-port', it's recommended to not specify any non-TLS port.
@@ -39,6 +40,10 @@ then try to use the TLS port. This means a man-in-the-middle 
could force
 the whole SPICE session to go in clear text regardless of the TLS settings
 of the SPICE server.
 
+Other valid URI parameters are 'username' and 'password'. Be careful that
+passing a password through a SPICE URI might cause the password to be
+visible by any local user through 'ps'.
+
 =head1 OPTIONS
 
 The following options are accepted when running a SPICE client which
diff --git a/src/spice-session.c b/src/spice-session.c
index e6db424..d2aa5e7 100644
--- a/src/spice-session.c
+++ b/src/spice-session.c
@@ -389,6 +389,7 @@ spice_session_finalize(GObject *gobject)
 
 #define URI_SCHEME_SPICE "spice://"
 #define URI_SCHEME_SPICE_UNIX "spice+unix://"
+#define URI_SCHEME_SPICE_TLS "spice+tls://"
 #define URI_QUERY_START ";?"
 #define URI_QUERY_SEP   ";&"
 
@@ -425,6 +426,7 @@ static int spice_parse_uri(SpiceSession *session, const 
char *original_uri)
 gchar *authority = NULL;
 gchar *query = NULL;
 gchar *tmp = NULL;
+bool tls_scheme = false;
 
 g_return_val_if_fail(original_uri != NULL, -1);
 
@@ -438,12 +440,16 @@ static int spice_parse_uri(SpiceSession *session, const 
char *original_uri)
 /* Break up the URI into its various parts, scheme, authority,
  * path (ignored) and query
  */
-if (!g_str_has_prefix(uri, URI_SCHEME_SPICE)) {
+if (g_str_has_prefix(uri, URI_SCHEME_SPICE)) {
+authority = uri + strlen(URI_SCHEME_SPICE);
+} else if (g_str_has_prefix(uri, URI_SCHEME_SPICE_TLS)) {
+authority = uri + strlen(URI_SCHEME_SPICE_TLS);
+tls_scheme = true;
+} else {
 g_warning("Expected a URI scheme of '%s' in URI '%s'",
   URI_SCHEME_SPICE, uri);
 goto fail;
 }
-authority = uri + strlen(URI_SCHEME_SPICE);
 
 tmp = strchr(authority, '@');
 if (tmp) {
@@ -531,6 +537,11 @@ static int spice_parse_uri(SpiceSession *session, const 
char *original_uri)
 if (*query)
 query++;
 
+if (tls_scheme && (g_str_equal(key, "port") || g_str_equal(key, 
"tls-port"))) {
+g_warning(URI_SCHEME_SPICE_TLS " scheme doesn't accept '%s'", key);
+continue;
+}
+
 target_key = NULL;
 if (g_str_equal(key, "port")) {
 target_key = 
@@ -568,8 +579,12 @@ end:
 s->unix_path = g_strdup(path);
 g_free(uri);
 s->host = host;
-s->port = port;
-s->tls_port = tls_port;
+if (tls_scheme) {
+s->tls_port = port;
+} else {
+s->port = port;
+s->tls_port = tls_port;
+}
 s->username = username;
 s->password = password;
 return 0;
-- 
2.16.1.73.g5832b7e9f2

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

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

[Christophe Fergeau]

On Thu, Feb 15, 2018 at 05:06:17PM +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin 
> 
> Changes since v3:
> - Integrate comments about performance impact and solution
> - Integrate comments about impact of a failed assertion
> - Add prohibition against side effects
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  docs/spice_style.txt | 36 +---
>  1 file changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index 5dc41cb1..3bc70570 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -113,10 +113,40 @@ 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
> +--
> +
> +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.
> +
> +Assertions checks have a significant performance impact should be placed 
> under the `ENABLE_EXTRA_CHECKS` conditional.

"which have" ?

> +
> +[source,c]
> +
> +void function(void)
> +{
> +while (condition()) { // Hot loop
> +spice_assert(ENABLE_EXTRA_CHECKS && expensive_check());
> +}
> +...
> +}
> +
> +
> +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 disabled 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.

I would expect program abortion to be mentioned here in addition to the
printing of the message.

> +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.

I'd remove this and just say to use g_*

> +
> +Be mindful of the impact of a failing assertion. For example, assertions in 
> the server should not abort it, as this would kill the QEMU process and the 
> virtual machine.
> +
> +Assertions should not:
> +
> +- Be used as a replacement for proper error management or to check unsafe 
> data such as user input
> +- Have side effects, since an assertion check may be disabled

*if* we really plan to disable assertions in some cases, I think we'd
need to be more explicit about when it's going to happen. If we don't
consider it a valid usecase to disable assertions, then we can drop
this.


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

Re: [Spice-devel] [PATCH spice-streaming-agent v2 2/2] explicit instead of static registration for built-in plugins

[Frediano Ziglio]

> 
> > On 6 Feb 2018, at 10:51, Frediano Ziglio  wrote:
> > 
> >>> 
> >>> On 5 Feb 2018, at 15:29, Lukáš Hrázký  wrote:
> >>> 
> >>> The static registration (that is, having a static list of pointers to
> >>> static plugin variables and calling a generic function in the main
> >>> initialization code path to register them) allows to add plugins without
> >>> registering each of them explicitly.
> >>> 
> >>> However, in a single codebase, and having very few plugins, there is
> >>> very little advantage to it and the tradeoff for the complexity of this
> >>> initialization is not worth it. A single call for every built-in plugin
> >>> is much more simple and clear.
> >>> 
> >>> Signed-off-by: Lukáš Hrázký 
> >>> ---
> >>> src/Makefile.am   |  2 --
> >>> src/concrete-agent.cpp|  3 ---
> >>> src/mjpeg-fallback.cpp|  6 +-
> >>> src/mjpeg-fallback.hpp|  1 +
> >>> src/spice-streaming-agent.cpp |  4 
> >>> src/static-plugin.cpp | 23 ---
> >>> src/static-plugin.hpp | 35 ---
> >>> 7 files changed, 6 insertions(+), 68 deletions(-)
> >>> delete mode 100644 src/static-plugin.cpp
> >>> delete mode 100644 src/static-plugin.hpp
> >>> 
> >>> diff --git a/src/Makefile.am b/src/Makefile.am
> >>> index 8d5c5bd..590db1f 100644
> >>> --- a/src/Makefile.am
> >>> +++ b/src/Makefile.am
> >>> @@ -49,8 +49,6 @@ spice_streaming_agent_LDADD = \
> >>> 
> >>> spice_streaming_agent_SOURCES = \
> >>>   spice-streaming-agent.cpp \
> >>> - static-plugin.cpp \
> >>> - static-plugin.hpp \
> >>>   concrete-agent.cpp \
> >>>   concrete-agent.hpp \
> >>>   mjpeg-fallback.cpp \
> >>> diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
> >>> index 873a69e..c69da43 100644
> >>> --- a/src/concrete-agent.cpp
> >>> +++ b/src/concrete-agent.cpp
> >>> @@ -11,7 +11,6 @@
> >>> #include 
> >>> 
> >>> #include "concrete-agent.hpp"
> >>> -#include "static-plugin.hpp"
> >>> 
> >>> using namespace std;
> >>> using namespace SpiceStreamingAgent;
> >>> @@ -58,8 +57,6 @@ void ConcreteAgent::AddOption(const char *name, const
> >>> char *value)
> >>> 
> >>> void ConcreteAgent::LoadPlugins(const string )
> >>> {
> >>> -StaticPlugin::InitAll(*this);
> >>> -
> >>>string pattern = directory + "/*.so";
> >>>glob_t globbuf;
> >>> 
> >>> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> >>> index 7c918a7..d7c676d 100644
> >>> --- a/src/mjpeg-fallback.cpp
> >>> +++ b/src/mjpeg-fallback.cpp
> >>> @@ -15,7 +15,6 @@
> >>> #include 
> >>> #include 
> >>> 
> >>> -#include "static-plugin.hpp"
> >>> #include "jpeg.hpp"
> >>> 
> >>> using namespace std;
> >>> @@ -191,8 +190,7 @@ SpiceVideoCodecType MjpegPlugin::VideoCodecType()
> >>> const
> >>> {
> >>>return SPICE_VIDEO_CODEC_TYPE_MJPEG;
> >>> }
> >>> 
> >>> -static bool
> >>> -mjpeg_plugin_init(Agent* agent)
> >>> +bool MjpegPlugin::Register(Agent* agent)
> >>> {
> >>>if (agent->Version() != PluginVersion)
> >>>return false;
> >>> @@ -205,5 +203,3 @@ mjpeg_plugin_init(Agent* agent)
> >>> 
> >>>return true;
> >>> }
> >>> -
> >>> -static StaticPlugin mjpeg_plugin(mjpeg_plugin_init);
> >>> diff --git a/src/mjpeg-fallback.hpp b/src/mjpeg-fallback.hpp
> >>> index 8044244..0e9ed6a 100644
> >>> --- a/src/mjpeg-fallback.hpp
> >>> +++ b/src/mjpeg-fallback.hpp
> >>> @@ -25,6 +25,7 @@ public:
> >>>unsigned Rank() override;
> >>>void ParseOptions(const ConfigureOption *options);
> >>>SpiceVideoCodecType VideoCodecType() const;
> >>> +static bool Register(Agent* agent);
> >>> private:
> >>>MjpegSettings settings = { 10, 80 };
> >>> };
> >>> diff --git a/src/spice-streaming-agent.cpp
> >>> b/src/spice-streaming-agent.cpp
> >>> index 94d9d25..8458975 100644
> >>> --- a/src/spice-streaming-agent.cpp
> >>> +++ b/src/spice-streaming-agent.cpp
> >>> @@ -34,6 +34,7 @@
> >>> 
> >>> #include "hexdump.h"
> >>> #include "concrete-agent.hpp"
> >>> +#include "mjpeg-fallback.hpp"
> >>> 
> >>> using namespace std;
> >>> using namespace SpiceStreamingAgent;
> >>> @@ -489,6 +490,9 @@ int main(int argc, char* argv[])
> >>>}
> >>>}
> >>> 
> >>> +// register built-in plugins
> >>> +MjpegPlugin::Register();
> >>> +
> >>>agent.LoadPlugins(PLUGINSDIR);
> >>> 
> >>>register_interrupts();
> >>> diff --git a/src/static-plugin.cpp b/src/static-plugin.cpp
> >>> deleted file mode 100644
> >>> index d5feb22..000
> >>> --- a/src/static-plugin.cpp
> >>> +++ /dev/null
> >>> @@ -1,23 +0,0 @@
> >>> -/* Utility to manage registration of plugins compiled statically
> >>> - *
> >>> - * \copyright
> >>> - * Copyright 2017 Red Hat Inc. All rights reserved.
> >>> - */
> >>> -
> >>> -#ifdef HAVE_CONFIG_H
> >>> -#include 
> >>> -#endif
> >>> -
> >>> -#include 
> >>> -#include "static-plugin.hpp"
> >>> -
> >>> -using namespace SpiceStreamingAgent;
> >>> -
> >>> -const StaticPlugin *StaticPlugin::list = 

[Spice-devel] [PATCH spice-gtk v2 3/4] uri: generate spice://host:port or spice+tls://host:port

[marcandre . lureau]

From: Marc-André Lureau 

Signed-off-by: Marc-André Lureau 
---
 src/spice-session.c | 31 +++
 tests/session.c | 20 ++--
 2 files changed, 33 insertions(+), 18 deletions(-)

diff --git a/src/spice-session.c b/src/spice-session.c
index d2aa5e7..6faf0f2 100644
--- a/src/spice-session.c
+++ b/src/spice-session.c
@@ -400,17 +400,32 @@ static gchar* spice_uri_create(SpiceSession *session)
 if (s->unix_path != NULL) {
 return g_strdup_printf(URI_SCHEME_SPICE_UNIX "%s", s->unix_path);
 } else if (s->host != NULL) {
+GString *str;
 g_return_val_if_fail(s->port != NULL || s->tls_port != NULL, NULL);
 
-GString *str = g_string_new(URI_SCHEME_SPICE);
+if (!!s->tls_port + !!s->port == 1) {
+/* use spice://foo:4390 or spice+tls://.. form */
+const char *port;
 
-g_string_append(str, s->host);
-g_string_append(str, "?");
-if (s->port != NULL) {
-g_string_append_printf(str, "port=%s&", s->port);
-}
-if (s->tls_port != NULL) {
-g_string_append_printf(str, "tls-port=%s", s->tls_port);
+if (s->tls_port) {
+str = g_string_new(URI_SCHEME_SPICE_TLS);
+port = s->tls_port;
+} else {
+str = g_string_new(URI_SCHEME_SPICE);
+port = s->port;
+}
+g_string_append_printf(str, "%s:%s", s->host, port);
+} else {
+/* use spice://foo?port=4390= form */
+str = g_string_new(URI_SCHEME_SPICE);
+g_string_append(str, s->host);
+g_string_append(str, "?");
+if (s->port != NULL) {
+g_string_append_printf(str, "port=%s&", s->port);
+}
+if (s->tls_port != NULL) {
+g_string_append_printf(str, "tls-port=%s", s->tls_port);
+}
 }
 return g_string_free(str, FALSE);
 }
diff --git a/tests/session.c b/tests/session.c
index 413d812..fc874fc 100644
--- a/tests/session.c
+++ b/tests/session.c
@@ -201,28 +201,28 @@ static void test_session_uri_ipv4_good(void)
   "localhost",
   NULL, NULL,
   "spice://localhost?port=5900=",
-  "spice://localhost?port=5900&" },
+  "spice://localhost:5900" },
 { "5910", NULL,
   "localhost",
   "user", NULL,
   "spice://user@localhost?tls-port==5910",
-  "spice://localhost?port=5910&" },
+  "spice://localhost:5910" },
 { NULL, "5920",
   "localhost",
   "user", "password",
   "spice://user@localhost?tls-port=5920==password",
-  "spice://localhost?tls-port=5920",
+  "spice+tls://localhost:5920",
   "password may be visible in process listings"},
 { NULL, "5930",
   "localhost",
   NULL, NULL,
   "spice://localhost?port==5930",
-  "spice://localhost?tls-port=5930" },
+  "spice+tls://localhost:5930" },
 { "42", NULL,
   "localhost",
   NULL, NULL,
   "spice://localhost:42",
-  "spice://localhost?port=42&" },
+  "spice://localhost:42" },
 { "42", "5930",
   "localhost",
   NULL, NULL,
@@ -246,28 +246,28 @@ static void test_session_uri_ipv6_good(void)
   "[2010:836B:4179::836B:4179]",
   NULL, NULL,
   "spice://[2010:836B:4179::836B:4179]?port=5900=",
-  "spice://[2010:836B:4179::836B:4179]?port=5900&" },
+  "spice://[2010:836B:4179::836B:4179]:5900" },
 { "5910", NULL,
   "[::192.9.5.5]",
   "user", NULL,
   "spice://user@[::192.9.5.5]?tls-port==5910",
-  "spice://[::192.9.5.5]?port=5910&" },
+  "spice://[::192.9.5.5]:5910" },
 { NULL, "5920",
   "[3ffe:2a00:100:7031::1]",
   "user", "password",
   
"spice://user@[3ffe:2a00:100:7031::1]?tls-port=5920==password",
-  "spice://[3ffe:2a00:100:7031::1]?tls-port=5920",
+  "spice+tls://[3ffe:2a00:100:7031::1]:5920",
   "password may be visible in process listings"},
 { NULL, "5930",
   "[1080:0:0:0:8:800:200C:417A]",
   NULL, NULL,
   "spice://[1080:0:0:0:8:800:200C:417A]?port==5930",
-  "spice://[1080:0:0:0:8:800:200C:417A]?tls-port=5930" },
+  "spice+tls://[1080:0:0:0:8:800:200C:417A]:5930" },
 { "42", NULL,
   "[FEDC:BA98:7654:3210:FEDC:BA98:7654:3210]",
   NULL, NULL,
   "spice://[FEDC:BA98:7654:3210:FEDC:BA98:7654:3210]:42",
-  "spice://[FEDC:BA98:7654:3210:FEDC:BA98:7654:3210]?port=42&" },
+  "spice://[FEDC:BA98:7654:3210:FEDC:BA98:7654:3210]:42" },
 { "42", "5930",
   "[::192.9.5.5]",
   NULL, NULL,
-- 
2.16.1.73.g5832b7e9f2

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

[Christophe de Dinechin]

> On 16 Feb 2018, at 10:12, Christophe Fergeau  wrote:
> 
> On Thu, Feb 15, 2018 at 05:06:17PM +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin 
>> 
>> Changes since v3:
>> - Integrate comments about performance impact and solution
>> - Integrate comments about impact of a failed assertion
>> - Add prohibition against side effects
>> 
>> Signed-off-by: Christophe de Dinechin 
>> ---
>> docs/spice_style.txt | 36 +---
>> 1 file changed, 33 insertions(+), 3 deletions(-)
>> 
>> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
>> index 5dc41cb1..3bc70570 100644
>> --- a/docs/spice_style.txt
>> +++ b/docs/spice_style.txt
>> @@ -113,10 +113,40 @@ 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, but pay attention to performance impact. 
>> Assertions checks have a significant performance impact should be placed 
>> under the `ENABLE_EXTRA_CHECKS` conditional.
> 
> [snip]
> 
>> +
>> +Be mindful of the impact of a failing assertion. For example, assertions in 
>> the server should not abort it, as this would kill the QEMU process and the 
>> virtual machine.
> 
> For me, "assertion" implies abortion of the process,

Which is why I point out the impact.

> so the first
> sentence ("use assertions liberally") and this one are confusing.
> In particular, g_assert() which you mention in the snipped part does
> abort.

So? I don’t understand the problem. How would you rephrase?

> 
> Christophe

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

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

[Daniel P . Berrangé]

On Fri, Feb 16, 2018 at 11:13:06AM +0100, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau 
> 
> spice:// has a weird scheme encoding, where it can accept both plain
> and tls ports with URI query parameters. However, it's not very
> convenient nor very common to use (who really want to mix plain & tls
> channels?).

Is it worth formally deprecating the mixing of plain & tls on a per
channel basis in QEMU ?  The idea that you can be secure, and yet
still have some channels plain text is really dubious and promotes
dangerous practice to users.

> 
> Instead, let's introduce the more readable form spice+tls://host:port
> This form will not accept 'port' or 'tls-port' query string parameter.
> 
> Signed-off-by: Marc-André Lureau 

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH spice-gtk v2 1/4] session: warn with the original URI

[marcandre . lureau]

From: Marc-André Lureau 

Before:
(lt-spicy:18372): GSpice-WARNING **: 10:57:21.246: Double set of 'port' in URI 
'spice://foo'

After:
(lt-spicy:18654): GSpice-WARNING **: 10:57:21.246: Double set of 'port' in URI 
'spice://foo:23?port=24'

Signed-off-by: Marc-André Lureau 
---
 src/spice-session.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/spice-session.c b/src/spice-session.c
index 2aabf58..e6db424 100644
--- a/src/spice-session.c
+++ b/src/spice-session.c
@@ -545,7 +545,7 @@ static int spice_parse_uri(SpiceSession *session, const 
char *original_uri)
 }
 if (target_key) {
 if (*target_key) {
-g_warning("Double set of '%s' in URI '%s'", key, uri);
+g_warning("Double set of '%s' in URI '%s'", key, original_uri);
 goto fail;
 }
 *target_key = g_uri_unescape_string(value, NULL);
@@ -553,7 +553,7 @@ static int spice_parse_uri(SpiceSession *session, const 
char *original_uri)
 }
 
 if (port == NULL && tls_port == NULL) {
-g_warning("Missing port or tls-port in spice URI '%s'", uri);
+g_warning("Missing port or tls-port in spice URI '%s'", original_uri);
 goto fail;
 }
 
-- 
2.16.1.73.g5832b7e9f2

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

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

[Christophe Fergeau]

On Fri, Feb 16, 2018 at 10:40:58AM +0100, Christophe de Dinechin wrote:
> 
> 
> > On 16 Feb 2018, at 10:12, Christophe Fergeau  wrote:
> > 
> > On Thu, Feb 15, 2018 at 05:06:17PM +0100, Christophe de Dinechin wrote:
> >> From: Christophe de Dinechin 
> >> 
> >> Changes since v3:
> >> - Integrate comments about performance impact and solution
> >> - Integrate comments about impact of a failed assertion
> >> - Add prohibition against side effects
> >> 
> >> Signed-off-by: Christophe de Dinechin 
> >> ---
> >> docs/spice_style.txt | 36 +---
> >> 1 file changed, 33 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> >> index 5dc41cb1..3bc70570 100644
> >> --- a/docs/spice_style.txt
> >> +++ b/docs/spice_style.txt
> >> @@ -113,10 +113,40 @@ 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, but pay attention to performance impact. 
> >> Assertions checks have a significant performance impact should be placed 
> >> under the `ENABLE_EXTRA_CHECKS` conditional.
> > 
> > [snip]
> > 
> >> +
> >> +Be mindful of the impact of a failing assertion. For example, assertions 
> >> in the server should not abort it, as this would kill the QEMU process and 
> >> the virtual machine.
> > 
> > For me, "assertion" implies abortion of the process,
> 
> Which is why I point out the impact.
> 
> > so the first
> > sentence ("use assertions liberally") and this one are confusing.
> > In particular, g_assert() which you mention in the snipped part does
> > abort.
> 
> So? I don’t understand the problem. How would you rephrase?

I read the beginning as saying "use g_assert() whenever you want", and
the end saying "be very careful with g_assert() because it kills the
program".
In general, I would rework this section to be about assertions and
preconditions.
preconditions in the form of g_return_if_*, g_warn_if_*, ... can be used
liberally, should be used on most public entry points (maybe even on
non-static functions), but only to catch a situation where we have a
programming error, ie something which the programmer thinks should not
happen the way the code is written.
assertions in the form of g_assert (we want to deprecate spice helpers
which have glib equivalent) should be used very rarely, ideally never.

Christophe


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

Re: [Spice-devel] [PATCH spice-gtk v2 1/4] session: warn with the original URI

[Frediano Ziglio]

> 
> From: Marc-André Lureau 
> 
> Before:
> (lt-spicy:18372): GSpice-WARNING **: 10:57:21.246: Double set of 'port' in
> URI 'spice://foo'
> 
> After:
> (lt-spicy:18654): GSpice-WARNING **: 10:57:21.246: Double set of 'port' in
> URI 'spice://foo:23?port=24'
> 
> Signed-off-by: Marc-André Lureau 
> ---
>  src/spice-session.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/spice-session.c b/src/spice-session.c
> index 2aabf58..e6db424 100644
> --- a/src/spice-session.c
> +++ b/src/spice-session.c
> @@ -545,7 +545,7 @@ static int spice_parse_uri(SpiceSession *session, const
> char *original_uri)
>  }
>  if (target_key) {
>  if (*target_key) {
> -g_warning("Double set of '%s' in URI '%s'", key, uri);
> +g_warning("Double set of '%s' in URI '%s'", key,
> original_uri);
>  goto fail;
>  }
>  *target_key = g_uri_unescape_string(value, NULL);
> @@ -553,7 +553,7 @@ static int spice_parse_uri(SpiceSession *session, const
> char *original_uri)
>  }
>  
>  if (port == NULL && tls_port == NULL) {
> -g_warning("Missing port or tls-port in spice URI '%s'", uri);
> +g_warning("Missing port or tls-port in spice URI '%s'",
> original_uri);
>  goto fail;
>  }
>  

Acked-by: Frediano Ziglio 

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

[Spice-devel] [PATCH spice-streaming-agent] build: Execute tests while packaging RPM file

[Frediano Ziglio]

Signed-off-by: Frediano Ziglio 
---
 spice-streaming-agent.spec.in | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/spice-streaming-agent.spec.in b/spice-streaming-agent.spec.in
index 99b1f52..d137840 100644
--- a/spice-streaming-agent.spec.in
+++ b/spice-streaming-agent.spec.in
@@ -33,6 +33,9 @@ agent plugins.
 %configure
 make %{?_smp_mflags} V=1
 
+%check
+make check
+
 %install
 make install DESTDIR=%{buildroot} V=1
 if test -d "%{buildroot}/%{_libdir}/%{name}/plugins"; then
-- 
2.14.3

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

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

[Christophe de Dinechin]

> On 16 Feb 2018, at 10:05, Christophe Fergeau  wrote:
> 
> On Thu, Feb 15, 2018 at 05:06:19PM +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin 
>> 
>> Signed-off-by: Christophe de Dinechin 
>> ---
>> docs/spice_style.txt | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
>> index 996c5cd9..9e6536e4 100644
>> --- a/docs/spice_style.txt
>> +++ b/docs/spice_style.txt
>> @@ -178,7 +178,7 @@ If multiple related constants are to be defined, 
>> consider the use of enumeration
>> Short functions
>> ---
>> 
>> -Try to split code to short functions, each having simple functionality, in 
>> order to improve code readability and re-usability. Prefix with inline short 
>> functions or functions that were splitted for readability reason.
>> +Try to split code to short functions, each having simple functionality, in 
>> order to improve code readability and re-usability. Prefix with `inline` 
>> functions that were splitted for readability reason or that are very short.
> 
> I'd just drop the second sentence. The compiler usually knows better.

Agreed, done.

> 
> Christophe

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

[Spice-devel] [PATCH spice-gtk v2 4/4] tests: add spice+tls:// tests

[marcandre . lureau]

From: Marc-André Lureau 

They couldn't not be introduced before, because the test needs both
parsing and generation.

Signed-off-by: Marc-André Lureau 
---
 tests/session.c | 24 +++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/tests/session.c b/tests/session.c
index fc874fc..f0ecda2 100644
--- a/tests/session.c
+++ b/tests/session.c
@@ -21,7 +21,7 @@ static void test_session_uri_bad(void)
 struct {
 const GLogLevelFlags log_level;
 const gchar *message;
-} messages[2];
+} messages[4];
 } uris[] = {
 {
 "scheme://host?port",
@@ -111,6 +111,25 @@ static void test_session_uri_bad(void)
 "*assertion 's->port != NULL || s->tls_port != NULL' 
failed",
 },
 }
+},{
+"spice+tls://hostname?tls-port=1234=3456",
+{
+{
+G_LOG_LEVEL_WARNING,
+"spice+tls:// scheme doesn't accept 'tls-port'",
+},
+{
+G_LOG_LEVEL_WARNING,
+"spice+tls:// scheme doesn't accept 'port'",
+},
+{
+G_LOG_LEVEL_WARNING,
+"Missing port or tls-port in spice URI *",
+},{
+G_LOG_LEVEL_CRITICAL,
+"*assertion 's->port != NULL || s->tls_port != NULL' 
failed",
+},
+}
 },
 };
 
@@ -233,6 +252,9 @@ static void test_session_uri_ipv4_good(void)
   NULL, NULL,
   "spice://127.0.0.1:42?tls-port=5930",
   "spice://127.0.0.1?port=42=5930" },
+{ .uri_input  = "spice+tls://hostname:39",
+  .host = "hostname",
+  .tls_port = "39" }
 };
 
 test_session_uri_good(tests, G_N_ELEMENTS(tests));
-- 
2.16.1.73.g5832b7e9f2

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

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

[Frediano Ziglio]

> 
> From: Christophe de Dinechin 
> 
> Without this, GCC complains about signed / unsigned comparisons:
> 
> mjpeg-fallback.cpp:121:24: warning: comparison between signed and unsigned
> integer expressions [-Wsign-compare]
>  if (win_info.width != last_width || win_info.height != last_height) {
>  ~~~^
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  src/mjpeg-fallback.cpp| 2 +-
>  src/spice-streaming-agent.cpp | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> index 634864f..53804d9 100644
> --- a/src/mjpeg-fallback.cpp
> +++ b/src/mjpeg-fallback.cpp
> @@ -57,7 +57,7 @@ private:
>  std::vector frame;
>  
>  // last frame sizes
> -uint32_t last_width = ~0u, last_height = ~0u;
> +int last_width = ~0u, last_height = ~0u;
>  // last time before capture
>  uint64_t last_time = 0;
>  };

That is weird, so the compiler here is not giving any warning
doing an implicit unsigned -> signed conversion which should
be explicit!
Not that the ranges here is a problem.
On the other way a window with a negative width is very hard to
picture, we are basically hiding what the compiler see as a possible
issue, is not much different that just disabling the warning at
the end.

> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 4ec5e42..27b26a4 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -106,7 +106,7 @@ static int read_command_from_device(void)
>  return 0; // return -1;
>  }
>  n = read(streamfd, , hdr.size);
> -if (n != hdr.size) {
> +if (n != (int) hdr.size) {
>  syslog(LOG_WARNING,
> "read command from device FAILED -- read %d expected %d\n",
> n, hdr.size);

Acked

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

Re: [Spice-devel] [PATCH 04/17] Do not create std::string for constants

[Frediano Ziglio]

> 
> From: Christophe de Dinechin 
> 
> As per e-mail discussions (https://patchwork.freedesktop.org/patch/200629/),
> using std::string for C-style string constants is against C++ good practices,
> see
> https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rstr-zstring
> for reference.
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  src/spice-streaming-agent.cpp | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index b5c7e24..646d15b 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -315,12 +315,13 @@ static void cursor_changes(Display *display, int
> event_base)
>  }
>  
>  static void
> -do_capture(const std::string , FILE *f_log)
> +do_capture(const char *streamport, FILE *f_log)
>  {
> -streamfd = open(streamport.c_str(), O_RDWR);
> +streamfd = open(streamport, O_RDWR);
>  if (streamfd < 0)
>  throw std::runtime_error("failed to open the streaming device (" +
> - streamport + "): " + strerror(errno));
> + std::string(streamport) + "): "
> + + strerror(errno));
>  
>  unsigned int frame_count = 0;
>  while (!quit_requested) {
> @@ -404,7 +405,7 @@ done:
>  
>  int main(int argc, char* argv[])
>  {
> -std::string streamport = "/dev/virtio-ports/com.redhat.stream.0";
> +const char *streamport = "/dev/virtio-ports/com.redhat.stream.0";
>  char opt;
>  const char *log_filename = NULL;
>  int logmask = LOG_UPTO(LOG_WARNING);

Acked

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

Re: [Spice-devel] [PATCH 1/2] Add missing header

[Frediano Ziglio]

> 
> From: Christophe de Dinechin 
> 
> Without this header, clang complains with messages such as:
> 
> concrete-agent.cpp:62:37: error: invalid operands to binary expression
> ('const std::string' (aka 'const basic_string allocator >') and 'const char *')
> std::string pattern = directory + "/*.so";
>   ~ ^ ~~~
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/iterator:743:1:
> note: candidate template ignored: could not match
> 'reverse_iterator' against 'char const[6]'
> operator+(typename reverse_iterator<_Iter>::difference_type __n, const
> reverse_iterator<_Iter>& __x)
> ^
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  src/concrete-agent.cpp | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
> index 891c09b..ac37788 100644
> --- a/src/concrete-agent.cpp
> +++ b/src/concrete-agent.cpp
> @@ -9,6 +9,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "concrete-agent.hpp"
>  #include "static-plugin.hpp"

Acked.

I suppose on clang the std::string is defined in another header.
Is weird that got basic_string and std::string declarations but not
its operators.

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

Re: [Spice-devel] [PATCH 05/17] Use RAII to cleanup stream in case of exception or return

[Frediano Ziglio]

> 
> From: Christophe de Dinechin 
> 
> Get rid of C-style 'goto done' in do_capture.
> Get rid of global streamfd, pass it around (cleaned up in later patch)
> Fixes a race condition, make sure we only use stream after opening
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  src/spice-streaming-agent.cpp | 79
>  +--
>  1 file changed, 47 insertions(+), 32 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 646d15b..9b8fd45 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -53,14 +53,38 @@ struct SpiceStreamDataMessage
>  StreamMsgData msg;
>  };
>  
> +struct SpiceStreamCursorMessage
> +{
> +StreamDevHeader hdr;
> +StreamMsgCursorSet msg;
> +};
> +

This is weird in this patch

> +class Stream
> +{
> +public:
> +Stream(const char *name)
> +{
> +fd = open(name, O_RDWR);
> +if (fd < 0)
> +throw std::runtime_error("failed to open streaming device");
> +}
> +~Stream()
> +{
> +close(fd);

You probably what to check for -1 to avoid calling close(-1)

> +}
> +operator int() { return fd; }

Sure you want this? I think is safer to avoid implicit cast
also considering stuff like

  Stream s;
  if (s) ...

> +
> +private:
> +int fd = -1;

So with a default constructor you want a class with
an invalid state?
Or just disable default constructor.
I would disable also copy.

> +};
> +
>  static bool streaming_requested = false;
>  static bool quit_requested = false;
>  static bool log_binary = false;
>  static std::set client_codecs;
> -static int streamfd = -1;
>  static std::mutex stream_mtx;
>  
> -static int have_something_to_read(int timeout)
> +static int have_something_to_read(int streamfd, int timeout)

maybe would be better to use the "fd" name here, is no more
bound to stream.

>  {
>  struct pollfd pollfd = {streamfd, POLLIN, 0};
>  
> @@ -76,7 +100,7 @@ static int have_something_to_read(int timeout)
>  return 0;
>  }
>  
> -static int read_command_from_device(void)
> +static int read_command_from_device(int streamfd)

Have you considered Stream::read_command?
Ok, mostly in a following patch

>  {
>  StreamDevHeader hdr;
>  uint8_t msg[64];
> @@ -121,18 +145,18 @@ static int read_command_from_device(void)
>  return 1;
>  }
>  
> -static int read_command(bool blocking)
> +static int read_command(int streamfd, bool blocking)
>  {
>  int timeout = blocking?-1:0;
>  while (!quit_requested) {
> -if (!have_something_to_read(timeout)) {
> +if (!have_something_to_read(streamfd, timeout)) {
>  if (!blocking) {
>  return 0;
>  }
>  sleep(1);
>  continue;
>  }
> -return read_command_from_device();
> +return read_command_from_device(streamfd);
>  }
>  
>  return 1;
> @@ -157,7 +181,7 @@ write_all(int fd, const void *buf, const size_t len)
>  return written;
>  }
>  
> -static int spice_stream_send_format(unsigned w, unsigned h, unsigned c)
> +static int spice_stream_send_format(int streamfd, unsigned w, unsigned h,
> unsigned c)
>  {
>  
>  SpiceStreamFormatMessage msg;
> @@ -178,7 +202,7 @@ static int spice_stream_send_format(unsigned w, unsigned
> h, unsigned c)
>  return EXIT_SUCCESS;
>  }
>  
> -static int spice_stream_send_frame(const void *buf, const unsigned size)
> +static int spice_stream_send_frame(int streamfd, const void *buf, const
> unsigned size)
>  {
>  SpiceStreamDataMessage msg;
>  const size_t msgsize = sizeof(msg);
> @@ -254,7 +278,7 @@ static void usage(const char *progname)
>  }
>  
>  static void
> -send_cursor(unsigned width, unsigned height, int hotspot_x, int hotspot_y,
> +send_cursor(int streamfd, unsigned width, unsigned height, int hotspot_x,
> int hotspot_y,
>  std::function fill_cursor)
>  {
>  if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
> @@ -288,7 +312,7 @@ send_cursor(unsigned width, unsigned height, int
> hotspot_x, int hotspot_y,
>  write_all(streamfd, msg.get(), cursor_size);
>  }
>  
> -static void cursor_changes(Display *display, int event_base)
> +static void cursor_changes(int streamfd, Display *display, int event_base)
>  {
>  unsigned long last_serial = 0;
>  
> @@ -310,25 +334,20 @@ static void cursor_changes(Display *display, int
> event_base)
>  for (unsigned i = 0; i < cursor->width * cursor->height; ++i)
>  pixels[i] = cursor->pixels[i];
>  };
> -send_cursor(cursor->width, cursor->height, cursor->xhot,
> cursor->yhot, fill_cursor);
> +send_cursor(streamfd,
> +cursor->width, cursor->height, cursor->xhot,
> cursor->yhot, fill_cursor);
>  }
>  }
>  
>  static void
> -do_capture(const char *streamport, FILE *f_log)
> +do_capture(int streamfd, const 

Re: [Spice-devel] [PATCH 02/17] log_binary is really a boolean

[Frediano Ziglio]

> 
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  src/spice-streaming-agent.cpp | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 4ec5e42..6056129 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -55,9 +55,9 @@ struct SpiceStreamDataMessage
>  
>  static bool streaming_requested = false;
>  static bool quit_requested = false;
> +static bool log_binary = false;
>  static std::set client_codecs;
>  static int streamfd = -1;
> -static int log_binary = 0;
>  static std::mutex stream_mtx;
>  
>  static int have_something_to_read(int timeout)
> @@ -409,7 +409,7 @@ int main(int argc, char* argv[])
>  const char *log_filename = NULL;
>  int logmask = LOG_UPTO(LOG_WARNING);
>  struct option long_options[] = {

This can be static const now.

> -{ "log-binary", no_argument, _binary, 1},
> +{ "log-binary", no_argument, NULL, 'b'},
>  { "help", no_argument, NULL, 'h'},
>  { 0, 0, 0, 0}
>  };

I don't think this option deserve a short option.
Also the usage should be changed.

OT: "char opt" is a bug, not that of a problem.
The only bug I can see is somebody passing -$'\xff'
interpreted as a end of arguments.

> @@ -437,6 +437,9 @@ int main(int argc, char* argv[])
>  agent.AddOption(optarg, p);
>  break;
>  }
> +case 'b':
> +log_binary = true;
> +break;
>  case 'l':
>  log_filename = optarg;
>  break;

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

Re: [Spice-devel] [PATCH 05/17] Use RAII to cleanup stream in case of exception or return

[Christophe de Dinechin]

> On 16 Feb 2018, at 17:40, Frediano Ziglio  wrote:
> 
>> 
>> From: Christophe de Dinechin 
>> 
>> Get rid of C-style 'goto done' in do_capture.
>> Get rid of global streamfd, pass it around (cleaned up in later patch)
>> Fixes a race condition, make sure we only use stream after opening
>> 
>> Signed-off-by: Christophe de Dinechin 
>> ---
>> src/spice-streaming-agent.cpp | 79
>> +--
>> 1 file changed, 47 insertions(+), 32 deletions(-)
>> 
>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>> index 646d15b..9b8fd45 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -53,14 +53,38 @@ struct SpiceStreamDataMessage
>> StreamMsgData msg;
>> };
>> 
>> +struct SpiceStreamCursorMessage
>> +{
>> +StreamDevHeader hdr;
>> +StreamMsgCursorSet msg;
>> +};
>> +
> 
> This is weird in this patch

Yes. I may have merged two patches. Will check.

> 
>> +class Stream
>> +{
>> +public:
>> +Stream(const char *name)
>> +{
>> +fd = open(name, O_RDWR);
>> +if (fd < 0)
>> +throw std::runtime_error("failed to open streaming device");
>> +}
>> +~Stream()
>> +{
>> +close(fd);
> 
> You probably what to check for -1 to avoid calling close(-1)

I think this cannot happen, since in that case you’d have thrown from the ctor.

> 
>> +}
>> +operator int() { return fd; }
> 
> Sure you want this? I think is safer to avoid implicit cast
> also considering stuff like
> 
>  Stream s;
>  if (s) …

It is removed in a later patch once it’s no longer needed. The objective here 
is to minimize noisy changes at each step.

> 
>> +
>> +private:
>> +int fd = -1;
> 
> So with a default constructor you want a class with
> an invalid state?
> Or just disable default constructor.

It’s disabled, there’s a ctor with args.

> I would disable also copy.

Also implicitly disabled.

> 
>> +};
>> +
>> static bool streaming_requested = false;
>> static bool quit_requested = false;
>> static bool log_binary = false;
>> static std::set client_codecs;
>> -static int streamfd = -1;
>> static std::mutex stream_mtx;
>> 
>> -static int have_something_to_read(int timeout)
>> +static int have_something_to_read(int streamfd, int timeout)
> 
> maybe would be better to use the "fd" name here, is no more
> bound to stream.

Kept to minimize changes

>> {
>> struct pollfd pollfd = {streamfd, POLLIN, 0};
>> 
>> @@ -76,7 +100,7 @@ static int have_something_to_read(int timeout)
>> return 0;
>> }
>> 
>> -static int read_command_from_device(void)
>> +static int read_command_from_device(int streamfd)
> 
> Have you considered Stream::read_command?

For that specific case, I liked the “from_device”. But…

> Ok, mostly in a following patch


> 
>> {
>> StreamDevHeader hdr;
>> uint8_t msg[64];
>> @@ -121,18 +145,18 @@ static int read_command_from_device(void)
>> return 1;
>> }
>> 
>> -static int read_command(bool blocking)
>> +static int read_command(int streamfd, bool blocking)
>> {
>> int timeout = blocking?-1:0;
>> while (!quit_requested) {
>> -if (!have_something_to_read(timeout)) {
>> +if (!have_something_to_read(streamfd, timeout)) {
>> if (!blocking) {
>> return 0;
>> }
>> sleep(1);
>> continue;
>> }
>> -return read_command_from_device();
>> +return read_command_from_device(streamfd);
>> }
>> 
>> return 1;
>> @@ -157,7 +181,7 @@ write_all(int fd, const void *buf, const size_t len)
>> return written;
>> }
>> 
>> -static int spice_stream_send_format(unsigned w, unsigned h, unsigned c)
>> +static int spice_stream_send_format(int streamfd, unsigned w, unsigned h,
>> unsigned c)
>> {
>> 
>> SpiceStreamFormatMessage msg;
>> @@ -178,7 +202,7 @@ static int spice_stream_send_format(unsigned w, unsigned
>> h, unsigned c)
>> return EXIT_SUCCESS;
>> }
>> 
>> -static int spice_stream_send_frame(const void *buf, const unsigned size)
>> +static int spice_stream_send_frame(int streamfd, const void *buf, const
>> unsigned size)
>> {
>> SpiceStreamDataMessage msg;
>> const size_t msgsize = sizeof(msg);
>> @@ -254,7 +278,7 @@ static void usage(const char *progname)
>> }
>> 
>> static void
>> -send_cursor(unsigned width, unsigned height, int hotspot_x, int hotspot_y,
>> +send_cursor(int streamfd, unsigned width, unsigned height, int hotspot_x,
>> int hotspot_y,
>> std::function fill_cursor)
>> {
>> if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
>> @@ -288,7 +312,7 @@ send_cursor(unsigned width, unsigned height, int
>> hotspot_x, int hotspot_y,
>> write_all(streamfd, msg.get(), cursor_size);
>> }
>> 
>> -static void cursor_changes(Display *display, int event_base)
>> +static void cursor_changes(int streamfd, Display *display, int event_base)
>> {
>> 

[Spice-devel] [PATCH 00/17] WIP: Refactor the streaming agent towards a more standard C++ style

[Christophe de Dinechin]

From: Christophe de Dinechin 

The streaming agent started as C code. This series converts
the style to something that is more usual for C++, notably:

- Adds encapsulation and RAII for resources such as the stream
- Splits functionality into several classes with clear roles
- Puts message formatting and writing in a reused based class
- Isolate what's specific to each message in three derived classes
- Isolate X11-specific code in separate classes, one for cursor messages,
  one for the thread polling the data.

Reasons for marking this WIP:

1. This series is, unfortunately, not correctly tested, because on my
   machine, I currently have a black screen with the 'master' streaming
   agent, server, protocol, common, plugin and spicy. Fallback to MJPEG
   leads to a large repetition of messages like:

   (spicy:4217): GSpice-CRITICAL **: need more input data

   What I have tested using the -d option is that the syslog output
   from the agent looks similar (size of data captured, etc) relative
   to master both for the MJPEG plugin and with fallback. But without
   a picture, I am still concerned about the lack of testing.

2. The classes were isolated, but not moved in separate headers. This
   is intentional. I prefer to make sure that the changes on the code
   are agreed on before we move the individual classes to their own
   headers. It thinks it will also faclitate history browsing

3. This series compiles without warnings both with GCC in C++11 mode
   and by clang in gnu++11 mode. However, Frediano pointed out that it
   uses a designated intiializer syntax that is presently a GNU
   extension (the C99 designated initializer syntax). We may consider
   it a problem or not. If it's a problem, it's sufficient to remove
   the designators, but I think they add to readability.

4. Our current configure.ac requires a warning if all initializers are
   not present. Since padding was made explicit in the protocol, this
   requires the code to initialize padding fields, which I don't like.

5. The series integrates off-topic patches sent in a separate series, but
   which are necessary to successfully build with both clang and gcc.

This series can also be browsed at
https://gitlab.com/c3d/spice-streaming-agent/merge_requests/1/commits

Christophe de Dinechin (17):
  Add missing  header
  log_binary is really a boolean
  Eliminate signed/unsigned warning
  Do not create std::string for constants
  Use RAII to cleanup stream in case of exception or return
  Replace inefficient C-style initialization with C++-style
  Get rid of C-style memset initializations, use C++ style aggregates
  Use C++ style for cursor message initialization instead of C style
  Reorder headers according to style guide
  Since we use a namespace, simplify name of local classes
  Move read, write and locking into the 'Stream' class
  Convert message writing from C style to C++ style
  Add more meaningful syslog reporting
  Create a class encapsulating the X11 display cursor capture
  Create FrameLog class to abstract logging of frames
  Remove client_codecs global variable, moved inside the 'Stream' class
  Move the capture loop in the ConcreteAgent, get rid of global agent
variable

 src/concrete-agent.cpp|   1 +
 src/concrete-agent.hpp|   4 +
 src/mjpeg-fallback.cpp|   2 +-
 src/spice-streaming-agent.cpp | 521 +-
 4 files changed, 311 insertions(+), 217 deletions(-)

-- 
2.13.5 (Apple Git-94)

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

[Spice-devel] [PATCH 02/17] log_binary is really a boolean

[Christophe de Dinechin]

From: Christophe de Dinechin 

Signed-off-by: Christophe de Dinechin 
---
 src/spice-streaming-agent.cpp | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 4ec5e42..6056129 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -55,9 +55,9 @@ struct SpiceStreamDataMessage
 
 static bool streaming_requested = false;
 static bool quit_requested = false;
+static bool log_binary = false;
 static std::set client_codecs;
 static int streamfd = -1;
-static int log_binary = 0;
 static std::mutex stream_mtx;
 
 static int have_something_to_read(int timeout)
@@ -409,7 +409,7 @@ int main(int argc, char* argv[])
 const char *log_filename = NULL;
 int logmask = LOG_UPTO(LOG_WARNING);
 struct option long_options[] = {
-{ "log-binary", no_argument, _binary, 1},
+{ "log-binary", no_argument, NULL, 'b'},
 { "help", no_argument, NULL, 'h'},
 { 0, 0, 0, 0}
 };
@@ -437,6 +437,9 @@ int main(int argc, char* argv[])
 agent.AddOption(optarg, p);
 break;
 }
+case 'b':
+log_binary = true;
+break;
 case 'l':
 log_filename = optarg;
 break;
-- 
2.13.5 (Apple Git-94)

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

[Spice-devel] [PATCH 09/17] Reorder headers according to style guide

[Christophe de Dinechin]

From: Christophe de Dinechin 

Signed-off-by: Christophe de Dinechin 
---
 src/spice-streaming-agent.cpp | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 720173a..f845bd0 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -4,6 +4,9 @@
  * Copyright 2016-2017 Red Hat Inc. All rights reserved.
  */
 
+#include "concrete-agent.hpp"
+#include "hexdump.h"
+
 #include 
 #include 
 #include 
@@ -34,8 +37,7 @@
 #include 
 #include 
 
-#include "hexdump.h"
-#include "concrete-agent.hpp"
+
 
 using namespace spice::streaming_agent;
 
-- 
2.13.5 (Apple Git-94)

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

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

[Christophe de Dinechin]

From: Christophe de Dinechin 

- The Stream class now deals with locking and sending messages
- The Message<> template class deals with the general writing mechanisms
- Three classes, FormatMessage, FrameMessage and X11CursorMessage represent 
individual messages

The various classes should be moved to separate headers in a subsequent 
operation

The design uses templates to avoid any runtime overhead. All the calls are 
static.

Signed-off-by: Christophe de Dinechin 
---
 src/spice-streaming-agent.cpp | 250 --
 1 file changed, 117 insertions(+), 133 deletions(-)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index a989ee7..c174ea4 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -48,24 +48,6 @@ namespace spice
 namespace streaming_agent
 {
 
-struct FormatMessage
-{
-StreamDevHeader hdr;
-StreamMsgFormat msg;
-};
-
-struct DataMessage
-{
-StreamDevHeader hdr;
-StreamMsgData msg;
-};
-
-struct CursorMessage
-{
-StreamDevHeader hdr;
-StreamMsgCursorSet msg;
-};
-
 class Stream
 {
 public:
@@ -79,24 +61,131 @@ public:
 {
 close(streamfd);
 }
-operator int() { return streamfd; }
 
 int have_something_to_read(int timeout);
 int read_command_from_device(void);
 int read_command(bool blocking);
 
+
+template 
+bool send(PayloadArgs... payload)
+{
+Message message(payload...);
+std::lock_guard stream_guard(mutex);
+size_t expected = message.size(payload...);
+size_t written = message.write(*this, payload...);
+return written == expected;
+}
+
 size_t write_all(const void *buf, const size_t len);
-int send_format(unsigned w, unsigned h, uint8_t c);
-int send_frame(const void *buf, const unsigned size);
-void send_cursor(uint16_t width, uint16_t height,
- uint16_t hotspot_x, uint16_t hotspot_y,
- std::function fill_cursor);
 
 private:
 int streamfd = -1;
 std::mutex mutex;
 };
 
+
+template 
+struct Message
+{
+template 
+Message(PayloadArgs... payload)
+: hdr(StreamDevHeader {
+  .protocol_version = STREAM_DEVICE_PROTOCOL,
+  .padding = 0, // Workaround GCC bug "sorry: not implemented"
+  .type = Info::type,
+  .size = (uint32_t) (Info::size(payload...) - sizeof(hdr))
+  }),
+  msg(Info::make(payload...))
+{ }
+
+StreamDevHeader hdr;
+Payload msg;
+};
+
+
+struct FormatMessage : Message
+{
+FormatMessage(unsigned w, unsigned h, uint8_t c) : Message(w, h, c) {}
+static const StreamMsgType type = STREAM_TYPE_FORMAT;
+static size_t size(unsigned width, unsigned height, uint8_t codec)
+{
+return sizeof(FormatMessage);
+}
+static StreamMsgFormat make(unsigned w, unsigned h, uint8_t c)
+{
+return StreamMsgFormat{ .width = w, .height = h, .codec = c, .padding1 
= {} };
+}
+size_t write(Stream , unsigned w, unsigned h, uint8_t c)
+{
+return stream.write_all(this, sizeof(*this));
+}
+};
+
+
+struct FrameMessage : Message
+{
+FrameMessage(const void *frame, size_t length) : Message(frame, length) {}
+static const StreamMsgType type = STREAM_TYPE_DATA;
+static size_t size(const void *frame, size_t length)
+{
+return sizeof(FrameMessage) + length;
+}
+static StreamMsgData make(const void *frame, size_t length)
+{
+return StreamMsgData();
+}
+size_t write(Stream , const void *frame, size_t length)
+{
+return stream.write_all(, sizeof(hdr)) + stream.write_all(frame, 
length);
+}
+};
+
+struct X11CursorMessage : Message
+{
+X11CursorMessage(XFixesCursorImage *cursor)
+: Message(cursor),
+  pixels(new uint32_t[pixel_size(cursor)])
+{
+fill_pixels(cursor);
+}
+static const StreamMsgType type = STREAM_TYPE_CURSOR_SET;
+static size_t pixel_size(XFixesCursorImage *cursor)
+{
+return cursor->width * cursor->height;
+}
+static size_t size(XFixesCursorImage *cursor)
+{
+return sizeof(X11CursorMessage) + sizeof(uint32_t) * 
pixel_size(cursor);
+}
+static StreamMsgCursorSet make(XFixesCursorImage *cursor)
+{
+return StreamMsgCursorSet
+{
+.width = cursor->width,
+.height = cursor->height,
+.hot_spot_x = cursor->xhot,
+.hot_spot_y = cursor->yhot,
+.type = SPICE_CURSOR_TYPE_ALPHA,
+.padding1 = { },
+.data = { }
+};
+}
+size_t write(Stream , XFixesCursorImage *cursor)
+{
+return stream.write_all(, sizeof(hdr)) + 
stream.write_all(pixels.get(), hdr.size);
+}
+  

[Spice-devel] [PATCH 04/17] Do not create std::string for constants

[Christophe de Dinechin]

From: Christophe de Dinechin 

As per e-mail discussions (https://patchwork.freedesktop.org/patch/200629/),
using std::string for C-style string constants is against C++ good practices,
see 
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rstr-zstring
 for reference.

Signed-off-by: Christophe de Dinechin 
---
 src/spice-streaming-agent.cpp | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index b5c7e24..646d15b 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -315,12 +315,13 @@ static void cursor_changes(Display *display, int 
event_base)
 }
 
 static void
-do_capture(const std::string , FILE *f_log)
+do_capture(const char *streamport, FILE *f_log)
 {
-streamfd = open(streamport.c_str(), O_RDWR);
+streamfd = open(streamport, O_RDWR);
 if (streamfd < 0)
 throw std::runtime_error("failed to open the streaming device (" +
- streamport + "): " + strerror(errno));
+ std::string(streamport) + "): "
+ + strerror(errno));
 
 unsigned int frame_count = 0;
 while (!quit_requested) {
@@ -404,7 +405,7 @@ done:
 
 int main(int argc, char* argv[])
 {
-std::string streamport = "/dev/virtio-ports/com.redhat.stream.0";
+const char *streamport = "/dev/virtio-ports/com.redhat.stream.0";
 char opt;
 const char *log_filename = NULL;
 int logmask = LOG_UPTO(LOG_WARNING);
-- 
2.13.5 (Apple Git-94)

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

[Spice-devel] [PATCH 08/17] Use C++ style for cursor message initialization instead of C style

[Christophe de Dinechin]

From: Christophe de Dinechin 

Signed-off-by: Christophe de Dinechin 
---
 src/spice-streaming-agent.cpp | 47 +--
 1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 1e19e43..720173a 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -285,38 +285,45 @@ static void usage(const char *progname)
 }
 
 static void
-send_cursor(int streamfd, unsigned width, unsigned height, int hotspot_x, int 
hotspot_y,
+send_cursor(int streamfd,
+uint16_t width, uint16_t height,
+uint16_t hotspot_x, uint16_t hotspot_y,
 std::function fill_cursor)
 {
 if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
 height >= STREAM_MSG_CURSOR_SET_MAX_HEIGHT)
 return;
 
-size_t cursor_size =
-sizeof(StreamDevHeader) + sizeof(StreamMsgCursorSet) +
-width * height * sizeof(uint32_t);
-std::unique_ptr msg(new uint8_t[cursor_size]);
+const uint32_t cursor_msgsize =
+sizeof(SpiceStreamCursorMessage) + width * height * sizeof(uint32_t);
+const uint32_t hdrsize  = sizeof(StreamDevHeader);
 
-StreamDevHeader _hdr(*reinterpret_cast(msg.get()));
-memset(_hdr, 0, sizeof(dev_hdr));
-dev_hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
-dev_hdr.type = STREAM_TYPE_CURSOR_SET;
-dev_hdr.size = cursor_size - sizeof(StreamDevHeader);
+std::unique_ptr storage(new uint8_t[cursor_msgsize]);
 
-StreamMsgCursorSet _msg(*reinterpret_cast(msg.get() + sizeof(StreamDevHeader)));
-memset(_msg, 0, sizeof(cursor_msg));
-
-cursor_msg.type = SPICE_CURSOR_TYPE_ALPHA;
-cursor_msg.width = width;
-cursor_msg.height = height;
-cursor_msg.hot_spot_x = hotspot_x;
-cursor_msg.hot_spot_y = hotspot_y;
+SpiceStreamCursorMessage *cursor_msg =
+new(storage.get()) SpiceStreamCursorMessage {
+.hdr = {
+.protocol_version = STREAM_DEVICE_PROTOCOL,
+.padding = 0,   // Workaround GCC internal / not implemented 
compiler error
+.type = STREAM_TYPE_CURSOR_SET,
+.size = cursor_msgsize - hdrsize
+},
+.msg = {
+.width = width,
+.height = height,
+.hot_spot_x = hotspot_x,
+.hot_spot_y = hotspot_y,
+.type = SPICE_CURSOR_TYPE_ALPHA,
+.padding1 = { },
+.data = { }
+}
+};
 
-uint32_t *pixels = reinterpret_cast(cursor_msg.data);
+uint32_t *pixels = reinterpret_cast(cursor_msg->msg.data);
 fill_cursor(pixels);
 
 std::lock_guard stream_guard(stream_mtx);
-write_all(streamfd, msg.get(), cursor_size);
+write_all(streamfd, storage.get(), cursor_msgsize);
 }
 
 static void cursor_changes(int streamfd, Display *display, int event_base)
-- 
2.13.5 (Apple Git-94)

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

[Spice-devel] [PATCH 16/17] Remove client_codecs global variable, moved inside the 'Stream' class

[Christophe de Dinechin]

From: Christophe de Dinechin 

This is not a really nice abstraction at this point, but still a step in the 
right way

Signed-off-by: Christophe de Dinechin 
---
 src/spice-streaming-agent.cpp | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index b0c09d8..19f3c07 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -61,8 +61,11 @@ static uint64_t get_time(void)
 
 class Stream
 {
+typedef std::set codecs_t;
+
 public:
 Stream(const char *name)
+: codecs()
 {
 streamfd = open(name, O_RDWR);
 if (streamfd < 0)
@@ -73,11 +76,12 @@ public:
 close(streamfd);
 }
 
+const codecs_t _codecs() { return codecs; }
+
 int have_something_to_read(int timeout);
 int read_command_from_device(void);
 int read_command(bool blocking);
 
-
 template 
 bool send(PayloadArgs... payload)
 {
@@ -96,6 +100,7 @@ public:
 private:
 int streamfd = -1;
 std::mutex mutex;
+codecs_t codecs;
 };
 
 
@@ -312,7 +317,6 @@ void X11CursorThread::cursor_changes()
 
 static bool streaming_requested = false;
 static bool quit_requested = false;
-static std::set client_codecs;
 
 int Stream::have_something_to_read(int timeout)
 {
@@ -369,9 +373,9 @@ int Stream::read_command_from_device()
 streaming_requested = msg[0] != 0; /* num_codecs */
 syslog(LOG_INFO, "GOT START_STOP message -- request to %s streaming\n",
streaming_requested ? "START" : "STOP");
-client_codecs.clear();
+codecs.clear();
 for (int i = 1; i <= msg[0]; ++i)
-client_codecs.insert((SpiceVideoCodecType) msg[i]);
+codecs.insert((SpiceVideoCodecType) msg[i]);
 return 1;
 }
 
@@ -461,7 +465,7 @@ do_capture(Stream , const char *streamport, FrameLog 
_log)
 syslog(LOG_INFO, "streaming starts now\n");
 uint64_t time_last = 0;
 
-std::unique_ptr 
capture(agent.GetBestFrameCapture(client_codecs));
+std::unique_ptr 
capture(agent.GetBestFrameCapture(stream.client_codecs()));
 if (!capture)
 throw std::runtime_error("cannot find a suitable capture system");
 
-- 
2.13.5 (Apple Git-94)

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

Re: [Spice-devel] [PATCH 06/17] Replace inefficient C-style initialization with C++-style

[Frediano Ziglio]

> 
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  src/spice-streaming-agent.cpp | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 9b8fd45..69c27a3 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -251,9 +251,7 @@ static void handle_interrupt(int intr)
>  
>  static void register_interrupts(void)
>  {
> -struct sigaction sa;
> -
> -memset(, 0, sizeof(sa));
> +struct sigaction sa = { };
>  sa.sa_handler = handle_interrupt;
>  if ((sigaction(SIGINT, , NULL) != 0) &&
>  (sigaction(SIGTERM, , NULL) != 0)) {

Not your fault but I think the RHEL 7 compiler is not that smart/correct :-(


spice-streaming-agent.cpp: In function 'void register_interrupts()':
spice-streaming-agent.cpp:423:29: warning: missing initializer for member 
'sigaction::__sigaction_handler' [-Wmissing-field-initializers]
 struct sigaction sa = { };
 ^
spice-streaming-agent.cpp:423:29: warning: missing initializer for member 
'sigaction::sa_mask' [-Wmissing-field-initializers]
spice-streaming-agent.cpp:423:29: warning: missing initializer for member 
'sigaction::sa_flags' [-Wmissing-field-initializers]
spice-streaming-agent.cpp:423:29: warning: missing initializer for member 
'sigaction::sa_restorer' [-Wmissing-field-initializers]


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

[Spice-devel] [PATCH 10/17] Since we use a namespace, simplify name of local classes

[Christophe de Dinechin]

From: Christophe de Dinechin 

Signed-off-by: Christophe de Dinechin 
---
 src/spice-streaming-agent.cpp | 28 ++--
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index f845bd0..f0d79ae 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -43,19 +43,24 @@ using namespace spice::streaming_agent;
 
 static ConcreteAgent agent;
 
-struct SpiceStreamFormatMessage
+namespace spice
+{
+namespace streaming_agent
+{
+
+struct FormatMessage
 {
 StreamDevHeader hdr;
 StreamMsgFormat msg;
 };
 
-struct SpiceStreamDataMessage
+struct DataMessage
 {
 StreamDevHeader hdr;
 StreamMsgData msg;
 };
 
-struct SpiceStreamCursorMessage
+struct CursorMessage
 {
 StreamDevHeader hdr;
 StreamMsgCursorSet msg;
@@ -80,6 +85,9 @@ private:
 int fd = -1;
 };
 
+}} // namespace spice::streaming_agent
+
+
 static bool streaming_requested = false;
 static bool quit_requested = false;
 static bool log_binary = false;
@@ -185,9 +193,9 @@ write_all(int fd, const void *buf, const size_t len)
 
 static int spice_stream_send_format(int streamfd, unsigned w, unsigned h, 
uint8_t c)
 {
-const size_t msgsize = sizeof(SpiceStreamFormatMessage);
+const size_t msgsize = sizeof(FormatMessage);
 const size_t hdrsize  = sizeof(StreamDevHeader);
-SpiceStreamFormatMessage msg = {
+FormatMessage msg = {
 .hdr = {
 .protocol_version = STREAM_DEVICE_PROTOCOL,
 .padding = 0,   // Workaround GCC "not implemented" bug
@@ -212,8 +220,8 @@ static int spice_stream_send_format(int streamfd, unsigned 
w, unsigned h, uint8_
 static int spice_stream_send_frame(int streamfd, const void *buf, const 
unsigned size)
 {
 ssize_t n;
-const size_t msgsize = sizeof(SpiceStreamFormatMessage);
-SpiceStreamDataMessage msg = {
+const size_t msgsize = sizeof(FormatMessage);
+DataMessage msg = {
 .hdr = {
 .protocol_version = STREAM_DEVICE_PROTOCOL,
 .padding = 0,   // Workaround GCC "not implemented" bug
@@ -297,13 +305,13 @@ send_cursor(int streamfd,
 return;
 
 const uint32_t cursor_msgsize =
-sizeof(SpiceStreamCursorMessage) + width * height * sizeof(uint32_t);
+sizeof(CursorMessage) + width * height * sizeof(uint32_t);
 const uint32_t hdrsize  = sizeof(StreamDevHeader);
 
 std::unique_ptr storage(new uint8_t[cursor_msgsize]);
 
-SpiceStreamCursorMessage *cursor_msg =
-new(storage.get()) SpiceStreamCursorMessage {
+CursorMessage *cursor_msg =
+new(storage.get()) CursorMessage {
 .hdr = {
 .protocol_version = STREAM_DEVICE_PROTOCOL,
 .padding = 0,   // Workaround GCC internal / not implemented 
compiler error
-- 
2.13.5 (Apple Git-94)

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

[Spice-devel] [PATCH 05/17] Use RAII to cleanup stream in case of exception or return

[Christophe de Dinechin]

From: Christophe de Dinechin 

Get rid of C-style 'goto done' in do_capture.
Get rid of global streamfd, pass it around (cleaned up in later patch)
Fixes a race condition, make sure we only use stream after opening

Signed-off-by: Christophe de Dinechin 
---
 src/spice-streaming-agent.cpp | 79 +--
 1 file changed, 47 insertions(+), 32 deletions(-)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 646d15b..9b8fd45 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -53,14 +53,38 @@ struct SpiceStreamDataMessage
 StreamMsgData msg;
 };
 
+struct SpiceStreamCursorMessage
+{
+StreamDevHeader hdr;
+StreamMsgCursorSet msg;
+};
+
+class Stream
+{
+public:
+Stream(const char *name)
+{
+fd = open(name, O_RDWR);
+if (fd < 0)
+throw std::runtime_error("failed to open streaming device");
+}
+~Stream()
+{
+close(fd);
+}
+operator int() { return fd; }
+
+private:
+int fd = -1;
+};
+
 static bool streaming_requested = false;
 static bool quit_requested = false;
 static bool log_binary = false;
 static std::set client_codecs;
-static int streamfd = -1;
 static std::mutex stream_mtx;
 
-static int have_something_to_read(int timeout)
+static int have_something_to_read(int streamfd, int timeout)
 {
 struct pollfd pollfd = {streamfd, POLLIN, 0};
 
@@ -76,7 +100,7 @@ static int have_something_to_read(int timeout)
 return 0;
 }
 
-static int read_command_from_device(void)
+static int read_command_from_device(int streamfd)
 {
 StreamDevHeader hdr;
 uint8_t msg[64];
@@ -121,18 +145,18 @@ static int read_command_from_device(void)
 return 1;
 }
 
-static int read_command(bool blocking)
+static int read_command(int streamfd, bool blocking)
 {
 int timeout = blocking?-1:0;
 while (!quit_requested) {
-if (!have_something_to_read(timeout)) {
+if (!have_something_to_read(streamfd, timeout)) {
 if (!blocking) {
 return 0;
 }
 sleep(1);
 continue;
 }
-return read_command_from_device();
+return read_command_from_device(streamfd);
 }
 
 return 1;
@@ -157,7 +181,7 @@ write_all(int fd, const void *buf, const size_t len)
 return written;
 }
 
-static int spice_stream_send_format(unsigned w, unsigned h, unsigned c)
+static int spice_stream_send_format(int streamfd, unsigned w, unsigned h, 
unsigned c)
 {
 
 SpiceStreamFormatMessage msg;
@@ -178,7 +202,7 @@ static int spice_stream_send_format(unsigned w, unsigned h, 
unsigned c)
 return EXIT_SUCCESS;
 }
 
-static int spice_stream_send_frame(const void *buf, const unsigned size)
+static int spice_stream_send_frame(int streamfd, const void *buf, const 
unsigned size)
 {
 SpiceStreamDataMessage msg;
 const size_t msgsize = sizeof(msg);
@@ -254,7 +278,7 @@ static void usage(const char *progname)
 }
 
 static void
-send_cursor(unsigned width, unsigned height, int hotspot_x, int hotspot_y,
+send_cursor(int streamfd, unsigned width, unsigned height, int hotspot_x, int 
hotspot_y,
 std::function fill_cursor)
 {
 if (width >= STREAM_MSG_CURSOR_SET_MAX_WIDTH ||
@@ -288,7 +312,7 @@ send_cursor(unsigned width, unsigned height, int hotspot_x, 
int hotspot_y,
 write_all(streamfd, msg.get(), cursor_size);
 }
 
-static void cursor_changes(Display *display, int event_base)
+static void cursor_changes(int streamfd, Display *display, int event_base)
 {
 unsigned long last_serial = 0;
 
@@ -310,25 +334,20 @@ static void cursor_changes(Display *display, int 
event_base)
 for (unsigned i = 0; i < cursor->width * cursor->height; ++i)
 pixels[i] = cursor->pixels[i];
 };
-send_cursor(cursor->width, cursor->height, cursor->xhot, cursor->yhot, 
fill_cursor);
+send_cursor(streamfd,
+cursor->width, cursor->height, cursor->xhot, cursor->yhot, 
fill_cursor);
 }
 }
 
 static void
-do_capture(const char *streamport, FILE *f_log)
+do_capture(int streamfd, const char *streamport, FILE *f_log)
 {
-streamfd = open(streamport, O_RDWR);
-if (streamfd < 0)
-throw std::runtime_error("failed to open the streaming device (" +
- std::string(streamport) + "): "
- + strerror(errno));
-
 unsigned int frame_count = 0;
 while (!quit_requested) {
 while (!quit_requested && !streaming_requested) {
-if (read_command(true) < 0) {
+if (read_command(streamfd, true) < 0) {
 syslog(LOG_ERR, "FAILED to read command\n");
-goto done;
+return;
 }
 }
 
@@ -370,7 +389,7 @@ do_capture(const char *streamport, FILE *f_log)
 
 syslog(LOG_DEBUG, "wXh %uX%u  

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

[Christophe de Dinechin]

From: Christophe de Dinechin 

Signed-off-by: Christophe de Dinechin 
---
 src/spice-streaming-agent.cpp | 47 ++-
 1 file changed, 28 insertions(+), 19 deletions(-)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 69c27a3..1e19e43 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -181,19 +181,24 @@ write_all(int fd, const void *buf, const size_t len)
 return written;
 }
 
-static int spice_stream_send_format(int streamfd, unsigned w, unsigned h, 
unsigned c)
+static int spice_stream_send_format(int streamfd, unsigned w, unsigned h, 
uint8_t c)
 {
-
-SpiceStreamFormatMessage msg;
-const size_t msgsize = sizeof(msg);
-const size_t hdrsize  = sizeof(msg.hdr);
-memset(, 0, msgsize);
-msg.hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
-msg.hdr.type = STREAM_TYPE_FORMAT;
-msg.hdr.size = msgsize - hdrsize; /* includes only the body? */
-msg.msg.width = w;
-msg.msg.height = h;
-msg.msg.codec = c;
+const size_t msgsize = sizeof(SpiceStreamFormatMessage);
+const size_t hdrsize  = sizeof(StreamDevHeader);
+SpiceStreamFormatMessage msg = {
+.hdr = {
+.protocol_version = STREAM_DEVICE_PROTOCOL,
+.padding = 0,   // Workaround GCC "not implemented" bug
+.type = STREAM_TYPE_FORMAT,
+.size = msgsize - hdrsize
+},
+.msg = {
+.width = w,
+.height = h,
+.codec = c,
+.padding1 = { }
+}
+};
 syslog(LOG_DEBUG, "writing format\n");
 std::lock_guard stream_guard(stream_mtx);
 if (write_all(streamfd, , msgsize) != msgsize) {
@@ -204,14 +209,18 @@ static int spice_stream_send_format(int streamfd, 
unsigned w, unsigned h, unsign
 
 static int spice_stream_send_frame(int streamfd, const void *buf, const 
unsigned size)
 {
-SpiceStreamDataMessage msg;
-const size_t msgsize = sizeof(msg);
 ssize_t n;
+const size_t msgsize = sizeof(SpiceStreamFormatMessage);
+SpiceStreamDataMessage msg = {
+.hdr = {
+.protocol_version = STREAM_DEVICE_PROTOCOL,
+.padding = 0,   // Workaround GCC "not implemented" bug
+.type = STREAM_TYPE_DATA,
+.size = size  /* includes only the body? */
+},
+.msg = {}
+};
 
-memset(, 0, msgsize);
-msg.hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
-msg.hdr.type = STREAM_TYPE_DATA;
-msg.hdr.size = size; /* includes only the body? */
 std::lock_guard stream_guard(stream_mtx);
 n = write_all(streamfd, , msgsize);
 syslog(LOG_DEBUG,
@@ -379,7 +388,7 @@ do_capture(int streamfd, const char *streamport, FILE 
*f_log)
 
 if (frame.stream_start) {
 unsigned width, height;
-unsigned char codec;
+uint8_t codec;
 
 width = frame.size.width;
 height = frame.size.height;
-- 
2.13.5 (Apple Git-94)

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

[Spice-devel] [PATCH 11/17] Move read, write and locking into the 'Stream' class

[Christophe de Dinechin]

From: Christophe de Dinechin 

Signed-off-by: Christophe de Dinechin 
---
 src/spice-streaming-agent.cpp | 86 +++
 1 file changed, 47 insertions(+), 39 deletions(-)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index f0d79ae..a989ee7 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -71,18 +71,30 @@ class Stream
 public:
 Stream(const char *name)
 {
-fd = open(name, O_RDWR);
-if (fd < 0)
+streamfd = open(name, O_RDWR);
+if (streamfd < 0)
 throw std::runtime_error("failed to open streaming device");
 }
 ~Stream()
 {
-close(fd);
+close(streamfd);
 }
-operator int() { return fd; }
+operator int() { return streamfd; }
+
+int have_something_to_read(int timeout);
+int read_command_from_device(void);
+int read_command(bool blocking);
+
+size_t write_all(const void *buf, const size_t len);
+int send_format(unsigned w, unsigned h, uint8_t c);
+int send_frame(const void *buf, const unsigned size);
+void send_cursor(uint16_t width, uint16_t height,
+ uint16_t hotspot_x, uint16_t hotspot_y,
+ std::function fill_cursor);
 
 private:
-int fd = -1;
+int streamfd = -1;
+std::mutex mutex;
 };
 
 }} // namespace spice::streaming_agent
@@ -92,9 +104,8 @@ static bool streaming_requested = false;
 static bool quit_requested = false;
 static bool log_binary = false;
 static std::set client_codecs;
-static std::mutex stream_mtx;
 
-static int have_something_to_read(int streamfd, int timeout)
+int Stream::have_something_to_read(int timeout)
 {
 struct pollfd pollfd = {streamfd, POLLIN, 0};
 
@@ -110,13 +121,13 @@ static int have_something_to_read(int streamfd, int 
timeout)
 return 0;
 }
 
-static int read_command_from_device(int streamfd)
+int Stream::read_command_from_device()
 {
 StreamDevHeader hdr;
 uint8_t msg[64];
 int n;
 
-std::lock_guard stream_guard(stream_mtx);
+std::lock_guard stream_guard(mutex);
 n = read(streamfd, , sizeof(hdr));
 if (n != sizeof(hdr)) {
 syslog(LOG_WARNING,
@@ -155,29 +166,28 @@ static int read_command_from_device(int streamfd)
 return 1;
 }
 
-static int read_command(int streamfd, bool blocking)
+int Stream::read_command(bool blocking)
 {
 int timeout = blocking?-1:0;
 while (!quit_requested) {
-if (!have_something_to_read(streamfd, timeout)) {
+if (!have_something_to_read(timeout)) {
 if (!blocking) {
 return 0;
 }
 sleep(1);
 continue;
 }
-return read_command_from_device(streamfd);
+return read_command_from_device();
 }
 
 return 1;
 }
 
-static size_t
-write_all(int fd, const void *buf, const size_t len)
+size_t Stream::write_all(const void *buf, const size_t len)
 {
 size_t written = 0;
 while (written < len) {
-int l = write(fd, (const char *) buf + written, len - written);
+int l = write(streamfd, (const char *) buf + written, len - written);
 if (l < 0) {
 if (errno == EINTR) {
 continue;
@@ -191,7 +201,7 @@ write_all(int fd, const void *buf, const size_t len)
 return written;
 }
 
-static int spice_stream_send_format(int streamfd, unsigned w, unsigned h, 
uint8_t c)
+int Stream::send_format(unsigned w, unsigned h, uint8_t c)
 {
 const size_t msgsize = sizeof(FormatMessage);
 const size_t hdrsize  = sizeof(StreamDevHeader);
@@ -210,14 +220,14 @@ static int spice_stream_send_format(int streamfd, 
unsigned w, unsigned h, uint8_
 }
 };
 syslog(LOG_DEBUG, "writing format\n");
-std::lock_guard stream_guard(stream_mtx);
-if (write_all(streamfd, , msgsize) != msgsize) {
+std::lock_guard stream_guard(mutex);
+if (write_all(, msgsize) != msgsize) {
 return EXIT_FAILURE;
 }
 return EXIT_SUCCESS;
 }
 
-static int spice_stream_send_frame(int streamfd, const void *buf, const 
unsigned size)
+int Stream::send_frame(const void *buf, const unsigned size)
 {
 ssize_t n;
 const size_t msgsize = sizeof(FormatMessage);
@@ -231,8 +241,8 @@ static int spice_stream_send_frame(int streamfd, const void 
*buf, const unsigned
 .msg = {}
 };
 
-std::lock_guard stream_guard(stream_mtx);
-n = write_all(streamfd, , msgsize);
+std::lock_guard stream_guard(mutex);
+n = write_all(, msgsize);
 syslog(LOG_DEBUG,
"wrote %ld bytes of header of data msg with frame of size %u 
bytes\n",
n, msg.hdr.size);
@@ -241,7 +251,7 @@ static int spice_stream_send_frame(int streamfd, const void 
*buf, const unsigned
n, msgsize);
 return EXIT_FAILURE;
 }
-n = write_all(streamfd, buf, size);
+n = write_all(buf, size);
 

[Spice-devel] [PATCH 06/17] Replace inefficient C-style initialization with C++-style

[Christophe de Dinechin]

From: Christophe de Dinechin 

Signed-off-by: Christophe de Dinechin 
---
 src/spice-streaming-agent.cpp | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 9b8fd45..69c27a3 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -251,9 +251,7 @@ static void handle_interrupt(int intr)
 
 static void register_interrupts(void)
 {
-struct sigaction sa;
-
-memset(, 0, sizeof(sa));
+struct sigaction sa = { };
 sa.sa_handler = handle_interrupt;
 if ((sigaction(SIGINT, , NULL) != 0) &&
 (sigaction(SIGTERM, , NULL) != 0)) {
-- 
2.13.5 (Apple Git-94)

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

[Spice-devel] [PATCH 01/17] Add missing header

[Christophe de Dinechin]

From: Christophe de Dinechin 

Signed-off-by: Christophe de Dinechin 
---
 src/concrete-agent.cpp | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
index 891c09b..ac37788 100644
--- a/src/concrete-agent.cpp
+++ b/src/concrete-agent.cpp
@@ -9,6 +9,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "concrete-agent.hpp"
 #include "static-plugin.hpp"
-- 
2.13.5 (Apple Git-94)

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

[Spice-devel] [PATCH 14/17] Create a class encapsulating the X11 display cursor capture

[Christophe de Dinechin]

From: Christophe de Dinechin 

This class will need to be moved to a separate file in a later patch.
This is done in two steps to make the evolution of the code easier to track,
as well as to facilitate later 'git bisect'

Signed-off-by: Christophe de Dinechin 
---
 src/spice-streaming-agent.cpp | 106 ++
 1 file changed, 66 insertions(+), 40 deletions(-)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index faf850c..f4df6be 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -191,6 +191,70 @@ private:
 std::unique_ptr pixels;
 };
 
+
+class X11CursorThread
+{
+public:
+X11CursorThread(Stream );
+~X11CursorThread();
+
+static void record_cursor_changes(X11CursorThread *self) { 
self->cursor_changes(); }
+void cursor_changes();
+
+private:
+Stream 
+Display *display;
+std::thread thread;
+};
+
+
+X11CursorThread::X11CursorThread(Stream )
+: stream(stream),
+  display(XOpenDisplay(NULL)),
+  thread(record_cursor_changes, this)
+{
+if (display == NULL) {
+throw std::runtime_error("failed to open display\n");
+}
+thread.detach();
+}
+
+X11CursorThread::~X11CursorThread()
+{
+XCloseDisplay(display);
+}
+
+void X11CursorThread::cursor_changes()
+{
+unsigned long last_serial = 0;
+
+int event_base, error_base;
+if (!XFixesQueryExtension(display, _base, _base)) {
+syslog(LOG_WARNING, "XFixesQueryExtension failed, not sending cursor 
changes\n");
+return; // also terminates the thread
+}
+Window rootwindow = DefaultRootWindow(display);
+XFixesSelectCursorInput(display, rootwindow, 
XFixesDisplayCursorNotifyMask);
+
+while (true) {
+XEvent event;
+XNextEvent(display, );
+if (event.type != event_base + 1)
+continue;
+
+XFixesCursorImage *cursor = XFixesGetCursorImage(display);
+if (!cursor)
+continue;
+
+if (cursor->cursor_serial == last_serial)
+continue;
+
+last_serial = cursor->cursor_serial;
+if (!stream.send(cursor))
+syslog(LOG_WARNING, "FAILED to send cursor\n");
+}
+}
+
 }} // namespace spice::streaming_agent
 
 
@@ -338,29 +402,6 @@ static void usage(const char *progname)
 exit(1);
 }
 
-static void cursor_changes(Stream *stream, Display *display, int event_base)
-{
-unsigned long last_serial = 0;
-
-while (1) {
-XEvent event;
-XNextEvent(display, );
-if (event.type != event_base + 1)
-continue;
-
-XFixesCursorImage *cursor = XFixesGetCursorImage(display);
-if (!cursor)
-continue;
-
-if (cursor->cursor_serial == last_serial)
-continue;
-
-last_serial = cursor->cursor_serial;
-if (!stream->send(cursor))
-syslog(LOG_WARNING, "FAILED to send cursor\n");
-}
-}
-
 static void
 do_capture(Stream , const char *streamport, FILE *f_log)
 {
@@ -503,25 +544,10 @@ int main(int argc, char* argv[])
 }
 }
 
-Display *display = XOpenDisplay(NULL);
-if (display == NULL) {
-syslog(LOG_ERR, "failed to open display\n");
-return EXIT_FAILURE;
-}
-int event_base, error_base;
-if (!XFixesQueryExtension(display, _base, _base)) {
-syslog(LOG_ERR, "XFixesQueryExtension failed\n");
-return EXIT_FAILURE;
-}
-Window rootwindow = DefaultRootWindow(display);
-XFixesSelectCursorInput(display, rootwindow, 
XFixesDisplayCursorNotifyMask);
-
-Stream streamfd(streamport);
-std::thread cursor_th(cursor_changes, , display, event_base);
-cursor_th.detach();
-
 int ret = EXIT_SUCCESS;
 try {
+Stream streamfd(streamport);
+X11CursorThread cursor_thread(streamfd);
 do_capture(streamfd, streamport, f_log);
 }
 catch (std::runtime_error ) {
-- 
2.13.5 (Apple Git-94)

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

[Spice-devel] [PATCH 15/17] Create FrameLog class to abstract logging of frames

[Christophe de Dinechin]

From: Christophe de Dinechin 

Signed-off-by: Christophe de Dinechin 
---
 src/spice-streaming-agent.cpp | 99 ++-
 1 file changed, 60 insertions(+), 39 deletions(-)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index f4df6be..b0c09d8 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -48,6 +48,17 @@ namespace spice
 namespace streaming_agent
 {
 
+/* returns current time in micro-seconds */
+static uint64_t get_time(void)
+{
+struct timeval now;
+
+gettimeofday(, NULL);
+
+return (uint64_t)now.tv_sec * 100 + (uint64_t)now.tv_usec;
+
+}
+
 class Stream
 {
 public:
@@ -145,6 +156,7 @@ struct FrameMessage : Message
 }
 };
 
+
 struct X11CursorMessage : Message
 {
 X11CursorMessage(XFixesCursorImage *cursor)
@@ -192,6 +204,46 @@ private:
 };
 
 
+class FrameLog
+{
+public:
+FrameLog(const char *filename, bool binary = false);
+~FrameLog();
+
+operator bool() { return log != NULL; }
+void dump(const void *buffer, size_t length);
+
+private:
+FILE *log;
+bool binary;
+};
+
+
+FrameLog::FrameLog(const char *filename, bool binary)
+: log(filename ? fopen(filename, "wb") : NULL), binary(binary)
+{
+if (filename && !log) {
+// We do not abort the program in that case, it's only a warning
+syslog(LOG_WARNING, "Failed to open hexdump log file '%s': %m\n", 
filename);
+}
+}
+
+FrameLog::~FrameLog()
+{
+if (log)
+fclose(log);
+}
+
+void FrameLog::dump(const void *buffer, size_t length)
+{
+if (binary) {
+fwrite(buffer, length, 1, log);
+} else {
+fprintf(log, "%" PRIu64 ": Frame of %zu bytes:\n", get_time(), length);
+hexdump(buffer, length, log);
+}
+}
+
 class X11CursorThread
 {
 public:
@@ -260,7 +312,6 @@ void X11CursorThread::cursor_changes()
 
 static bool streaming_requested = false;
 static bool quit_requested = false;
-static bool log_binary = false;
 static std::set client_codecs;
 
 int Stream::have_something_to_read(int timeout)
@@ -359,17 +410,6 @@ size_t Stream::write_all(const char *what, const void 
*buf, const size_t len)
 return written;
 }
 
-/* returns current time in micro-seconds */
-static uint64_t get_time(void)
-{
-struct timeval now;
-
-gettimeofday(, NULL);
-
-return (uint64_t)now.tv_sec * 100 + (uint64_t)now.tv_usec;
-
-}
-
 static void handle_interrupt(int intr)
 {
 syslog(LOG_INFO, "Got signal %d, exiting", intr);
@@ -392,7 +432,7 @@ static void usage(const char *progname)
 printf("options are:\n");
 printf("\t-p portname  -- virtio-serial port to use\n");
 printf("\t-l file -- log frames to file\n");
-printf("\t--log-binary -- log binary frames (following -l)\n");
+printf("\t-b or --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");
@@ -403,7 +443,7 @@ static void usage(const char *progname)
 }
 
 static void
-do_capture(Stream , const char *streamport, FILE *f_log)
+do_capture(Stream , const char *streamport, FrameLog _log)
 {
 unsigned int frame_count = 0;
 while (!quit_requested) {
@@ -455,14 +495,8 @@ do_capture(Stream , const char *streamport, FILE 
*f_log)
 if (!stream.send(width, height, codec))
 throw std::runtime_error("FAILED to send format message");
 }
-if (f_log) {
-if (log_binary) {
-fwrite(frame.buffer, frame.buffer_size, 1, f_log);
-} else {
-fprintf(f_log, "%" PRIu64 ": Frame of %zu bytes:\n",
-get_time(), frame.buffer_size);
-hexdump(frame.buffer, frame.buffer_size, f_log);
-}
+if (frame_log) {
+frame_log.dump(frame.buffer, frame.buffer_size);
 }
 if (!stream.send(frame.buffer, frame.buffer_size)) {
 syslog(LOG_ERR, "FAILED to send a frame\n");
@@ -484,6 +518,7 @@ int main(int argc, char* argv[])
 const char *streamport = "/dev/virtio-ports/com.redhat.stream.0";
 char opt;
 const char *log_filename = NULL;
+bool log_binary = false;
 int logmask = LOG_UPTO(LOG_WARNING);
 struct option long_options[] = {
 { "log-binary", no_argument, NULL, 'b'},
@@ -496,7 +531,7 @@ int main(int argc, char* argv[])
 
 setlogmask(logmask);
 
-while ((opt = getopt_long(argc, argv, "hp:c:l:d", long_options, NULL)) != 
-1) {
+while ((opt = getopt_long(argc, argv, "bhp:c:l:d", long_options, NULL)) != 
-1) {
 switch (opt) {
 case 0:
 /* Handle long options if needed */
@@ -534,31 +569,17 @@ int main(int argc, char* argv[])
 

[Spice-devel] [PATCH 13/17] Add more meaningful syslog reporting

[Christophe de Dinechin]

From: Christophe de Dinechin 

Signed-off-by: Christophe de Dinechin 
---
 src/spice-streaming-agent.cpp | 21 +
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index c174ea4..faf850c 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -74,10 +74,13 @@ public:
 std::lock_guard stream_guard(mutex);
 size_t expected = message.size(payload...);
 size_t written = message.write(*this, payload...);
-return written == expected;
+bool result = written == expected;
+if (!result)
+syslog(LOG_WARNING, "sent only %zu bytes out of %zu", written, 
expected);
+return result;
 }
 
-size_t write_all(const void *buf, const size_t len);
+size_t write_all(const char *what, const void *buf, const size_t len);
 
 private:
 int streamfd = -1;
@@ -118,7 +121,7 @@ struct FormatMessage : Message
 }
 size_t write(Stream , unsigned w, unsigned h, uint8_t c)
 {
-return stream.write_all(this, sizeof(*this));
+return stream.write_all("FormatMessage", this, sizeof(*this));
 }
 };
 
@@ -137,7 +140,8 @@ struct FrameMessage : Message
 }
 size_t write(Stream , const void *frame, size_t length)
 {
-return stream.write_all(, sizeof(hdr)) + stream.write_all(frame, 
length);
+return stream.write_all("FrameMessage header", , sizeof(hdr))
++  stream.write_all("FrameMessage frame", frame, length);
 }
 };
 
@@ -173,7 +177,8 @@ struct X11CursorMessage : Message
 }
 size_t write(Stream , XFixesCursorImage *cursor)
 {
-return stream.write_all(, sizeof(hdr)) + 
stream.write_all(pixels.get(), hdr.size);
+return stream.write_all("X11CursorMessage header", , sizeof(hdr))
++  stream.write_all("X11CursorMessage pixels", pixels.get(), 
hdr.size);
 }
 void fill_pixels(XFixesCursorImage *cursor)
 {
@@ -272,7 +277,7 @@ int Stream::read_command(bool blocking)
 return 1;
 }
 
-size_t Stream::write_all(const void *buf, const size_t len)
+size_t Stream::write_all(const char *what, const void *buf, const size_t len)
 {
 size_t written = 0;
 while (written < len) {
@@ -281,12 +286,12 @@ size_t Stream::write_all(const void *buf, const size_t 
len)
 if (errno == EINTR) {
 continue;
 }
-syslog(LOG_ERR, "write failed - %m");
+syslog(LOG_ERR, "write %s failed - %m", what);
 return l;
 }
 written += l;
 }
-syslog(LOG_DEBUG, "write_all -- %u bytes written\n", (unsigned)written);
+syslog(LOG_DEBUG, "write %s -- %u bytes written\n", what, 
(unsigned)written);
 return written;
 }
 
-- 
2.13.5 (Apple Git-94)

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

[Spice-devel] [PATCH 17/17] Move the capture loop in the ConcreteAgent, get rid of global agent variable

[Christophe de Dinechin]

From: Christophe de Dinechin 

Signed-off-by: Christophe de Dinechin 
---
 src/concrete-agent.hpp|  4 
 src/spice-streaming-agent.cpp | 14 ++
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/src/concrete-agent.hpp b/src/concrete-agent.hpp
index 5bca23b..346ba6c 100644
--- a/src/concrete-agent.hpp
+++ b/src/concrete-agent.hpp
@@ -14,6 +14,9 @@
 namespace spice {
 namespace streaming_agent {
 
+class Stream;
+class FrameLog;
+
 struct ConcreteConfigureOption: ConfigureOption
 {
 ConcreteConfigureOption(const char *name, const char *value)
@@ -33,6 +36,7 @@ public:
 void Register(Plugin& plugin) override;
 const ConfigureOption* Options() const override;
 void LoadPlugins(const std::string );
+void CaptureLoop(Stream , FrameLog _log);
 // pointer must remain valid
 void AddOption(const char *name, const char *value);
 FrameCapture *GetBestFrameCapture(const std::set& 
codecs);
diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 19f3c07..fcb460c 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -41,8 +41,6 @@
 
 using namespace spice::streaming_agent;
 
-static ConcreteAgent agent;
-
 namespace spice
 {
 namespace streaming_agent
@@ -446,8 +444,8 @@ static void usage(const char *progname)
 exit(1);
 }
 
-static void
-do_capture(Stream , const char *streamport, FrameLog _log)
+
+void ConcreteAgent::CaptureLoop(Stream , FrameLog _log)
 {
 unsigned int frame_count = 0;
 while (!quit_requested) {
@@ -465,7 +463,7 @@ do_capture(Stream , const char *streamport, FrameLog 
_log)
 syslog(LOG_INFO, "streaming starts now\n");
 uint64_t time_last = 0;
 
-std::unique_ptr 
capture(agent.GetBestFrameCapture(stream.client_codecs()));
+std::unique_ptr 
capture(GetBestFrameCapture(stream.client_codecs()));
 if (!capture)
 throw std::runtime_error("cannot find a suitable capture system");
 
@@ -535,6 +533,7 @@ int main(int argc, char* argv[])
 
 setlogmask(logmask);
 
+ConcreteAgent agent;
 while ((opt = getopt_long(argc, argv, "bhp:c:l:d", long_options, NULL)) != 
-1) {
 switch (opt) {
 case 0:
@@ -569,16 +568,15 @@ int main(int argc, char* argv[])
 }
 }
 
-agent.LoadPlugins(PLUGINSDIR);
-
 register_interrupts();
 
 int ret = EXIT_SUCCESS;
 try {
+agent.LoadPlugins(PLUGINSDIR);
 Stream streamfd(streamport);
 X11CursorThread cursor_thread(streamfd);
 FrameLog frame_log(log_filename, log_binary);
-do_capture(streamfd, streamport, frame_log);
+agent.CaptureLoop(streamfd, frame_log);
 }
 catch (std::runtime_error ) {
 syslog(LOG_ERR, "%s\n", err.what());
-- 
2.13.5 (Apple Git-94)

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

[Spice-devel] [PATCH 03/17] Eliminate signed/unsigned warning

[Christophe de Dinechin]

From: Christophe de Dinechin 

Signed-off-by: Christophe de Dinechin 
---
 src/mjpeg-fallback.cpp| 2 +-
 src/spice-streaming-agent.cpp | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
index 634864f..53804d9 100644
--- a/src/mjpeg-fallback.cpp
+++ b/src/mjpeg-fallback.cpp
@@ -57,7 +57,7 @@ private:
 std::vector frame;
 
 // last frame sizes
-uint32_t last_width = ~0u, last_height = ~0u;
+int last_width = ~0u, last_height = ~0u;
 // last time before capture
 uint64_t last_time = 0;
 };
diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 6056129..b5c7e24 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -106,7 +106,7 @@ static int read_command_from_device(void)
 return 0; // return -1;
 }
 n = read(streamfd, , hdr.size);
-if (n != hdr.size) {
+if (n != (int) hdr.size) {
 syslog(LOG_WARNING,
"read command from device FAILED -- read %d expected %d\n",
n, hdr.size);
-- 
2.13.5 (Apple Git-94)

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

Re: [Spice-devel] [PATCH 06/17] Replace inefficient C-style initialization with C++-style

[Christophe de Dinechin]

> On 16 Feb 2018, at 17:43, Frediano Ziglio  wrote:
> 
>> 
>> From: Christophe de Dinechin 
>> 
>> Signed-off-by: Christophe de Dinechin 
>> ---
>> src/spice-streaming-agent.cpp | 4 +---
>> 1 file changed, 1 insertion(+), 3 deletions(-)
>> 
>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>> index 9b8fd45..69c27a3 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -251,9 +251,7 @@ static void handle_interrupt(int intr)
>> 
>> static void register_interrupts(void)
>> {
>> -struct sigaction sa;
>> -
>> -memset(, 0, sizeof(sa));
>> +struct sigaction sa = { };
>> sa.sa_handler = handle_interrupt;
>> if ((sigaction(SIGINT, , NULL) != 0) &&
>> (sigaction(SIGTERM, , NULL) != 0)) {
> 
> Not your fault but I think the RHEL 7 compiler is not that smart/correct :-(
> 
> 
> spice-streaming-agent.cpp: In function 'void register_interrupts()':
> spice-streaming-agent.cpp:423:29: warning: missing initializer for member 
> 'sigaction::__sigaction_handler' [-Wmissing-field-initializers]
> struct sigaction sa = { };
> ^
> spice-streaming-agent.cpp:423:29: warning: missing initializer for member 
> 'sigaction::sa_mask' [-Wmissing-field-initializers]
> spice-streaming-agent.cpp:423:29: warning: missing initializer for member 
> 'sigaction::sa_flags' [-Wmissing-field-initializers]
> spice-streaming-agent.cpp:423:29: warning: missing initializer for member 
> 'sigaction::sa_restorer' [-Wmissing-field-initializers]
> 
Ach. Pity. I had tried on Fed25, not RHEL7 :-(

On Fed25, you get that for { 0 }, but not with { }…



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

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

[Christophe de Dinechin]

From: Christophe de Dinechin 

Signed-off-by: Christophe de Dinechin 
---
 autogen.sh | 9 +
 1 file changed, 9 insertions(+)
 create mode 100755 autogen.sh

diff --git a/autogen.sh b/autogen.sh
new file mode 100755
index 000..afa3e39
--- /dev/null
+++ b/autogen.sh
@@ -0,0 +1,9 @@
+#!/bin/sh
+
+set -e # exit on errors
+
+autoreconf --verbose --force --install
+
+if [ -z "$NOCONFIGURE" ]; then
+./configure ${1+"$@"}
+fi
-- 
2.13.5 (Apple Git-94)

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

[Spice-devel] [PATCH 0/2] Warning and build error fixes

[Christophe de Dinechin]

From: Christophe de Dinechin 

This series eliminates warnings reported by GCC and instantiation
failures reported by clang due to missing  header.

Christophe de Dinechin (2):
  Add missing  header
  Eliminate signed/unsigned warning

 src/concrete-agent.cpp| 1 +
 src/mjpeg-fallback.cpp| 2 +-
 src/spice-streaming-agent.cpp | 2 +-
 3 files changed, 3 insertions(+), 2 deletions(-)

-- 
2.13.5 (Apple Git-94)

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

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

[Christophe de Dinechin]

From: Christophe de Dinechin 

Without this, GCC complains about signed / unsigned comparisons:

mjpeg-fallback.cpp:121:24: warning: comparison between signed and unsigned 
integer expressions [-Wsign-compare]
 if (win_info.width != last_width || win_info.height != last_height) {
 ~~~^

Signed-off-by: Christophe de Dinechin 
---
 src/mjpeg-fallback.cpp| 2 +-
 src/spice-streaming-agent.cpp | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
index 634864f..53804d9 100644
--- a/src/mjpeg-fallback.cpp
+++ b/src/mjpeg-fallback.cpp
@@ -57,7 +57,7 @@ private:
 std::vector frame;
 
 // last frame sizes
-uint32_t last_width = ~0u, last_height = ~0u;
+int last_width = ~0u, last_height = ~0u;
 // last time before capture
 uint64_t last_time = 0;
 };
diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 4ec5e42..27b26a4 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -106,7 +106,7 @@ static int read_command_from_device(void)
 return 0; // return -1;
 }
 n = read(streamfd, , hdr.size);
-if (n != hdr.size) {
+if (n != (int) hdr.size) {
 syslog(LOG_WARNING,
"read command from device FAILED -- read %d expected %d\n",
n, hdr.size);
-- 
2.13.5 (Apple Git-94)

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

[Spice-devel] [PATCH 1/2] Add missing header

[Christophe de Dinechin]

From: Christophe de Dinechin 

Without this header, clang complains with messages such as:

concrete-agent.cpp:62:37: error: invalid operands to binary expression ('const 
std::string' (aka 'const basic_string') and 'const char *')
std::string pattern = directory + "/*.so";
  ~ ^ ~~~
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/iterator:743:1:
 note: candidate template ignored: could not match 
'reverse_iterator' against 'char const[6]'
operator+(typename reverse_iterator<_Iter>::difference_type __n, const 
reverse_iterator<_Iter>& __x)
^

Signed-off-by: Christophe de Dinechin 
---
 src/concrete-agent.cpp | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
index 891c09b..ac37788 100644
--- a/src/concrete-agent.cpp
+++ b/src/concrete-agent.cpp
@@ -9,6 +9,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "concrete-agent.hpp"
 #include "static-plugin.hpp"
-- 
2.13.5 (Apple Git-94)

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

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

[Daniel P . Berrangé]

On Fri, Feb 16, 2018 at 04:18:19PM +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin 
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  autogen.sh | 9 +
>  1 file changed, 9 insertions(+)
>  create mode 100755 autogen.sh
> 
> diff --git a/autogen.sh b/autogen.sh
> new file mode 100755
> index 000..afa3e39
> --- /dev/null
> +++ b/autogen.sh

This assumes you're running autogen.sh from the source tree. Many folks
prefer todo a vpath builds, so it might be invoked from outside the
source tree like

   mkdir build
   cd build
   ../autogen.sh

> @@ -0,0 +1,9 @@
> +#!/bin/sh
> +
> +set -e # exit on errors

srcdir=`dirname $0`
test -z "$srcdir" && srcdir=.

THEDIR=`pwd`
cd $srcdir

> +
> +autoreconf --verbose --force --install

cd $THEDIR

> +
> +if [ -z "$NOCONFIGURE" ]; then
> +./configure ${1+"$@"}

$srcdir/configure 

> +fi

would make it cope with VPATH builds.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-streaming-agent] build: Execute tests while packaging RPM file

[Christophe de Dinechin]

> On 16 Feb 2018, at 12:51, Frediano Ziglio  wrote:
> 
> Signed-off-by: Frediano Ziglio 
> ---
> spice-streaming-agent.spec.in | 3 +++
> 1 file changed, 3 insertions(+)
> 
> diff --git a/spice-streaming-agent.spec.in b/spice-streaming-agent.spec.in
> index 99b1f52..d137840 100644
> --- a/spice-streaming-agent.spec.in
> +++ b/spice-streaming-agent.spec.in
> @@ -33,6 +33,9 @@ agent plugins.
> %configure
> make %{?_smp_mflags} V=1
> 
> +%check
> +make check
> +

Acked-by: Christophe de Dinechin  

> %install
> make install DESTDIR=%{buildroot} V=1
> if test -d "%{buildroot}/%{_libdir}/%{name}/plugins"; then
> -- 
> 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

Re: [Spice-devel] [PATCH spice-server v3 2/2] stream-device: Implement mouse movement

[Jonathon Jongsma]

On Tue, 2018-02-13 at 13:58 +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/stream-device.c | 38 +-
>  1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/server/stream-device.c b/server/stream-device.c
> index f6fd9108..bf03efb6 100644
> --- a/server/stream-device.c
> +++ b/server/stream-device.c
> @@ -47,6 +47,7 @@ struct StreamDevice {
>  StreamMsgFormat format;
>  StreamMsgCapabilities capabilities;
>  StreamMsgCursorSet cursor_set;
> +StreamMsgCursorMove cursor_move;
>  uint8_t buf[STREAM_MSG_CAPABILITIES_MAX_BYTES];
>  } *msg;
>  uint32_t msg_pos;
> @@ -70,7 +71,8 @@ G_DEFINE_TYPE(StreamDevice, stream_device,
> RED_TYPE_CHAR_DEVICE)
>  typedef bool StreamMsgHandler(StreamDevice *dev,
> SpiceCharDeviceInstance *sin)
>  SPICE_GNUC_WARN_UNUSED_RESULT;
>  
> -static StreamMsgHandler handle_msg_format, handle_msg_data,
> handle_msg_cursor_set;
> +static StreamMsgHandler handle_msg_format, handle_msg_data,
> handle_msg_cursor_set,
> +handle_msg_cursor_move;
>  
>  static bool handle_msg_invalid(StreamDevice *dev,
> SpiceCharDeviceInstance *sin,
> const char *error_msg)
> SPICE_GNUC_WARN_UNUSED_RESULT;
> @@ -128,6 +130,13 @@ stream_device_partial_read(StreamDevice *dev,
> SpiceCharDeviceInstance *sin)
>  case STREAM_TYPE_CURSOR_SET:
>  handled = handle_msg_cursor_set(dev, sin);
>  break;
> +case STREAM_TYPE_CURSOR_MOVE:
> +if (dev->hdr.size != sizeof(StreamMsgCursorMove)) {
> +handled = handle_msg_invalid(dev, sin, "Wrong size for
> StreamMsgCursorMove");
> +} else {
> +handled = handle_msg_cursor_move(dev, sin);
> +}
> +break;
>  case STREAM_TYPE_CAPABILITIES:
>  /* FIXME */
>  default:
> @@ -371,6 +380,33 @@ handle_msg_cursor_set(StreamDevice *dev,
> SpiceCharDeviceInstance *sin)
>  return true;
>  }
>  
> +static bool
> +handle_msg_cursor_move(StreamDevice *dev, SpiceCharDeviceInstance
> *sin)
> +{
> +SpiceCharDeviceInterface *sif =
> spice_char_device_get_interface(sin);
> +int n = sif->read(sin, dev->msg->buf + dev->msg_pos, dev-
> >hdr.size - dev->msg_pos);
> +if (n <= 0) {
> +return false;
> +}
> +dev->msg_pos += n;
> +if (dev->msg_pos != dev->hdr.size) {
> +return false;
> +}
> +
> +StreamMsgCursorMove *move = >msg->cursor_move;
> +move->x = GINT32_FROM_LE(move->x);
> +move->y = GINT32_FROM_LE(move->y);
> +
> +RedCursorCmd *cmd = g_new0(RedCursorCmd, 1);
> +cmd->type = QXL_CURSOR_MOVE;
> +cmd->u.position.x = move->x;
> +cmd->u.position.y = move->y;
> +
> +cursor_channel_process_cmd(dev->cursor_channel, cmd);
> +
> +return true;
> +}
> +
>  static void
>  stream_device_send_msg_to_client(RedCharDevice *self, RedPipeItem
> *msg, RedClient *client)
>  {


Acked-by: Jonathon Jongsma 


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

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

[Jonathon Jongsma]

On Tue, 2018-02-13 at 13:58 +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/stream-device.c | 165
> ++---
>  1 file changed, 158 insertions(+), 7 deletions(-)
> 
> diff --git a/server/stream-device.c b/server/stream-device.c
> index b7553c67..f6fd9108 100644
> --- a/server/stream-device.c
> +++ b/server/stream-device.c
> @@ -23,6 +23,7 @@
>  
>  #include "char-device.h"
>  #include "stream-channel.h"
> +#include "cursor-channel.h"
>  #include "reds.h"
>  
>  #define TYPE_STREAM_DEVICE stream_device_get_type()
> @@ -45,13 +46,16 @@ struct StreamDevice {
>  union {
>  StreamMsgFormat format;
>  StreamMsgCapabilities capabilities;
> +StreamMsgCursorSet cursor_set;
>  uint8_t buf[STREAM_MSG_CAPABILITIES_MAX_BYTES];
> -} msg;
> +} *msg;
>  uint32_t msg_pos;
> +uint32_t msg_len;
>  bool has_error;
>  bool opened;
>  bool flow_stopped;
>  StreamChannel *stream_channel;
> +CursorChannel *cursor_channel;
>  };
>  
>  struct StreamDeviceClass {
> @@ -66,7 +70,7 @@ G_DEFINE_TYPE(StreamDevice, stream_device,
> RED_TYPE_CHAR_DEVICE)
>  typedef bool StreamMsgHandler(StreamDevice *dev,
> SpiceCharDeviceInstance *sin)
>  SPICE_GNUC_WARN_UNUSED_RESULT;
>  
> -static StreamMsgHandler handle_msg_format, handle_msg_data;
> +static StreamMsgHandler handle_msg_format, handle_msg_data,
> handle_msg_cursor_set;
>  
>  static bool handle_msg_invalid(StreamDevice *dev,
> SpiceCharDeviceInstance *sin,
> const char *error_msg)
> SPICE_GNUC_WARN_UNUSED_RESULT;
> @@ -121,6 +125,9 @@ stream_device_partial_read(StreamDevice *dev,
> SpiceCharDeviceInstance *sin)
>  case STREAM_TYPE_DATA:
>  handled = handle_msg_data(dev, sin);
>  break;
> +case STREAM_TYPE_CURSOR_SET:
> +handled = handle_msg_cursor_set(dev, sin);
> +break;
>  case STREAM_TYPE_CAPABILITIES:
>  /* FIXME */
>  default:
> @@ -132,6 +139,14 @@ stream_device_partial_read(StreamDevice *dev,
> SpiceCharDeviceInstance *sin)
>   * the next message */
>  if (handled) {
>  dev->hdr_pos = 0;
> +
> +// Reallocate message buffer to the minimum.
> +// Currently the only message that requires resizing is the
> cursor shape,
> +// which is not expected to be sent so often.
> +if (dev->msg_len > sizeof(*dev->msg)) {
> +dev->msg = g_realloc(dev->msg, sizeof(*dev->msg));
> +dev->msg_len = sizeof(*dev->msg);
> +}
>  }
>  
>  if (handled || dev->has_error) {
> @@ -204,7 +219,7 @@ handle_msg_format(StreamDevice *dev,
> SpiceCharDeviceInstance *sin)
>  spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT);
>  }
>  
> -int n = sif->read(sin, dev->msg.buf + dev->msg_pos,
> sizeof(StreamMsgFormat) - dev->msg_pos);
> +int n = sif->read(sin, dev->msg->buf + dev->msg_pos,
> sizeof(StreamMsgFormat) - dev->msg_pos);
>  if (n < 0) {
>  return handle_msg_invalid(dev, sin, NULL);
>  }
> @@ -215,9 +230,9 @@ handle_msg_format(StreamDevice *dev,
> SpiceCharDeviceInstance *sin)
>  return false;
>  }
>  
> -dev->msg.format.width = GUINT32_FROM_LE(dev->msg.format.width);
> -dev->msg.format.height = GUINT32_FROM_LE(dev-
> >msg.format.height);
> -stream_channel_change_format(dev->stream_channel, 
> >msg.format);
> +dev->msg->format.width = GUINT32_FROM_LE(dev->msg-
> >format.width);
> +dev->msg->format.height = GUINT32_FROM_LE(dev->msg-
> >format.height);
> +stream_channel_change_format(dev->stream_channel, >msg-
> >format);
>  return true;
>  }
>  
> @@ -248,6 +263,114 @@ handle_msg_data(StreamDevice *dev,
> SpiceCharDeviceInstance *sin)
>  return dev->hdr.size == 0;
>  }
>  
> +/*
> + * Returns number of bits required for a pixel of a given cursor
> type.
> + *
> + * Take into account mask bits.
> + * Returns 0 on error.
> + */
> +static unsigned int
> +get_cursor_type_bits(unsigned int cursor_type)
> +{
> +switch (cursor_type) {
> +case SPICE_CURSOR_TYPE_ALPHA:
> +// RGBA
> +return 32;
> +case SPICE_CURSOR_TYPE_COLOR24:
> +// RGB + bitmask
> +return 24 + 1;
> +case SPICE_CURSOR_TYPE_COLOR32:
> +// RGBx + bitmask
> +return 32 + 1;
> +default:
> +return 0;
> +}
> +}
> +
> +static RedCursorCmd *
> +stream_msg_cursor_set_to_cursor_cmd(const StreamMsgCursorSet *msg,
> size_t msg_size)
> +{
> +RedCursorCmd *cmd = g_new0(RedCursorCmd, 1);
> +cmd->type = QXL_CURSOR_SET;
> +cmd->u.set.position.x = 0; // TODO
> +cmd->u.set.position.y = 0; // TODO
> +cmd->u.set.visible = 1; // TODO
> +SpiceCursor *cursor = >u.set.shape;
> +cursor->header.unique = 0;
> +cursor->header.type = msg->type;
> +cursor->header.width = GUINT16_FROM_LE(msg->width);
> +cursor->header.height = 

Re: [Spice-devel] [PATCH spice-server v5 1/8] stream-device: Avoid device to get stuck if multiple messages are batched

[Jonathon Jongsma]

On Tue, 2018-02-13 at 15:54 +, Frediano Ziglio wrote:
> If messages are sent together by the agent the device is reading
> only part of the data. This cause Qemu to not poll for new data and
> stream_device_read_msg_from_dev won't be called again.
> This can cause a stall. To avoid this continue handling data
> after a full message was processed.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/stream-device.c | 29 -
>  1 file changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/server/stream-device.c b/server/stream-device.c
> index 4eaa959b..3a7cb306 100644
> --- a/server/stream-device.c
> +++ b/server/stream-device.c
> @@ -71,16 +71,15 @@ static StreamMsgHandler handle_msg_format,
> handle_msg_data;
>  static bool handle_msg_invalid(StreamDevice *dev,
> SpiceCharDeviceInstance *sin,
> const char *error_msg)
> SPICE_GNUC_WARN_UNUSED_RESULT;
>  
> -static RedPipeItem *
> -stream_device_read_msg_from_dev(RedCharDevice *self,
> SpiceCharDeviceInstance *sin)
> +static bool
> +stream_device_partial_read(StreamDevice *dev,
> SpiceCharDeviceInstance *sin)
>  {
> -StreamDevice *dev = STREAM_DEVICE(self);
>  SpiceCharDeviceInterface *sif;
>  int n;
>  bool handled = false;
>  
>  if (dev->has_error || dev->flow_stopped || !dev->stream_channel) 
> {
> -return NULL;
> +return false;
>  }
>  
>  sif = spice_char_device_get_interface(sin);
> @@ -89,7 +88,7 @@ stream_device_read_msg_from_dev(RedCharDevice
> *self, SpiceCharDeviceInstance *si
>  while (dev->hdr_pos < sizeof(dev->hdr)) {
>  n = sif->read(sin, (uint8_t *) >hdr + dev->hdr_pos,
> sizeof(dev->hdr) - dev->hdr_pos);
>  if (n <= 0) {
> -return NULL;
> +return false;
>  }
>  dev->hdr_pos += n;
>  if (dev->hdr_pos >= sizeof(dev->hdr)) {
> @@ -123,6 +122,26 @@ stream_device_read_msg_from_dev(RedCharDevice
> *self, SpiceCharDeviceInstance *si
>  dev->hdr_pos = 0;
>  }
>  
> +if (handled || dev->has_error) {
> +// Qemu put the device on blocking state if we don't read
> all data
> +// so schedule another read.
> +// We arrive here if we processed that entire message or we
> +// got an error, try to read another message or discard the
> +// wrong data
> +return true;
> +}
> +
> +return false;
> +}
> +
> +static RedPipeItem *
> +stream_device_read_msg_from_dev(RedCharDevice *self,
> SpiceCharDeviceInstance *sin)
> +{
> +StreamDevice *dev = STREAM_DEVICE(self);
> +
> +while (stream_device_partial_read(dev, sin)) {
> +continue;
> +}
>  return NULL;
>  }
>  


Thanks. I prefer this approach instead of calling
red_char_device_wakeup(). The name stream_device_partial_read() is OK,
but it's not great. It is really reading up until the end of the next
message *or* until there is no more data to read. I thought about
stream_device_read_to_end_of_message(), but that's not very nice
either. I can't think of a great name.

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

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

[Frediano Ziglio]

> On Tue, 2018-02-13 at 13:58 +, Frediano Ziglio wrote:
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  server/stream-device.c | 165
> > ++---
> >  1 file changed, 158 insertions(+), 7 deletions(-)
> > 
> > diff --git a/server/stream-device.c b/server/stream-device.c
> > index b7553c67..f6fd9108 100644
> > --- a/server/stream-device.c
> > +++ b/server/stream-device.c
> > @@ -23,6 +23,7 @@
> >  
> >  #include "char-device.h"
> >  #include "stream-channel.h"
> > +#include "cursor-channel.h"
> >  #include "reds.h"
> >  
> >  #define TYPE_STREAM_DEVICE stream_device_get_type()
> > @@ -45,13 +46,16 @@ struct StreamDevice {
> >  union {
> >  StreamMsgFormat format;
> >  StreamMsgCapabilities capabilities;
> > +StreamMsgCursorSet cursor_set;
> >  uint8_t buf[STREAM_MSG_CAPABILITIES_MAX_BYTES];
> > -} msg;
> > +} *msg;
> >  uint32_t msg_pos;
> > +uint32_t msg_len;
> >  bool has_error;
> >  bool opened;
> >  bool flow_stopped;
> >  StreamChannel *stream_channel;
> > +CursorChannel *cursor_channel;
> >  };
> >  
> >  struct StreamDeviceClass {
> > @@ -66,7 +70,7 @@ G_DEFINE_TYPE(StreamDevice, stream_device,
> > RED_TYPE_CHAR_DEVICE)
> >  typedef bool StreamMsgHandler(StreamDevice *dev,
> > SpiceCharDeviceInstance *sin)
> >  SPICE_GNUC_WARN_UNUSED_RESULT;
> >  
> > -static StreamMsgHandler handle_msg_format, handle_msg_data;
> > +static StreamMsgHandler handle_msg_format, handle_msg_data,
> > handle_msg_cursor_set;
> >  
> >  static bool handle_msg_invalid(StreamDevice *dev,
> > SpiceCharDeviceInstance *sin,
> > const char *error_msg)
> > SPICE_GNUC_WARN_UNUSED_RESULT;
> > @@ -121,6 +125,9 @@ stream_device_partial_read(StreamDevice *dev,
> > SpiceCharDeviceInstance *sin)
> >  case STREAM_TYPE_DATA:
> >  handled = handle_msg_data(dev, sin);
> >  break;
> > +case STREAM_TYPE_CURSOR_SET:
> > +handled = handle_msg_cursor_set(dev, sin);
> > +break;
> >  case STREAM_TYPE_CAPABILITIES:
> >  /* FIXME */
> >  default:
> > @@ -132,6 +139,14 @@ stream_device_partial_read(StreamDevice *dev,
> > SpiceCharDeviceInstance *sin)
> >   * the next message */
> >  if (handled) {
> >  dev->hdr_pos = 0;
> > +
> > +// Reallocate message buffer to the minimum.
> > +// Currently the only message that requires resizing is the
> > cursor shape,
> > +// which is not expected to be sent so often.
> > +if (dev->msg_len > sizeof(*dev->msg)) {
> > +dev->msg = g_realloc(dev->msg, sizeof(*dev->msg));
> > +dev->msg_len = sizeof(*dev->msg);
> > +}
> >  }
> >  
> >  if (handled || dev->has_error) {
> > @@ -204,7 +219,7 @@ handle_msg_format(StreamDevice *dev,
> > SpiceCharDeviceInstance *sin)
> >  spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT);
> >  }
> >  
> > -int n = sif->read(sin, dev->msg.buf + dev->msg_pos,
> > sizeof(StreamMsgFormat) - dev->msg_pos);
> > +int n = sif->read(sin, dev->msg->buf + dev->msg_pos,
> > sizeof(StreamMsgFormat) - dev->msg_pos);
> >  if (n < 0) {
> >  return handle_msg_invalid(dev, sin, NULL);
> >  }
> > @@ -215,9 +230,9 @@ handle_msg_format(StreamDevice *dev,
> > SpiceCharDeviceInstance *sin)
> >  return false;
> >  }
> >  
> > -dev->msg.format.width = GUINT32_FROM_LE(dev->msg.format.width);
> > -dev->msg.format.height = GUINT32_FROM_LE(dev-
> > >msg.format.height);
> > -stream_channel_change_format(dev->stream_channel, 
> > >msg.format);
> > +dev->msg->format.width = GUINT32_FROM_LE(dev->msg-
> > >format.width);
> > +dev->msg->format.height = GUINT32_FROM_LE(dev->msg-
> > >format.height);
> > +stream_channel_change_format(dev->stream_channel, >msg-
> > >format);
> >  return true;
> >  }
> >  
> > @@ -248,6 +263,114 @@ handle_msg_data(StreamDevice *dev,
> > SpiceCharDeviceInstance *sin)
> >  return dev->hdr.size == 0;
> >  }
> >  
> > +/*
> > + * Returns number of bits required for a pixel of a given cursor
> > type.
> > + *
> > + * Take into account mask bits.
> > + * Returns 0 on error.
> > + */
> > +static unsigned int
> > +get_cursor_type_bits(unsigned int cursor_type)
> > +{
> > +switch (cursor_type) {
> > +case SPICE_CURSOR_TYPE_ALPHA:
> > +// RGBA
> > +return 32;
> > +case SPICE_CURSOR_TYPE_COLOR24:
> > +// RGB + bitmask
> > +return 24 + 1;
> > +case SPICE_CURSOR_TYPE_COLOR32:
> > +// RGBx + bitmask
> > +return 32 + 1;
> > +default:
> > +return 0;
> > +}
> > +}
> > +
> > +static RedCursorCmd *
> > +stream_msg_cursor_set_to_cursor_cmd(const StreamMsgCursorSet *msg,
> > size_t msg_size)
> > +{
> > +RedCursorCmd *cmd = g_new0(RedCursorCmd, 1);
> > +cmd->type = QXL_CURSOR_SET;
> > +cmd->u.set.position.x = 0; // TODO
> > +

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

[Jonathon Jongsma]

On Fri, 2018-02-16 at 17:17 -0500, Frediano Ziglio wrote:
> > On Tue, 2018-02-13 at 13:58 +, Frediano Ziglio wrote:
> > > Signed-off-by: Frediano Ziglio 
> > > ---
> > >  server/stream-device.c | 165
> > > ++---
> > >  1 file changed, 158 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/server/stream-device.c b/server/stream-device.c
> > > index b7553c67..f6fd9108 100644
> > > --- a/server/stream-device.c
> > > +++ b/server/stream-device.c
> > > @@ -23,6 +23,7 @@
> > >  
> > >  #include "char-device.h"
> > >  #include "stream-channel.h"
> > > +#include "cursor-channel.h"
> > >  #include "reds.h"
> > >  
> > >  #define TYPE_STREAM_DEVICE stream_device_get_type()
> > > @@ -45,13 +46,16 @@ struct StreamDevice {
> > >  union {
> > >  StreamMsgFormat format;
> > >  StreamMsgCapabilities capabilities;
> > > +StreamMsgCursorSet cursor_set;
> > >  uint8_t buf[STREAM_MSG_CAPABILITIES_MAX_BYTES];
> > > -} msg;
> > > +} *msg;
> > >  uint32_t msg_pos;
> > > +uint32_t msg_len;
> > >  bool has_error;
> > >  bool opened;
> > >  bool flow_stopped;
> > >  StreamChannel *stream_channel;
> > > +CursorChannel *cursor_channel;
> > >  };
> > >  
> > >  struct StreamDeviceClass {
> > > @@ -66,7 +70,7 @@ G_DEFINE_TYPE(StreamDevice, stream_device,
> > > RED_TYPE_CHAR_DEVICE)
> > >  typedef bool StreamMsgHandler(StreamDevice *dev,
> > > SpiceCharDeviceInstance *sin)
> > >  SPICE_GNUC_WARN_UNUSED_RESULT;
> > >  
> > > -static StreamMsgHandler handle_msg_format, handle_msg_data;
> > > +static StreamMsgHandler handle_msg_format, handle_msg_data,
> > > handle_msg_cursor_set;
> > >  
> > >  static bool handle_msg_invalid(StreamDevice *dev,
> > > SpiceCharDeviceInstance *sin,
> > > const char *error_msg)
> > > SPICE_GNUC_WARN_UNUSED_RESULT;
> > > @@ -121,6 +125,9 @@ stream_device_partial_read(StreamDevice *dev,
> > > SpiceCharDeviceInstance *sin)
> > >  case STREAM_TYPE_DATA:
> > >  handled = handle_msg_data(dev, sin);
> > >  break;
> > > +case STREAM_TYPE_CURSOR_SET:
> > > +handled = handle_msg_cursor_set(dev, sin);
> > > +break;
> > >  case STREAM_TYPE_CAPABILITIES:
> > >  /* FIXME */
> > >  default:
> > > @@ -132,6 +139,14 @@ stream_device_partial_read(StreamDevice
> > > *dev,
> > > SpiceCharDeviceInstance *sin)
> > >   * the next message */
> > >  if (handled) {
> > >  dev->hdr_pos = 0;
> > > +
> > > +// Reallocate message buffer to the minimum.
> > > +// Currently the only message that requires resizing is
> > > the
> > > cursor shape,
> > > +// which is not expected to be sent so often.
> > > +if (dev->msg_len > sizeof(*dev->msg)) {
> > > +dev->msg = g_realloc(dev->msg, sizeof(*dev->msg));
> > > +dev->msg_len = sizeof(*dev->msg);
> > > +}
> > >  }
> > >  
> > >  if (handled || dev->has_error) {
> > > @@ -204,7 +219,7 @@ handle_msg_format(StreamDevice *dev,
> > > SpiceCharDeviceInstance *sin)
> > >  spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT);
> > >  }
> > >  
> > > -int n = sif->read(sin, dev->msg.buf + dev->msg_pos,
> > > sizeof(StreamMsgFormat) - dev->msg_pos);
> > > +int n = sif->read(sin, dev->msg->buf + dev->msg_pos,
> > > sizeof(StreamMsgFormat) - dev->msg_pos);
> > >  if (n < 0) {
> > >  return handle_msg_invalid(dev, sin, NULL);
> > >  }
> > > @@ -215,9 +230,9 @@ handle_msg_format(StreamDevice *dev,
> > > SpiceCharDeviceInstance *sin)
> > >  return false;
> > >  }
> > >  
> > > -dev->msg.format.width = GUINT32_FROM_LE(dev-
> > > >msg.format.width);
> > > -dev->msg.format.height = GUINT32_FROM_LE(dev-
> > > > msg.format.height);
> > > 
> > > -stream_channel_change_format(dev->stream_channel, 
> > > > msg.format);
> > > 
> > > +dev->msg->format.width = GUINT32_FROM_LE(dev->msg-
> > > > format.width);
> > > 
> > > +dev->msg->format.height = GUINT32_FROM_LE(dev->msg-
> > > > format.height);
> > > 
> > > +stream_channel_change_format(dev->stream_channel, >msg-
> > > > format);
> > > 
> > >  return true;
> > >  }
> > >  
> > > @@ -248,6 +263,114 @@ handle_msg_data(StreamDevice *dev,
> > > SpiceCharDeviceInstance *sin)
> > >  return dev->hdr.size == 0;
> > >  }
> > >  
> > > +/*
> > > + * Returns number of bits required for a pixel of a given cursor
> > > type.
> > > + *
> > > + * Take into account mask bits.
> > > + * Returns 0 on error.
> > > + */
> > > +static unsigned int
> > > +get_cursor_type_bits(unsigned int cursor_type)
> > > +{
> > > +switch (cursor_type) {
> > > +case SPICE_CURSOR_TYPE_ALPHA:
> > > +// RGBA
> > > +return 32;
> > > +case SPICE_CURSOR_TYPE_COLOR24:
> > > +// RGB + bitmask
> > > +return 24 + 1;
> > > +case SPICE_CURSOR_TYPE_COLOR32:
> > > +//