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

[Christophe de Dinechin]

> On 20 Feb 2018, at 22:38, Jonathon Jongsma  wrote:
> 
> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin 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;
>> +};
>> +
>> +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;
>> +};
> 
> 
> What about making the 'Stream' constructor take an fd so that the same
> class can be used to encapsulate e.g. socket fds as well?

 I guess you are thinking about the vsock case?

Well, the whole idea of RAII is that one class has both open and close, so I 
don’t think passing an fd as input would work.

If we need to deal with a socket fd, we could have an additional ctor doing a 
‘socket’ call (the dtor closing the socket), and another one looking like 
‘accept’, something like:

Stream(int domain, int type, int protocol); // socket()-like
Stream(int sockfd, struct sockaddr *addr, socklen_t *len); // 
accept-like

If we do that, then each ‘Stream’ would own one fd and only one, and be 
responsible for closing it. But then, we’d probably want to split the Stream 
class between the fd-management aspect (deals with the fd only, offers read and 
write), and the message packaging aspect (holds a mutex, responsible for 
writing or receiving messages, the function I called ‘send’, …)

That being said, my objective was not to create some sort of general purpose 
class, KISS, just what we need. If what we need is for vsocks, that can be done 
in a later step as I outlined above without too much disruption.


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

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

[Marc-André Lureau]

Hi

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

It may be possible to have channels that are secured above the spice
channels (with the so called "port" channel), so you may want to have
a mix of plain and tls prots. In practice, I don't think anyone does
that.

As you said, it is best to enforce the behaviour on the server side, in qemu.

On the client side, we could default to --spice-secure-channels=all
and have some extra warnings.

In any case, that URI series doesn't need to be delayed for it I imagine.


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

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

[Jonathon Jongsma]

On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin 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;
> +};
> +
> +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;
> +};


What about making the 'Stream' constructor take an fd so that the same
class can be used to encapsulate e.g. socket fds as well?


> +
>  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) + "): "
> - 

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

[Jonathon Jongsma]

On Tue, 2018-02-20 at 14:29 +0100, Lukáš Hrázký wrote:
> On Tue, 2018-02-20 at 10:47 +0100, Christophe de Dinechin wrote:
> > > On 20 Feb 2018, at 10:43, Lukáš Hrázký 
> > > wrote:
> > > 
> > > On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> > > > 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)
> > > 
> > > I would like to name the class something more descriptive for
> > > what it
> > > is becoming. Class named Stream in a file named
> > > "stream.{cpp,hpp}"
> > > could be almost anything.
> > 
> > But it’s not named “Stream”, it’s called
> > “spice::streaming_agent::Stream” ;-)
> > 
> > I chose short names because I was in that namespace. Otherwise, I
> > agree with you.
> > 
> > Do you think that the name is still too vague even within the
> > namespace?
> 
> I think so. Everything in streaming agent is in that namespace, even
> if
> it wasn't, you know you're looking at the streaming agent code and
> think of the types in that context. Stream is still a pretty generic
> name, I suppose you could imagine a number of things under it.
> 
> So what is this class in our case. It handles the socket
> communication
> over the streaming channel to the server. Is it accurate to call it a
> channel here? If so, maybe StreamingChannel?

It's true that "Stream" in the context of the streaming agent could
make you think that it was actually representing the encoded video
stream, rather than an encapsulation of a communication channel. But I
don't like the name StreamDispatcher. Glib uses the name GIOChannel for
something similar. Maybe IOChannel?

> 
> > > My best name is StreamDispatcher so far, not
> > > entirely happy about it :)
> > 
> > 
> > > 
> > > > {
> > > > -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);
> > > 
> > > This method could also use a better name. write_bytes()?
> > > write_buffer()?
> > 
> > I intended to do a rename in a follow up. My current choice was
> > “write_packet”, because precisely, it’s not writing bytes or a
> > buffer, it’s making sure the whole packet gets sent.
> 
> What do you mean by packet here? Does it have a specific meaning in
> this context? It just sends an array of binary data, doesn't it? Like
> later on you use it to write the header and message body separately.
> 
> > > 
> > > > +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 != 

[Spice-devel] [PATCH spice-streaming-agent v3 1/4] Use exception handling data from streaming device

[Frediano Ziglio]

In all paths errors from this function are treated like fatal
error, there's no need to handle all manually potentially
forgetting to handle errors.
Also avoid to deal directly with logging moving the responsibility
to other layers.

Signed-off-by: Frediano Ziglio 
---
 src/spice-streaming-agent.cpp | 35 ++-
 1 file changed, 14 insertions(+), 21 deletions(-)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 1f41a6f..5613934 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -77,7 +77,7 @@ static int have_something_to_read(int timeout)
 return 0;
 }
 
-static int read_command_from_device(void)
+static void read_command_from_device(void)
 {
 StreamDevHeader hdr;
 uint8_t msg[64];
@@ -86,40 +86,32 @@ static int read_command_from_device(void)
 std::lock_guard stream_guard(stream_mtx);
 n = read(streamfd, , sizeof(hdr));
 if (n != sizeof(hdr)) {
-syslog(LOG_WARNING,
-   "read command from device FAILED -- read %d expected %lu\n",
-   n, sizeof(hdr));
-return -1;
+throw std::runtime_error("read command from device FAILED -- read " + 
std::to_string(n) +
+ " expected " + std::to_string(sizeof(hdr)));
 }
 if (hdr.protocol_version != STREAM_DEVICE_PROTOCOL) {
-syslog(LOG_WARNING, "BAD VERSION %d (expected is %d)\n", 
hdr.protocol_version,
-   STREAM_DEVICE_PROTOCOL);
-return -1;
+throw std::runtime_error("BAD VERSION " + 
std::to_string(hdr.protocol_version) +
+ " (expected is " + 
std::to_string(STREAM_DEVICE_PROTOCOL) + ")");
 }
 if (hdr.type != STREAM_TYPE_START_STOP) {
-syslog(LOG_WARNING, "UNKNOWN msg of type %d\n", hdr.type);
-return -1;
+throw std::runtime_error("UNKNOWN msg of type " + 
std::to_string(hdr.type));
 }
 if (hdr.size >= sizeof(msg)) {
-syslog(LOG_WARNING,
-   "msg size (%d) is too long (longer than %lu)\n",
-   hdr.size, sizeof(msg));
-return -1;
+throw std::runtime_error("msg size (" + std::to_string(hdr.size) + ") 
is too long "
+ "(longer than " + std::to_string(sizeof(msg)) 
+ ")");
 }
 n = read(streamfd, , hdr.size);
 if (n != hdr.size) {
-syslog(LOG_WARNING,
-   "read command from device FAILED -- read %d expected %d\n",
-   n, hdr.size);
-return -1;
+throw std::runtime_error("read command from device FAILED -- read " + 
std::to_string(n) +
+ " expected " + std::to_string(hdr.size));
 }
 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();
-for (int i = 1; i <= msg[0]; ++i)
+for (int i = 1; i <= msg[0]; ++i) {
 client_codecs.insert((SpiceVideoCodecType) msg[i]);
-return 1;
+}
 }
 
 static int read_command(bool blocking)
@@ -133,7 +125,8 @@ static int read_command(bool blocking)
 sleep(1);
 continue;
 }
-return read_command_from_device();
+read_command_from_device();
+break;
 }
 
 return 1;
-- 
2.14.3

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

[Spice-devel] [PATCH spice-streaming-agent v3 0/4] Handle more completely device messages

[Frediano Ziglio]

Avoids losing sync with the device.
Handle some missing messages.

Changes since v2:
- fix typo;
- use the term "handle" instead of "read";
- return void from handling functions.

Changes since v1:
- avoid the usage of ERROR macro.

Frediano Ziglio (4):
  Use exception handling data from streaming device
  Separate handling start/stop message from server
  Handle capabilities
  Stub to handle errors from server

 src/spice-streaming-agent.cpp | 94 +++
 1 file changed, 68 insertions(+), 26 deletions(-)

-- 
2.14.3

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

[Spice-devel] [PATCH spice-streaming-agent v3 2/4] Separate handling start/stop message from server

[Frediano Ziglio]

Prepare to add support for other messages.

Signed-off-by: Frediano Ziglio 
---
 src/spice-streaming-agent.cpp | 26 ++
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 5613934..8d91f2d 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -77,10 +77,11 @@ static int have_something_to_read(int timeout)
 return 0;
 }
 
+static void handle_stream_start_stop(uint32_t len);
+
 static void read_command_from_device(void)
 {
 StreamDevHeader hdr;
-uint8_t msg[64];
 int n;
 
 std::lock_guard stream_guard(stream_mtx);
@@ -93,17 +94,26 @@ static void read_command_from_device(void)
 throw std::runtime_error("BAD VERSION " + 
std::to_string(hdr.protocol_version) +
  " (expected is " + 
std::to_string(STREAM_DEVICE_PROTOCOL) + ")");
 }
-if (hdr.type != STREAM_TYPE_START_STOP) {
-throw std::runtime_error("UNKNOWN msg of type " + 
std::to_string(hdr.type));
+
+switch (hdr.type) {
+case STREAM_TYPE_START_STOP:
+return handle_stream_start_stop(hdr.size);
 }
-if (hdr.size >= sizeof(msg)) {
-throw std::runtime_error("msg size (" + std::to_string(hdr.size) + ") 
is too long "
+throw std::runtime_error("UNKNOWN msg of type " + 
std::to_string(hdr.type));
+}
+
+static void handle_stream_start_stop(uint32_t len)
+{
+uint8_t msg[256];
+
+if (len >= sizeof(msg)) {
+throw std::runtime_error("msg size (" + std::to_string(len) + ") is 
too long "
  "(longer than " + std::to_string(sizeof(msg)) 
+ ")");
 }
-n = read(streamfd, , hdr.size);
-if (n != hdr.size) {
+int n = read(streamfd, , len);
+if (n != len) {
 throw std::runtime_error("read command from device FAILED -- read " + 
std::to_string(n) +
- " expected " + std::to_string(hdr.size));
+ " expected " + std::to_string(len));
 }
 streaming_requested = (msg[0] != 0); /* num_codecs */
 syslog(LOG_INFO, "GOT START_STOP message -- request to %s streaming\n",
-- 
2.14.3

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

[Spice-devel] [PATCH spice-streaming-agent v3 3/4] Handle capabilities

[Frediano Ziglio]

Do not bail if the server is attempting to communicate some extensions
but just ignore as at the moment we support none.

Signed-off-by: Frediano Ziglio 
---
 src/spice-streaming-agent.cpp | 30 ++
 1 file changed, 30 insertions(+)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 8d91f2d..31c655c 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -40,6 +40,8 @@
 
 using namespace spice::streaming_agent;
 
+static size_t write_all(int fd, const void *buf, const size_t len);
+
 static ConcreteAgent agent;
 
 struct SpiceStreamFormatMessage
@@ -77,6 +79,7 @@ static int have_something_to_read(int timeout)
 return 0;
 }
 
+static void handle_stream_capabilities(uint32_t len);
 static void handle_stream_start_stop(uint32_t len);
 
 static void read_command_from_device(void)
@@ -96,6 +99,8 @@ static void read_command_from_device(void)
 }
 
 switch (hdr.type) {
+case STREAM_TYPE_CAPABILITIES:
+return handle_stream_capabilities(hdr.size);
 case STREAM_TYPE_START_STOP:
 return handle_stream_start_stop(hdr.size);
 }
@@ -124,6 +129,31 @@ static void handle_stream_start_stop(uint32_t len)
 }
 }
 
+static void handle_stream_capabilities(uint32_t len)
+{
+uint8_t caps[STREAM_MSG_CAPABILITIES_MAX_BYTES];
+
+if (len > sizeof(caps)) {
+throw std::runtime_error("capability message too long");
+}
+int n = read(streamfd, caps, len);
+if (n != len) {
+throw std::runtime_error("read command from device FAILED -- read " + 
std::to_string(n) +
+ " expected " + std::to_string(len));
+}
+
+// we currently do not support extensions so just reply so
+StreamDevHeader hdr = {
+STREAM_DEVICE_PROTOCOL,
+0,
+STREAM_TYPE_CAPABILITIES,
+0
+};
+if (write_all(streamfd, , sizeof(hdr)) != sizeof(hdr)) {
+throw std::runtime_error("error writing capabilities");
+}
+}
+
 static int read_command(bool blocking)
 {
 int timeout = blocking?-1:0;
-- 
2.14.3

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

[Spice-devel] [PATCH spice-streaming-agent v3 4/4] Stub to handle errors from server

[Frediano Ziglio]

Base error message handling.

Signed-off-by: Frediano Ziglio 
---
 src/spice-streaming-agent.cpp | 9 +
 1 file changed, 9 insertions(+)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 31c655c..343b252 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -81,6 +81,7 @@ static int have_something_to_read(int timeout)
 
 static void handle_stream_capabilities(uint32_t len);
 static void handle_stream_start_stop(uint32_t len);
+static void handle_stream_error(uint32_t len);
 
 static void read_command_from_device(void)
 {
@@ -101,6 +102,8 @@ static void read_command_from_device(void)
 switch (hdr.type) {
 case STREAM_TYPE_CAPABILITIES:
 return handle_stream_capabilities(hdr.size);
+case STREAM_TYPE_NOTIFY_ERROR:
+return handle_stream_error(hdr.size);
 case STREAM_TYPE_START_STOP:
 return handle_stream_start_stop(hdr.size);
 }
@@ -154,6 +157,12 @@ static void handle_stream_capabilities(uint32_t len)
 }
 }
 
+static void handle_stream_error(uint32_t len)
+{
+// TODO read message and use it
+throw std::runtime_error("got an error message from server");
+}
+
 static int read_command(bool blocking)
 {
 int timeout = blocking?-1:0;
-- 
2.14.3

___
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/5] Handle capabilities

[Frediano Ziglio]

> On Tue, 2018-02-20 at 18:33 +0100, Christophe de Dinechin wrote:
> > > On 20 Feb 2018, at 15:30, Frediano Ziglio 
> > > wrote:
> > > 
> > > Do not bail if the server is attempting to communicate some
> > > extensions
> > > but just ignore as at the moment we support none.
> > > 
> > > Signed-off-by: Frediano Ziglio 
> > > ---
> > > src/spice-streaming-agent.cpp | 31 +++
> > > 1 file changed, 31 insertions(+)
> > > 
> > > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-
> > > agent.cpp
> > > index d72c30b..9547e49 100644
> > > --- a/src/spice-streaming-agent.cpp
> > > +++ b/src/spice-streaming-agent.cpp
> > > @@ -40,6 +40,8 @@
> > > 
> > > using namespace spice::streaming_agent;
> > > 
> > > +static size_t write_all(int fd, const void *buf, const size_t
> > > len);
> > > +
> > > static ConcreteAgent agent;
> > > 
> > > struct SpiceStreamFormatMessage
> > > @@ -77,6 +79,7 @@ static int have_something_to_read(int timeout)
> > > return 0;
> > > }
> > > 
> > > +static int read_stream_capabilities(uint32_t len);
> > > static int read_stream_start_stop(uint32_t len);
> > > 
> > > static int read_command_from_device(void)
> > > @@ -96,6 +99,8 @@ static int read_command_from_device(void)
> > > }
> > > 
> > > switch (hdr.type) {
> > > +case STREAM_TYPE_CAPABILITIES:
> > > +return read_stream_capabilities(hdr.size);
> > > case STREAM_TYPE_START_STOP:
> > > return read_stream_start_stop(hdr.size);
> > > }
> > > @@ -124,6 +129,32 @@ static int read_stream_start_stop(uint32_t
> > > len)
> > > return 1;
> > > }
> > > 
> > > +static int read_stream_capabilities(uint32_t len)
> > 
> > It says “read” but reads and writes.
> > 

Yes, maybe better name them "handle_XXX".

> > Do you have many other pending patches that introduce new messages?
> > 

No, with these all possible current messages are supported.

> > This messes with the outgoing message refactoring I have in flight,
> > but also with the incoming refactoring that Lukas is looking at.
> > 
> > So my guess is this one should go first (it’s relatively small). Or
> > if it’s OK to wait a little, be rewritten on top of my series, but
> > I’m not too sure how to do that in the spirit of what Lukas wants to
> > do for input refactoring.
> > 

Depends on how much stuff this blocks (testing or other) and how
long is the "little", I would say end of week.

> > 
> > > +{
> > > +uint8_t caps[STREAM_MSG_CAPABILITIES_MAX_BYTES];
> > > +
> > > +if (len > sizeof(caps)) {
> > > +throw std::runtime_error("capability message too long");
> > > +}
> > > +int n = read(streamfd, caps, len);
> > > +if (n != len) {
> > > +throw std::runtime_error("read command from device FAILED
> > > -- read " + std::to_string(n) +
> > > + " expected " +
> > > std::to_string(len));
> > > +}
> 
> In most other places in the stream device, we assume that we may get
> partial reads (including partial headers, which are small). Here you
> report an error if you don't read the full message in one read. That
> seems inconsistent.
> 

I think the inconsistency is not having a read_all and having a
write_all.
Note that on server code is async so we need to support partial
read, here not much (calls are blocking). The current code works
as we don't handle much interrupts beside for terminating so
handling EINTR as an error is fine but would be better to
handle EINTR even here.
If we move to socket without a read_all we'll have problems
as sockets are more willing to have partial read.

> > > +
> > > +// we currently do not suppor extensions so just reply so
> 
> "suppor" -> "support"
> 
> > > +StreamDevHeader hdr = {
> > > +STREAM_DEVICE_PROTOCOL,
> > > +0,
> > > +STREAM_TYPE_CAPABILITIES,
> > > +0
> > > +};
> > > +if (write_all(streamfd, , sizeof(hdr)) != sizeof(hdr)) {
> > > +throw std::runtime_error("error writing capabilities");
> > > +}
> > > +return 1;
> 
> It seems that the return value of these functions are becoming
> pointless after we have started introducing more exceptions.
> 

Yes, I think would be better to return a void instead, at least on
the "handle" functions.

> > > +}
> > > +
> > > static int read_command(bool blocking)
> > > {
> > > int timeout = blocking?-1:0;

Frediano
___
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/5] Handle capabilities

[Jonathon Jongsma]

On Tue, 2018-02-20 at 18:33 +0100, Christophe de Dinechin wrote:
> > On 20 Feb 2018, at 15:30, Frediano Ziglio 
> > wrote:
> > 
> > Do not bail if the server is attempting to communicate some
> > extensions
> > but just ignore as at the moment we support none.
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> > src/spice-streaming-agent.cpp | 31 +++
> > 1 file changed, 31 insertions(+)
> > 
> > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-
> > agent.cpp
> > index d72c30b..9547e49 100644
> > --- a/src/spice-streaming-agent.cpp
> > +++ b/src/spice-streaming-agent.cpp
> > @@ -40,6 +40,8 @@
> > 
> > using namespace spice::streaming_agent;
> > 
> > +static size_t write_all(int fd, const void *buf, const size_t
> > len);
> > +
> > static ConcreteAgent agent;
> > 
> > struct SpiceStreamFormatMessage
> > @@ -77,6 +79,7 @@ static int have_something_to_read(int timeout)
> > return 0;
> > }
> > 
> > +static int read_stream_capabilities(uint32_t len);
> > static int read_stream_start_stop(uint32_t len);
> > 
> > static int read_command_from_device(void)
> > @@ -96,6 +99,8 @@ static int read_command_from_device(void)
> > }
> > 
> > switch (hdr.type) {
> > +case STREAM_TYPE_CAPABILITIES:
> > +return read_stream_capabilities(hdr.size);
> > case STREAM_TYPE_START_STOP:
> > return read_stream_start_stop(hdr.size);
> > }
> > @@ -124,6 +129,32 @@ static int read_stream_start_stop(uint32_t
> > len)
> > return 1;
> > }
> > 
> > +static int read_stream_capabilities(uint32_t len)
> 
> It says “read” but reads and writes.
> 
> Do you have many other pending patches that introduce new messages?
> 
> This messes with the outgoing message refactoring I have in flight,
> but also with the incoming refactoring that Lukas is looking at.
> 
> So my guess is this one should go first (it’s relatively small). Or
> if it’s OK to wait a little, be rewritten on top of my series, but
> I’m not too sure how to do that in the spirit of what Lukas wants to
> do for input refactoring.
> 
> 
> > +{
> > +uint8_t caps[STREAM_MSG_CAPABILITIES_MAX_BYTES];
> > +
> > +if (len > sizeof(caps)) {
> > +throw std::runtime_error("capability message too long");
> > +}
> > +int n = read(streamfd, caps, len);
> > +if (n != len) {
> > +throw std::runtime_error("read command from device FAILED
> > -- read " + std::to_string(n) +
> > + " expected " +
> > std::to_string(len));
> > +}

In most other places in the stream device, we assume that we may get
partial reads (including partial headers, which are small). Here you
report an error if you don't read the full message in one read. That
seems inconsistent.

> > +
> > +// we currently do not suppor extensions so just reply so

"suppor" -> "support"

> > +StreamDevHeader hdr = {
> > +STREAM_DEVICE_PROTOCOL,
> > +0,
> > +STREAM_TYPE_CAPABILITIES,
> > +0
> > +};
> > +if (write_all(streamfd, , sizeof(hdr)) != sizeof(hdr)) {
> > +throw std::runtime_error("error writing capabilities");
> > +}
> > +return 1;

It seems that the return value of these functions are becoming
pointless after we have started introducing more exceptions.

> > +}
> > +
> > static int read_command(bool blocking)
> > {
> > int timeout = blocking?-1:0;
> > -- 
> > 2.14.3
> > 
> > ___
> > Spice-devel mailing list
> > Spice-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel 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]

[Forgot to send, sorry for delay]

> On 19 Feb 2018, at 19:03, Lukáš Hrázký  wrote:
> 
> On Fri, 2018-02-16 at 17:59 +0100, Christophe de Dinechin wrote:
>>> 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");
> 
> Braces around if body (according to the coding style). Repeated a few
> times below.
> 
 +}
 +~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.
> 
> I'd also rather not see it, but if it's removed later, no big deal.

So I feel the general agreement is that it’s better to add an explicit getter 
and use that rather than implicitly convert.

> 
>>> 
 +
 +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.
> 
> It is not, actually, disabled here. It gets disabled later when you add
> the mutex member, as that is a non-copyable type.

You are right, of course. But like for the above, the idea is to minimize 
“noise” in the series.

Of course, I could explicitly delete constructors and assignment. I feel, but I 
may be wrong, that having a mutex field “says that”, both in terms of language 
safety and in terms of “what it means” (i.e. you are really creating a class 
for the purpose of RAII and thread safety).

If you think it’s better to add a few lines, I’m OK with it, but I don’t like 
it much as I read it more as noise than anything else. There are a few choices 
here, roughly in my personal order of preference:
1. Introduce = delete in this patch, remove it when mutex is added
2. Introduce = delete in this patch and leave it there even when it becomes 
“useless"
3 Leave a temporary “unsafe” state for a couple of commits
4. Merge the two and make sure there’s the mutex at the beginning

As a side note, this reminds me of a talk I highly recommend, 
https://www.youtube.com/watch?v=4AfRAVcThyA in case you have not seen it. Among 
other things, Herb Sutter proposes a $class mechanism that makes it possible to 
define a class of classes, e.g. “interface”, so that you explain with $class 
interface { … } what an interface is, and then you can write “interface X { .. 
}” and have an interface class (e.g. only pure virtual methods, etc).

Here, I see the mutex as indicative of a “locked resource” class, which I 
understand as having only explicit ctors for initialization and nothing else to 
consider, no copy, no assignment, etc. But it could also be made explicit with 
this mechanism. Post C++ 20, so not right away…

> 
> It might be a good idea to explicitly delete the copy ctr/ao anyway...
> In case someone ever removes the mutex and doesn't realize this.

That’s my choice #2, it could easily become #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)
>>> 
>>> maybe would be better to use the 

Re: [Spice-devel] [PATCH spice-streaming-agent v2 4/5] Handle capabilities

[Christophe de Dinechin]

> On 20 Feb 2018, at 15:30, Frediano Ziglio  wrote:
> 
> Do not bail if the server is attempting to communicate some extensions
> but just ignore as at the moment we support none.
> 
> Signed-off-by: Frediano Ziglio 
> ---
> src/spice-streaming-agent.cpp | 31 +++
> 1 file changed, 31 insertions(+)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index d72c30b..9547e49 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -40,6 +40,8 @@
> 
> using namespace spice::streaming_agent;
> 
> +static size_t write_all(int fd, const void *buf, const size_t len);
> +
> static ConcreteAgent agent;
> 
> struct SpiceStreamFormatMessage
> @@ -77,6 +79,7 @@ static int have_something_to_read(int timeout)
> return 0;
> }
> 
> +static int read_stream_capabilities(uint32_t len);
> static int read_stream_start_stop(uint32_t len);
> 
> static int read_command_from_device(void)
> @@ -96,6 +99,8 @@ static int read_command_from_device(void)
> }
> 
> switch (hdr.type) {
> +case STREAM_TYPE_CAPABILITIES:
> +return read_stream_capabilities(hdr.size);
> case STREAM_TYPE_START_STOP:
> return read_stream_start_stop(hdr.size);
> }
> @@ -124,6 +129,32 @@ static int read_stream_start_stop(uint32_t len)
> return 1;
> }
> 
> +static int read_stream_capabilities(uint32_t len)

It says “read” but reads and writes.

Do you have many other pending patches that introduce new messages?

This messes with the outgoing message refactoring I have in flight, but also 
with the incoming refactoring that Lukas is looking at.

So my guess is this one should go first (it’s relatively small). Or if it’s OK 
to wait a little, be rewritten on top of my series, but I’m not too sure how to 
do that in the spirit of what Lukas wants to do for input refactoring.


> +{
> +uint8_t caps[STREAM_MSG_CAPABILITIES_MAX_BYTES];
> +
> +if (len > sizeof(caps)) {
> +throw std::runtime_error("capability message too long");
> +}
> +int n = read(streamfd, caps, len);
> +if (n != len) {
> +throw std::runtime_error("read command from device FAILED -- read " 
> + std::to_string(n) +
> + " expected " + std::to_string(len));
> +}
> +
> +// we currently do not suppor extensions so just reply so
> +StreamDevHeader hdr = {
> +STREAM_DEVICE_PROTOCOL,
> +0,
> +STREAM_TYPE_CAPABILITIES,
> +0
> +};
> +if (write_all(streamfd, , sizeof(hdr)) != sizeof(hdr)) {
> +throw std::runtime_error("error writing capabilities");
> +}
> +return 1;
> +}
> +
> static int read_command(bool blocking)
> {
> int timeout = blocking?-1:0;
> -- 
> 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-streaming-agent] Detect and handle exception creating capture engine

[Christophe de Dinechin]

> On 20 Feb 2018, at 18:05, Frediano Ziglio  wrote:
> 
>>> 
>>> On 20 Feb 2018, at 17:49, Frediano Ziglio  wrote:
>>> 
>>> Currently exception from a plugin are not handled when creating
>>> a capture engine.
>>> Capture the exception and try to use another plugin instead of
>>> bailing out.
>>> This was tested with an experimental GStreamer plugin.
>>> 
>>> Signed-off-by: Frediano Ziglio 
>>> ---
>>> src/concrete-agent.cpp | 9 -
>>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
>>> index 0720782..112ef93 100644
>>> --- a/src/concrete-agent.cpp
>>> +++ b/src/concrete-agent.cpp
>>> @@ -113,7 +113,14 @@ FrameCapture *ConcreteAgent::GetBestFrameCapture(const
>>> std::set>>// check client supports the codec
>>>if (codecs.find(plugin.second->VideoCodecType()) == codecs.end())
>>>continue;
>>> -FrameCapture *capture = plugin.second->CreateCapture();
>>> +
>>> +FrameCapture *capture;
>>> +try {
>>> +capture = plugin.second->CreateCapture();
>>> +} catch (const std::runtime_error ) {
>>> +syslog(LOG_ERR, "Error getting capture engine: %s",
>>> err.what());
>>> +continue;
>>> +}
>> 
>> I noticed that you usually capture std::runtime_error. Why not
>> std::exception? You still get “what”, and that lets you catch bad_alloc too.
>> 
>>>if (capture) {
>>>return capture;
>>>}
> 
> Lot of classes which inherit from std::exception are not normal program
> exceptions but hard cases (beside std::logic_error) which are hard to
> deal with so I prefer to limit to std::runtime_error.

I understand your rationale.

I the next iteration of my top-level cleanup, I plan to introduce a top-level 
catch for std::exception, where I think it makes sense (i.e we are exiting 
anyway). So what you are saying is that if there is a bad_allloc in 
CreateCapture, it’s unsafe to continue, we should throw and let the top-level 
catch clause deal with it, correct?

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

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

Re: [Spice-devel] [PATCH spice-streaming-agent] Detect and handle exception creating capture engine

[Frediano Ziglio]

> 
> > On 20 Feb 2018, at 17:49, Frediano Ziglio  wrote:
> > 
> > Currently exception from a plugin are not handled when creating
> > a capture engine.
> > Capture the exception and try to use another plugin instead of
> > bailing out.
> > This was tested with an experimental GStreamer plugin.
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> > src/concrete-agent.cpp | 9 -
> > 1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
> > index 0720782..112ef93 100644
> > --- a/src/concrete-agent.cpp
> > +++ b/src/concrete-agent.cpp
> > @@ -113,7 +113,14 @@ FrameCapture *ConcreteAgent::GetBestFrameCapture(const
> > std::set > // check client supports the codec
> > if (codecs.find(plugin.second->VideoCodecType()) == codecs.end())
> > continue;
> > -FrameCapture *capture = plugin.second->CreateCapture();
> > +
> > +FrameCapture *capture;
> > +try {
> > +capture = plugin.second->CreateCapture();
> > +} catch (const std::runtime_error ) {
> > +syslog(LOG_ERR, "Error getting capture engine: %s",
> > err.what());
> > +continue;
> > +}
> 
> I noticed that you usually capture std::runtime_error. Why not
> std::exception? You still get “what”, and that lets you catch bad_alloc too.
> 
> > if (capture) {
> > return capture;
> > }

Lot of classes which inherit from std::exception are not normal program
exceptions but hard cases (beside std::logic_error) which are hard to
deal with so I prefer to limit to std::runtime_error.

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

Re: [Spice-devel] [PATCH spice-streaming-agent] Detect and handle exception creating capture engine

[Christophe de Dinechin]

> On 20 Feb 2018, at 17:49, Frediano Ziglio  wrote:
> 
> Currently exception from a plugin are not handled when creating
> a capture engine.
> Capture the exception and try to use another plugin instead of
> bailing out.
> This was tested with an experimental GStreamer plugin.
> 
> Signed-off-by: Frediano Ziglio 
> ---
> src/concrete-agent.cpp | 9 -
> 1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
> index 0720782..112ef93 100644
> --- a/src/concrete-agent.cpp
> +++ b/src/concrete-agent.cpp
> @@ -113,7 +113,14 @@ FrameCapture *ConcreteAgent::GetBestFrameCapture(const 
> std::set // check client supports the codec
> if (codecs.find(plugin.second->VideoCodecType()) == codecs.end())
> continue;
> -FrameCapture *capture = plugin.second->CreateCapture();
> +
> +FrameCapture *capture;
> +try {
> +capture = plugin.second->CreateCapture();
> +} catch (const std::runtime_error ) {
> +syslog(LOG_ERR, "Error getting capture engine: %s", err.what());
> +continue;
> +}

I noticed that you usually capture std::runtime_error. Why not std::exception? 
You still get “what”, and that lets you catch bad_alloc too.

> if (capture) {
> return capture;
> }
> -- 
> 2.14.3
> 
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Spice-devel] [PATCH spice-streaming-agent] Detect and handle exception creating capture engine

[Frediano Ziglio]

Currently exception from a plugin are not handled when creating
a capture engine.
Capture the exception and try to use another plugin instead of
bailing out.
This was tested with an experimental GStreamer plugin.

Signed-off-by: Frediano Ziglio 
---
 src/concrete-agent.cpp | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/concrete-agent.cpp b/src/concrete-agent.cpp
index 0720782..112ef93 100644
--- a/src/concrete-agent.cpp
+++ b/src/concrete-agent.cpp
@@ -113,7 +113,14 @@ FrameCapture *ConcreteAgent::GetBestFrameCapture(const 
std::setVideoCodecType()) == codecs.end())
 continue;
-FrameCapture *capture = plugin.second->CreateCapture();
+
+FrameCapture *capture;
+try {
+capture = plugin.second->CreateCapture();
+} catch (const std::runtime_error ) {
+syslog(LOG_ERR, "Error getting capture engine: %s", err.what());
+continue;
+}
 if (capture) {
 return capture;
 }
-- 
2.14.3

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

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

[Christophe de Dinechin]

> On 20 Feb 2018, at 16:51, Lukáš Hrázký  wrote:
> 
> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
>> 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");
> 
> How about we leave this patch out and do it properly straight away? I can do 
> it…

I don’t mind, but maybe it’s still enough progress that we keep it, and then we 
have all your refactoring (including input messages) in one place?

(Here, I’m concerned about tracing the evolution of the code. The step I did 
seems obvious from the existing code. Maybe yours is too? Not having looked at 
it yet, I don’t know)

I don’t mind either way.

> 
> Lukas
> ___
> 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 12/17] Convert message writing from C style to C++ style

[Frediano Ziglio]

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

style, no column indentation.

> +};
> +
> +
> +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);

I think this is wrong as contains also the size of "pixels"
smart pointer.
Maybe you mean sizeof(Message) ?

> +}
> +static StreamMsgCursorSet make(XFixesCursorImage 

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

[Christophe de Dinechin]

> On 20 Feb 2018, at 16:53, Lukáš Hrázký  wrote:
> 
> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
>> 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)
> 
> Should be “capture_loop”

Yes, but this is one case where local consistency trumped the new style guide 
:-)

> 

>> {
>> 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());

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

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

[Lukáš Hrázký]

On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> 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)

Should be "capture_loop"

>  {
>  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());
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Lukáš Hrázký]

On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> 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");

How about we leave this patch out and do it properly straight away? I
can do it...

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

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

[Lukáš Hrázký]

On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> 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;
> +};

I would still very much like this class to be a functor that is passed
to a thread. It IMHO makes for a better encapsulation. You can then
launch it any way you like, for example on the main thread in a test.

FWIW, I came up with the name CursorUpdater :) (since if it's not a
thread we can't call it so)

> +
> +
> +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 ) {
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Christophe Fergeau]

On Tue, Feb 20, 2018 at 12:48:48PM +0100, Victor Toso wrote:
> On Thu, Jan 18, 2018 at 10:31:26AM +0100, Christophe Fergeau wrote:
> > At least on X.org, malicious code could run the equivalent of "watch
> > xsel -o --clipboard" in a VM, and would then be able to track all the
> > clipboard content, even when the spice-gtk widget is not focused.
> > 
> > At the moment, applications call spice_set_session_option(), and then
> > set SpiceGtkSession::auto-clipboard to TRUE (or to its saved state).
> > This commit adds a --spice-disable-clipboard option, and if it's set,
> > SpiceGtkSession::auto-clipboard will not be changeable and will always
> > be FALSE.
> > The only side effect I noticed is that enabling "clipboard sharing" in
> > GNOME Boxes VM preferences will appear to work, but will not enable
> > clipboard, and will be reset to off next time the preferences dialog is
> > open.
> 
> You mean running gnome-boxes --spice-disable-clipboard still
> shows clipboard enabled? Any bug filed already? If yes, I think
> we could add to the commit log.

There's a "clipboard sharing enabled" GObject property on
some spice-gtk object. Boxes uses it to enable/disable clipboard
sharing.
Now if you use --spice-disable-clipboard, this property is always going
to be disabled even if Boxes tries to set it to TRUE. But Boxes is not
aware of that, so when clicks on the checkbox in the UI, it appears to
be toggled, but if you close the dialog and come back, then it will be
shown as disabled again.
I haven't filed yet a Boxes bug for that.

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 2/6] Separate ERROR macro in a different utility header

[Christophe de Dinechin]

> On 20 Feb 2018, at 15:57, Lukáš Hrázký  wrote:
> 
> On Tue, 2018-02-20 at 15:48 +0100, Christophe de Dinechin wrote:
>>> On 20 Feb 2018, at 15:45, Lukáš Hrázký  wrote:
>>> 
>>> On Tue, 2018-02-20 at 15:07 +0100, Christophe de Dinechin wrote:
> On 19 Feb 2018, at 17:47, Frediano Ziglio  wrote:
> 
>> 
>> On Mon, 2018-02-19 at 15:52 +, Frediano Ziglio wrote:
>>> Allows to reuse it.
>>> 
>>> Signed-off-by: Frediano Ziglio 
>>> ---
>>> src/Makefile.am|  1 +
>>> src/mjpeg-fallback.cpp |  7 +--
>>> src/utils.hpp  | 18 ++
>>> 3 files changed, 20 insertions(+), 6 deletions(-)
>>> create mode 100644 src/utils.hpp
>>> 
>>> diff --git a/src/Makefile.am b/src/Makefile.am
>>> index 3717b5c..ba3b1bf 100644
>>> --- a/src/Makefile.am
>>> +++ b/src/Makefile.am
>>> @@ -55,4 +55,5 @@ spice_streaming_agent_SOURCES = \
>>> mjpeg-fallback.hpp \
>>> jpeg.cpp \
>>> jpeg.hpp \
>>> +   utils.hpp \
>>> $(NULL)
>>> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
>>> index cf704c6..0f31834 100644
>>> --- a/src/mjpeg-fallback.cpp
>>> +++ b/src/mjpeg-fallback.cpp
>>> @@ -6,6 +6,7 @@
>>> 
>>> #include 
>>> #include "mjpeg-fallback.hpp"
>>> +#include "utils.hpp"
>>> 
>>> #include 
>>> #include 
>>> @@ -19,12 +20,6 @@
>>> 
>>> using namespace spice::streaming_agent;
>>> 
>>> -#define ERROR(args) do { \
>>> -std::ostringstream _s; \
>>> -_s << args; \
>>> -throw std::runtime_error(_s.str()); \
>>> -} while(0)
>>> -
>>> static inline uint64_t get_time()
>>> {
>>>   timespec now;
>>> diff --git a/src/utils.hpp b/src/utils.hpp
>>> new file mode 100644
>>> index 000..1e43eff
>>> --- /dev/null
>>> +++ b/src/utils.hpp
>>> @@ -0,0 +1,18 @@
>>> +/* Miscellaneous utilities
>>> + *
>>> + * \copyright
>>> + * Copyright 2018 Red Hat Inc. All rights reserved.
>>> + */
>>> +#ifndef SPICE_STREAMING_AGENT_UTILS_HPP
>>> +#define SPICE_STREAMING_AGENT_UTILS_HPP
>>> +
>>> +#include 
>>> +#include 
>>> +
>>> +#define ERROR(args) do { \
>>> +std::ostringstream _s; \
>>> +_s << args; \
>>> +throw std::runtime_error(_s.str()); \
>>> +} while(0)
>>> +
>>> +#endif // SPICE_STREAMING_AGENT_UTILS_HPP
>> 
>> Can we please not do this :) It isn't such a chore to throw the
>> exceptions directly. This adds a level of indirection (code-wise) and
>> macros are (personal opinion alert) best avoided in C++ unless you
>> absolutely have to...
>> 
>> Lukas
>> 
> 
> Yes, I'm taking to much shortcuts. I was considering a format library like
> https://github.com/fmtlib/fmt. Did you even used a format library?
>>> 
>>> Forgot to reply... I haven't used it, and as Christophe, I also
>>> probably wouldn't, though maybe for different reasons :)
>>> 
 No, I would not do that.
 
 When you are about to throw an exception, you should probably avoid 
 anything that may fail in a different way, e.g. run out of memory. This is 
 the reason std::runtime_error::what returns a const char * and not a 
 string.
 
 If you want to do some formatting, may I suggest deferred formatting, 
 which addresses that problem. In other words, instead of:
 
throw std::runtime_error(std::string(“Flobnic error “) + 
 std::to_string(error_id) + “ when doing “ + operation”);
 
 consider:
 
struct flobnic_error : std::runtime_error
{
flobnic_error(const char *message, unsigned error_id, const 
 char *operation)
: std::runtime_error(message), error_id(error_id), 
 operation(operation) {}
// Optional
const char *what() { /* format here, but risky */ }
unsigned error_id;
const char *operation;
}
 
 and then whoever catches can then properly filter errors:
 
catch(flobnic_error ) {
// Emit message here that can use error_id and operation
// If you defined your own “what”, you may use it here
// (and need to decide where the formatted “what” was built
// i.e. whether you need to free it)
}
 
 It’s a little more work, but it:
 - Passes more information around (both exception type and the original 
 arguments
 - Avoids additional runtime errors that might arise when formatting before 
 throwing.
 
 
 Does that make sense?
>>> 
>>> I don't think this is practical... For some cases, it does make sense
>>> to create exception classes, but in general, we have some many
>>> 

Re: [Spice-devel] [PATCH spice-streaming-agent 2/6] Separate ERROR macro in a different utility header

[Christophe de Dinechin]

> On 20 Feb 2018, at 15:48, Christophe de Dinechin  wrote:
> 
> 
> 
>> On 20 Feb 2018, at 15:45, Lukáš Hrázký  wrote:
>> 
>> On Tue, 2018-02-20 at 15:07 +0100, Christophe de Dinechin wrote:
 On 19 Feb 2018, at 17:47, Frediano Ziglio  wrote:
 
> 
> On Mon, 2018-02-19 at 15:52 +, Frediano Ziglio wrote:
>> Allows to reuse it.
>> 
>> Signed-off-by: Frediano Ziglio 
>> ---
>> src/Makefile.am|  1 +
>> src/mjpeg-fallback.cpp |  7 +--
>> src/utils.hpp  | 18 ++
>> 3 files changed, 20 insertions(+), 6 deletions(-)
>> create mode 100644 src/utils.hpp
>> 
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index 3717b5c..ba3b1bf 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -55,4 +55,5 @@ spice_streaming_agent_SOURCES = \
>>  mjpeg-fallback.hpp \
>>  jpeg.cpp \
>>  jpeg.hpp \
>> +utils.hpp \
>>  $(NULL)
>> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
>> index cf704c6..0f31834 100644
>> --- a/src/mjpeg-fallback.cpp
>> +++ b/src/mjpeg-fallback.cpp
>> @@ -6,6 +6,7 @@
>> 
>> #include 
>> #include "mjpeg-fallback.hpp"
>> +#include "utils.hpp"
>> 
>> #include 
>> #include 
>> @@ -19,12 +20,6 @@
>> 
>> using namespace spice::streaming_agent;
>> 
>> -#define ERROR(args) do { \
>> -std::ostringstream _s; \
>> -_s << args; \
>> -throw std::runtime_error(_s.str()); \
>> -} while(0)
>> -
>> static inline uint64_t get_time()
>> {
>>   timespec now;
>> diff --git a/src/utils.hpp b/src/utils.hpp
>> new file mode 100644
>> index 000..1e43eff
>> --- /dev/null
>> +++ b/src/utils.hpp
>> @@ -0,0 +1,18 @@
>> +/* Miscellaneous utilities
>> + *
>> + * \copyright
>> + * Copyright 2018 Red Hat Inc. All rights reserved.
>> + */
>> +#ifndef SPICE_STREAMING_AGENT_UTILS_HPP
>> +#define SPICE_STREAMING_AGENT_UTILS_HPP
>> +
>> +#include 
>> +#include 
>> +
>> +#define ERROR(args) do { \
>> +std::ostringstream _s; \
>> +_s << args; \
>> +throw std::runtime_error(_s.str()); \
>> +} while(0)
>> +
>> +#endif // SPICE_STREAMING_AGENT_UTILS_HPP
> 
> Can we please not do this :) It isn't such a chore to throw the
> exceptions directly. This adds a level of indirection (code-wise) and
> macros are (personal opinion alert) best avoided in C++ unless you
> absolutely have to...
> 
> Lukas
> 
 
 Yes, I'm taking to much shortcuts. I was considering a format library like
 https://github.com/fmtlib/fmt. Did you even used a format library?
>> 
>> Forgot to reply... I haven't used it, and as Christophe, I also
>> probably wouldn't, though maybe for different reasons :)
>> 
>>> No, I would not do that.
>>> 
>>> When you are about to throw an exception, you should probably avoid 
>>> anything that may fail in a different way, e.g. run out of memory. This is 
>>> the reason std::runtime_error::what returns a const char * and not a string.
>>> 
>>> If you want to do some formatting, may I suggest deferred formatting, which 
>>> addresses that problem. In other words, instead of:
>>> 
>>> throw std::runtime_error(std::string(“Flobnic error “) + 
>>> std::to_string(error_id) + “ when doing “ + operation”);
>>> 
>>> consider:
>>> 
>>> struct flobnic_error : std::runtime_error
>>> {
>>> flobnic_error(const char *message, unsigned error_id, const 
>>> char *operation)
>>> : std::runtime_error(message), error_id(error_id), 
>>> operation(operation) {}
>>> // Optional
>>> const char *what() { /* format here, but risky */ }
>>> unsigned error_id;
>>> const char *operation;
>>> }
>>> 
>>> and then whoever catches can then properly filter errors:
>>> 
>>> catch(flobnic_error ) {
>>> // Emit message here that can use error_id and operation
>>> // If you defined your own “what”, you may use it here
>>> // (and need to decide where the formatted “what” was built
>>> // i.e. whether you need to free it)
>>> }
>>> 
>>> It’s a little more work, but it:
>>> - Passes more information around (both exception type and the original 
>>> arguments
>>> - Avoids additional runtime errors that might arise when formatting before 
>>> throwing.
>>> 
>>> 
>>> Does that make sense?
>> 
>> I don't think this is practical... For some cases, it does make sense
>> to create exception classes, but in general, we have some many
>> different errors throughout the code that creating exceptions for each
>> of them indeed doesn't make any sense :)
> 
> You really only need one class, let’s call it ’SpiceError’, and a 
> discriminating 

Re: [Spice-devel] [PATCH spice-streaming-agent 2/6] Separate ERROR macro in a different utility header

[Lukáš Hrázký]

On Tue, 2018-02-20 at 15:48 +0100, Christophe de Dinechin wrote:
> > On 20 Feb 2018, at 15:45, Lukáš Hrázký  wrote:
> > 
> > On Tue, 2018-02-20 at 15:07 +0100, Christophe de Dinechin wrote:
> > > > On 19 Feb 2018, at 17:47, Frediano Ziglio  wrote:
> > > > 
> > > > > 
> > > > > On Mon, 2018-02-19 at 15:52 +, Frediano Ziglio wrote:
> > > > > > Allows to reuse it.
> > > > > > 
> > > > > > Signed-off-by: Frediano Ziglio 
> > > > > > ---
> > > > > > src/Makefile.am|  1 +
> > > > > > src/mjpeg-fallback.cpp |  7 +--
> > > > > > src/utils.hpp  | 18 ++
> > > > > > 3 files changed, 20 insertions(+), 6 deletions(-)
> > > > > > create mode 100644 src/utils.hpp
> > > > > > 
> > > > > > diff --git a/src/Makefile.am b/src/Makefile.am
> > > > > > index 3717b5c..ba3b1bf 100644
> > > > > > --- a/src/Makefile.am
> > > > > > +++ b/src/Makefile.am
> > > > > > @@ -55,4 +55,5 @@ spice_streaming_agent_SOURCES = \
> > > > > > mjpeg-fallback.hpp \
> > > > > > jpeg.cpp \
> > > > > > jpeg.hpp \
> > > > > > +   utils.hpp \
> > > > > > $(NULL)
> > > > > > diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> > > > > > index cf704c6..0f31834 100644
> > > > > > --- a/src/mjpeg-fallback.cpp
> > > > > > +++ b/src/mjpeg-fallback.cpp
> > > > > > @@ -6,6 +6,7 @@
> > > > > > 
> > > > > > #include 
> > > > > > #include "mjpeg-fallback.hpp"
> > > > > > +#include "utils.hpp"
> > > > > > 
> > > > > > #include 
> > > > > > #include 
> > > > > > @@ -19,12 +20,6 @@
> > > > > > 
> > > > > > using namespace spice::streaming_agent;
> > > > > > 
> > > > > > -#define ERROR(args) do { \
> > > > > > -std::ostringstream _s; \
> > > > > > -_s << args; \
> > > > > > -throw std::runtime_error(_s.str()); \
> > > > > > -} while(0)
> > > > > > -
> > > > > > static inline uint64_t get_time()
> > > > > > {
> > > > > >timespec now;
> > > > > > diff --git a/src/utils.hpp b/src/utils.hpp
> > > > > > new file mode 100644
> > > > > > index 000..1e43eff
> > > > > > --- /dev/null
> > > > > > +++ b/src/utils.hpp
> > > > > > @@ -0,0 +1,18 @@
> > > > > > +/* Miscellaneous utilities
> > > > > > + *
> > > > > > + * \copyright
> > > > > > + * Copyright 2018 Red Hat Inc. All rights reserved.
> > > > > > + */
> > > > > > +#ifndef SPICE_STREAMING_AGENT_UTILS_HPP
> > > > > > +#define SPICE_STREAMING_AGENT_UTILS_HPP
> > > > > > +
> > > > > > +#include 
> > > > > > +#include 
> > > > > > +
> > > > > > +#define ERROR(args) do { \
> > > > > > +std::ostringstream _s; \
> > > > > > +_s << args; \
> > > > > > +throw std::runtime_error(_s.str()); \
> > > > > > +} while(0)
> > > > > > +
> > > > > > +#endif // SPICE_STREAMING_AGENT_UTILS_HPP
> > > > > 
> > > > > Can we please not do this :) It isn't such a chore to throw the
> > > > > exceptions directly. This adds a level of indirection (code-wise) and
> > > > > macros are (personal opinion alert) best avoided in C++ unless you
> > > > > absolutely have to...
> > > > > 
> > > > > Lukas
> > > > > 
> > > > 
> > > > Yes, I'm taking to much shortcuts. I was considering a format library 
> > > > like
> > > > https://github.com/fmtlib/fmt. Did you even used a format library?
> > 
> > Forgot to reply... I haven't used it, and as Christophe, I also
> > probably wouldn't, though maybe for different reasons :)
> > 
> > > No, I would not do that.
> > > 
> > > When you are about to throw an exception, you should probably avoid 
> > > anything that may fail in a different way, e.g. run out of memory. This 
> > > is the reason std::runtime_error::what returns a const char * and not a 
> > > string.
> > > 
> > > If you want to do some formatting, may I suggest deferred formatting, 
> > > which addresses that problem. In other words, instead of:
> > > 
> > >   throw std::runtime_error(std::string(“Flobnic error “) + 
> > > std::to_string(error_id) + “ when doing “ + operation”);
> > > 
> > > consider:
> > > 
> > >   struct flobnic_error : std::runtime_error
> > >   {
> > >   flobnic_error(const char *message, unsigned error_id, const 
> > > char *operation)
> > >   : std::runtime_error(message), error_id(error_id), 
> > > operation(operation) {}
> > >   // Optional
> > >   const char *what() { /* format here, but risky */ }
> > >   unsigned error_id;
> > >   const char *operation;
> > >   }
> > > 
> > > and then whoever catches can then properly filter errors:
> > > 
> > >   catch(flobnic_error ) {
> > >   // Emit message here that can use error_id and operation
> > >   // If you defined your own “what”, you may use it here
> > >   // (and need to decide where the formatted “what” was built
> > >   // i.e. whether you need to free it)
> > >   }
> > > 
> > > It’s a little more work, but it:
> > > - Passes more information around (both exception type and the original 
> > > arguments
> > > - Avoids additional runtime 

Re: [Spice-devel] [PATCH spice-streaming-agent 2/6] Separate ERROR macro in a different utility header

[Christophe de Dinechin]

> On 20 Feb 2018, at 15:45, Lukáš Hrázký  wrote:
> 
> On Tue, 2018-02-20 at 15:07 +0100, Christophe de Dinechin wrote:
>>> On 19 Feb 2018, at 17:47, Frediano Ziglio  wrote:
>>> 
 
 On Mon, 2018-02-19 at 15:52 +, Frediano Ziglio wrote:
> Allows to reuse it.
> 
> Signed-off-by: Frediano Ziglio 
> ---
> src/Makefile.am|  1 +
> src/mjpeg-fallback.cpp |  7 +--
> src/utils.hpp  | 18 ++
> 3 files changed, 20 insertions(+), 6 deletions(-)
> create mode 100644 src/utils.hpp
> 
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 3717b5c..ba3b1bf 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -55,4 +55,5 @@ spice_streaming_agent_SOURCES = \
>   mjpeg-fallback.hpp \
>   jpeg.cpp \
>   jpeg.hpp \
> + utils.hpp \
>   $(NULL)
> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> index cf704c6..0f31834 100644
> --- a/src/mjpeg-fallback.cpp
> +++ b/src/mjpeg-fallback.cpp
> @@ -6,6 +6,7 @@
> 
> #include 
> #include "mjpeg-fallback.hpp"
> +#include "utils.hpp"
> 
> #include 
> #include 
> @@ -19,12 +20,6 @@
> 
> using namespace spice::streaming_agent;
> 
> -#define ERROR(args) do { \
> -std::ostringstream _s; \
> -_s << args; \
> -throw std::runtime_error(_s.str()); \
> -} while(0)
> -
> static inline uint64_t get_time()
> {
>timespec now;
> diff --git a/src/utils.hpp b/src/utils.hpp
> new file mode 100644
> index 000..1e43eff
> --- /dev/null
> +++ b/src/utils.hpp
> @@ -0,0 +1,18 @@
> +/* Miscellaneous utilities
> + *
> + * \copyright
> + * Copyright 2018 Red Hat Inc. All rights reserved.
> + */
> +#ifndef SPICE_STREAMING_AGENT_UTILS_HPP
> +#define SPICE_STREAMING_AGENT_UTILS_HPP
> +
> +#include 
> +#include 
> +
> +#define ERROR(args) do { \
> +std::ostringstream _s; \
> +_s << args; \
> +throw std::runtime_error(_s.str()); \
> +} while(0)
> +
> +#endif // SPICE_STREAMING_AGENT_UTILS_HPP
 
 Can we please not do this :) It isn't such a chore to throw the
 exceptions directly. This adds a level of indirection (code-wise) and
 macros are (personal opinion alert) best avoided in C++ unless you
 absolutely have to...
 
 Lukas
 
>>> 
>>> Yes, I'm taking to much shortcuts. I was considering a format library like
>>> https://github.com/fmtlib/fmt. Did you even used a format library?
> 
> Forgot to reply... I haven't used it, and as Christophe, I also
> probably wouldn't, though maybe for different reasons :)
> 
>> No, I would not do that.
>> 
>> When you are about to throw an exception, you should probably avoid anything 
>> that may fail in a different way, e.g. run out of memory. This is the reason 
>> std::runtime_error::what returns a const char * and not a string.
>> 
>> If you want to do some formatting, may I suggest deferred formatting, which 
>> addresses that problem. In other words, instead of:
>> 
>>  throw std::runtime_error(std::string(“Flobnic error “) + 
>> std::to_string(error_id) + “ when doing “ + operation”);
>> 
>> consider:
>> 
>>  struct flobnic_error : std::runtime_error
>>  {
>>  flobnic_error(const char *message, unsigned error_id, const 
>> char *operation)
>>  : std::runtime_error(message), error_id(error_id), 
>> operation(operation) {}
>>  // Optional
>>  const char *what() { /* format here, but risky */ }
>>  unsigned error_id;
>>  const char *operation;
>>  }
>> 
>> and then whoever catches can then properly filter errors:
>> 
>>  catch(flobnic_error ) {
>>  // Emit message here that can use error_id and operation
>>  // If you defined your own “what”, you may use it here
>>  // (and need to decide where the formatted “what” was built
>>  // i.e. whether you need to free it)
>>  }
>> 
>> It’s a little more work, but it:
>> - Passes more information around (both exception type and the original 
>> arguments
>> - Avoids additional runtime errors that might arise when formatting before 
>> throwing.
>> 
>> 
>> Does that make sense?
> 
> I don't think this is practical... For some cases, it does make sense
> to create exception classes, but in general, we have some many
> different errors throughout the code that creating exceptions for each
> of them indeed doesn't make any sense :)

You really only need one class, let’s call it ’SpiceError’, and a 
discriminating enum.

> 
> How do you propose in general to handle these arbitrary errors, for
> which we are throwing runtime_errors now? You may have noticed I've
> been replacing some old error handling with just what you described :)

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

[Christophe de Dinechin]

> On 20 Feb 2018, at 15:36, Lukáš Hrázký  wrote:
> 
> On Tue, 2018-02-20 at 15:22 +0100, Christophe de Dinechin wrote:
>>> On 19 Feb 2018, at 18:29, Lukáš Hrázký  wrote:
>>> 
>>> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
 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.
>>> 
>>> Christophe already knows, but in case anyone will benefit from this,
>>> it's the incomplete frame bug, fix should be on the ML here:
>>> 
>>> [Spice-devel] [PATCH spice-server 8/8] stream-channel: Send the full
>>> frame in a single message
>>> 
>>> Christophe, I've tested your patches, it's streaming, but the cursor
>>> disappears after a while, so something's not entirely right.
>> 
>> So there is probably an issue with the cursor message… I will look into it.
> 
> See my reply to 12/17 of the series, should probably be the issue.
> 
>> What guest type (e.g. f25, rhel…)? I’ve not seen that. I’ll try again and 
>> see if I can reproduce.
> 
> f27, should not be related.
> 
>>> 
 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
>>> 
>>> I'd like to point out the classes are not entirely isolated, there is
>>> still the static bool streaming_requested (besides the necessary
>>> quit_requested, although I have an idea for that), which ties together
>>> the ConcreteAgent::CaptureLoop and Stream::read_command_from_device.
>> 
>> Interesting. That will force me to re-review the whole thing, because I 
>> definitely remember moving streaming_requested within the Stream class, but 
>> I don’t see that anymore. Unless I ran into some issue and decided to 
>> postpone that?
> 
> Must've gotten lost... But the solution to put client_codecs and
> supposedly the streaming_requested is honestly ugly :)

Well, I really only put outfgoing messages in their own classes. Incoming 
messages remain to be done. That being said, the logic is really small, so I 
though that was OK for the moment.

> and seems like you started taking shortcuts :)

No, simply doing things in rough order of priorities.

> I was actually working on that part myself, so it turned out ok as I have 
> some rough solution, which I’ll need to incorporate with your design, if I 
> can manage that :)

Cool. So I won’t touch that part then.

> 
>>> 
>>> I have an unfinished patch for this, which will need heavy rebasing. In
>>> case you are/will be looking into it, Christophe, let me know.
>> 
>> My reading is that you have a patch for the coupled case (quit). Is it OK if 
>> I add one for streaming_requested to the next spin, or do you plan to do 
>> that?
> 
> I don't have the patch yet, just an idea for the quit. As for
> streaming_requested, see above. I'd like to do that, on top of your
> patches, once the dust settles a bit...
> 
>>> 
>>> Lukas
>>> 
 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 

Re: [Spice-devel] [PATCH spice-streaming-agent 2/6] Separate ERROR macro in a different utility header

[Lukáš Hrázký]

On Tue, 2018-02-20 at 15:07 +0100, Christophe de Dinechin wrote:
> > On 19 Feb 2018, at 17:47, Frediano Ziglio  wrote:
> > 
> > > 
> > > On Mon, 2018-02-19 at 15:52 +, Frediano Ziglio wrote:
> > > > Allows to reuse it.
> > > > 
> > > > Signed-off-by: Frediano Ziglio 
> > > > ---
> > > > src/Makefile.am|  1 +
> > > > src/mjpeg-fallback.cpp |  7 +--
> > > > src/utils.hpp  | 18 ++
> > > > 3 files changed, 20 insertions(+), 6 deletions(-)
> > > > create mode 100644 src/utils.hpp
> > > > 
> > > > diff --git a/src/Makefile.am b/src/Makefile.am
> > > > index 3717b5c..ba3b1bf 100644
> > > > --- a/src/Makefile.am
> > > > +++ b/src/Makefile.am
> > > > @@ -55,4 +55,5 @@ spice_streaming_agent_SOURCES = \
> > > > mjpeg-fallback.hpp \
> > > > jpeg.cpp \
> > > > jpeg.hpp \
> > > > +   utils.hpp \
> > > > $(NULL)
> > > > diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> > > > index cf704c6..0f31834 100644
> > > > --- a/src/mjpeg-fallback.cpp
> > > > +++ b/src/mjpeg-fallback.cpp
> > > > @@ -6,6 +6,7 @@
> > > > 
> > > > #include 
> > > > #include "mjpeg-fallback.hpp"
> > > > +#include "utils.hpp"
> > > > 
> > > > #include 
> > > > #include 
> > > > @@ -19,12 +20,6 @@
> > > > 
> > > > using namespace spice::streaming_agent;
> > > > 
> > > > -#define ERROR(args) do { \
> > > > -std::ostringstream _s; \
> > > > -_s << args; \
> > > > -throw std::runtime_error(_s.str()); \
> > > > -} while(0)
> > > > -
> > > > static inline uint64_t get_time()
> > > > {
> > > > timespec now;
> > > > diff --git a/src/utils.hpp b/src/utils.hpp
> > > > new file mode 100644
> > > > index 000..1e43eff
> > > > --- /dev/null
> > > > +++ b/src/utils.hpp
> > > > @@ -0,0 +1,18 @@
> > > > +/* Miscellaneous utilities
> > > > + *
> > > > + * \copyright
> > > > + * Copyright 2018 Red Hat Inc. All rights reserved.
> > > > + */
> > > > +#ifndef SPICE_STREAMING_AGENT_UTILS_HPP
> > > > +#define SPICE_STREAMING_AGENT_UTILS_HPP
> > > > +
> > > > +#include 
> > > > +#include 
> > > > +
> > > > +#define ERROR(args) do { \
> > > > +std::ostringstream _s; \
> > > > +_s << args; \
> > > > +throw std::runtime_error(_s.str()); \
> > > > +} while(0)
> > > > +
> > > > +#endif // SPICE_STREAMING_AGENT_UTILS_HPP
> > > 
> > > Can we please not do this :) It isn't such a chore to throw the
> > > exceptions directly. This adds a level of indirection (code-wise) and
> > > macros are (personal opinion alert) best avoided in C++ unless you
> > > absolutely have to...
> > > 
> > > Lukas
> > > 
> > 
> > Yes, I'm taking to much shortcuts. I was considering a format library like
> > https://github.com/fmtlib/fmt. Did you even used a format library?

Forgot to reply... I haven't used it, and as Christophe, I also
probably wouldn't, though maybe for different reasons :)

> No, I would not do that.
> 
> When you are about to throw an exception, you should probably avoid anything 
> that may fail in a different way, e.g. run out of memory. This is the reason 
> std::runtime_error::what returns a const char * and not a string.
> 
> If you want to do some formatting, may I suggest deferred formatting, which 
> addresses that problem. In other words, instead of:
> 
>   throw std::runtime_error(std::string(“Flobnic error “) + 
> std::to_string(error_id) + “ when doing “ + operation”);
> 
> consider:
> 
>   struct flobnic_error : std::runtime_error
>   {
>   flobnic_error(const char *message, unsigned error_id, const 
> char *operation)
>   : std::runtime_error(message), error_id(error_id), 
> operation(operation) {}
>   // Optional
>   const char *what() { /* format here, but risky */ }
>   unsigned error_id;
>   const char *operation;
>   }
> 
> and then whoever catches can then properly filter errors:
> 
>   catch(flobnic_error ) {
>   // Emit message here that can use error_id and operation
>   // If you defined your own “what”, you may use it here
>   // (and need to decide where the formatted “what” was built
>   // i.e. whether you need to free it)
>   }
> 
> It’s a little more work, but it:
> - Passes more information around (both exception type and the original 
> arguments
> - Avoids additional runtime errors that might arise when formatting before 
> throwing.
> 
> 
> Does that make sense?

I don't think this is practical... For some cases, it does make sense
to create exception classes, but in general, we have some many
different errors throughout the code that creating exceptions for each
of them indeed doesn't make any sense :)

How do you propose in general to handle these arbitrary errors, for
which we are throwing runtime_errors now? You may have noticed I've
been replacing some old error handling with just what you described :)
I've also never seen a problem with it, 

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

[Lukáš Hrázký]

On Tue, 2018-02-20 at 15:22 +0100, Christophe de Dinechin wrote:
> > On 19 Feb 2018, at 18:29, Lukáš Hrázký  wrote:
> > 
> > On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> > > 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.
> > 
> > Christophe already knows, but in case anyone will benefit from this,
> > it's the incomplete frame bug, fix should be on the ML here:
> > 
> > [Spice-devel] [PATCH spice-server 8/8] stream-channel: Send the full
> > frame in a single message
> > 
> > Christophe, I've tested your patches, it's streaming, but the cursor
> > disappears after a while, so something's not entirely right.
> 
> So there is probably an issue with the cursor message… I will look into it.

See my reply to 12/17 of the series, should probably be the issue.

> What guest type (e.g. f25, rhel…)? I’ve not seen that. I’ll try again and see 
> if I can reproduce.

f27, should not be related.

> > 
> > > 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
> > 
> > I'd like to point out the classes are not entirely isolated, there is
> > still the static bool streaming_requested (besides the necessary
> > quit_requested, although I have an idea for that), which ties together
> > the ConcreteAgent::CaptureLoop and Stream::read_command_from_device.
> 
> Interesting. That will force me to re-review the whole thing, because I 
> definitely remember moving streaming_requested within the Stream class, but I 
> don’t see that anymore. Unless I ran into some issue and decided to postpone 
> that?

Must've gotten lost... But the solution to put client_codecs and
supposedly the streaming_requested is honestly ugly :) and seems like
you started taking shortcuts :) I was actually working on that part
myself, so it turned out ok as I have some rough solution, which I'll
need to incorporate with your design, if I can manage that :)

> > 
> > I have an unfinished patch for this, which will need heavy rebasing. In
> > case you are/will be looking into it, Christophe, let me know.
> 
> My reading is that you have a patch for the coupled case (quit). Is it OK if 
> I add one for streaming_requested to the next spin, or do you plan to do that?

I don't have the patch yet, just an idea for the quit. As for
streaming_requested, see above. I'd like to do that, on top of your
patches, once the dust settles a bit...

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

[Spice-devel] [PATCH spice-streaming-agent v2 3/5] Separate handling start/stop message from server

[Frediano Ziglio]

Prepare to add support for other messages.

Signed-off-by: Frediano Ziglio 
---
 src/spice-streaming-agent.cpp | 26 ++
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 80e3408..d72c30b 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -77,10 +77,11 @@ static int have_something_to_read(int timeout)
 return 0;
 }
 
+static int read_stream_start_stop(uint32_t len);
+
 static int read_command_from_device(void)
 {
 StreamDevHeader hdr;
-uint8_t msg[64];
 int n;
 
 std::lock_guard stream_guard(stream_mtx);
@@ -93,17 +94,26 @@ static int read_command_from_device(void)
 throw std::runtime_error("BAD VERSION " + 
std::to_string(hdr.protocol_version) +
  " (expected is " + 
std::to_string(STREAM_DEVICE_PROTOCOL) + ")");
 }
-if (hdr.type != STREAM_TYPE_START_STOP) {
-throw std::runtime_error("UNKNOWN msg of type " + 
std::to_string(hdr.type));
+
+switch (hdr.type) {
+case STREAM_TYPE_START_STOP:
+return read_stream_start_stop(hdr.size);
 }
-if (hdr.size >= sizeof(msg)) {
-throw std::runtime_error("msg size (" + std::to_string(hdr.size) + ") 
is too long "
+throw std::runtime_error("UNKNOWN msg of type " + 
std::to_string(hdr.type));
+}
+
+static int read_stream_start_stop(uint32_t len)
+{
+uint8_t msg[256];
+
+if (len >= sizeof(msg)) {
+throw std::runtime_error("msg size (" + std::to_string(len) + ") is 
too long "
  "(longer than " + std::to_string(sizeof(msg)) 
+ ")");
 }
-n = read(streamfd, , hdr.size);
-if (n != hdr.size) {
+int n = read(streamfd, , len);
+if (n != len) {
 throw std::runtime_error("read command from device FAILED -- read " + 
std::to_string(n) +
- " expected " + std::to_string(hdr.size));
+ " expected " + std::to_string(len));
 }
 streaming_requested = (msg[0] != 0); /* num_codecs */
 syslog(LOG_INFO, "GOT START_STOP message -- request to %s streaming\n",
-- 
2.14.3

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

[Spice-devel] [PATCH spice-streaming-agent v2 0/5] Handle more completely device messages

[Frediano Ziglio]

Avoids losing sync with the device.
Handle some missing messages.

Changes since v1:
- avoid the usage of ERROR macro.

Frediano Ziglio (5):
  Be more restrictive about error handling
  Use exception handling data from streaming device
  Separate handling start/stop message from server
  Handle capabilities
  Stub to handle errors from server

 src/spice-streaming-agent.cpp | 87 ---
 1 file changed, 65 insertions(+), 22 deletions(-)

-- 
2.14.3

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

[Spice-devel] [PATCH spice-streaming-agent v2 4/5] Handle capabilities

[Frediano Ziglio]

Do not bail if the server is attempting to communicate some extensions
but just ignore as at the moment we support none.

Signed-off-by: Frediano Ziglio 
---
 src/spice-streaming-agent.cpp | 31 +++
 1 file changed, 31 insertions(+)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index d72c30b..9547e49 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -40,6 +40,8 @@
 
 using namespace spice::streaming_agent;
 
+static size_t write_all(int fd, const void *buf, const size_t len);
+
 static ConcreteAgent agent;
 
 struct SpiceStreamFormatMessage
@@ -77,6 +79,7 @@ static int have_something_to_read(int timeout)
 return 0;
 }
 
+static int read_stream_capabilities(uint32_t len);
 static int read_stream_start_stop(uint32_t len);
 
 static int read_command_from_device(void)
@@ -96,6 +99,8 @@ static int read_command_from_device(void)
 }
 
 switch (hdr.type) {
+case STREAM_TYPE_CAPABILITIES:
+return read_stream_capabilities(hdr.size);
 case STREAM_TYPE_START_STOP:
 return read_stream_start_stop(hdr.size);
 }
@@ -124,6 +129,32 @@ static int read_stream_start_stop(uint32_t len)
 return 1;
 }
 
+static int read_stream_capabilities(uint32_t len)
+{
+uint8_t caps[STREAM_MSG_CAPABILITIES_MAX_BYTES];
+
+if (len > sizeof(caps)) {
+throw std::runtime_error("capability message too long");
+}
+int n = read(streamfd, caps, len);
+if (n != len) {
+throw std::runtime_error("read command from device FAILED -- read " + 
std::to_string(n) +
+ " expected " + std::to_string(len));
+}
+
+// we currently do not suppor extensions so just reply so
+StreamDevHeader hdr = {
+STREAM_DEVICE_PROTOCOL,
+0,
+STREAM_TYPE_CAPABILITIES,
+0
+};
+if (write_all(streamfd, , sizeof(hdr)) != sizeof(hdr)) {
+throw std::runtime_error("error writing capabilities");
+}
+return 1;
+}
+
 static int read_command(bool blocking)
 {
 int timeout = blocking?-1:0;
-- 
2.14.3

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

[Spice-devel] [PATCH spice-streaming-agent v2 2/5] Use exception handling data from streaming device

[Frediano Ziglio]

In all paths errors from this function are treated like fatal
error, there's no need to handle all manually potentially
forgetting to handle errors.
Also avoid to deal directly with logging moving the responsibility
to other layers.

Signed-off-by: Frediano Ziglio 
---
 src/spice-streaming-agent.cpp | 26 +-
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 1f41a6f..80e3408 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -86,32 +86,24 @@ static int read_command_from_device(void)
 std::lock_guard stream_guard(stream_mtx);
 n = read(streamfd, , sizeof(hdr));
 if (n != sizeof(hdr)) {
-syslog(LOG_WARNING,
-   "read command from device FAILED -- read %d expected %lu\n",
-   n, sizeof(hdr));
-return -1;
+throw std::runtime_error("read command from device FAILED -- read " + 
std::to_string(n) +
+ " expected " + std::to_string(sizeof(hdr)));
 }
 if (hdr.protocol_version != STREAM_DEVICE_PROTOCOL) {
-syslog(LOG_WARNING, "BAD VERSION %d (expected is %d)\n", 
hdr.protocol_version,
-   STREAM_DEVICE_PROTOCOL);
-return -1;
+throw std::runtime_error("BAD VERSION " + 
std::to_string(hdr.protocol_version) +
+ " (expected is " + 
std::to_string(STREAM_DEVICE_PROTOCOL) + ")");
 }
 if (hdr.type != STREAM_TYPE_START_STOP) {
-syslog(LOG_WARNING, "UNKNOWN msg of type %d\n", hdr.type);
-return -1;
+throw std::runtime_error("UNKNOWN msg of type " + 
std::to_string(hdr.type));
 }
 if (hdr.size >= sizeof(msg)) {
-syslog(LOG_WARNING,
-   "msg size (%d) is too long (longer than %lu)\n",
-   hdr.size, sizeof(msg));
-return -1;
+throw std::runtime_error("msg size (" + std::to_string(hdr.size) + ") 
is too long "
+ "(longer than " + std::to_string(sizeof(msg)) 
+ ")");
 }
 n = read(streamfd, , hdr.size);
 if (n != hdr.size) {
-syslog(LOG_WARNING,
-   "read command from device FAILED -- read %d expected %d\n",
-   n, hdr.size);
-return -1;
+throw std::runtime_error("read command from device FAILED -- read " + 
std::to_string(n) +
+ " expected " + std::to_string(hdr.size));
 }
 streaming_requested = (msg[0] != 0); /* num_codecs */
 syslog(LOG_INFO, "GOT START_STOP message -- request to %s streaming\n",
-- 
2.14.3

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

[Spice-devel] [PATCH spice-streaming-agent v2 5/5] Stub to handle errors from server

[Frediano Ziglio]

Base error message handling.

Signed-off-by: Frediano Ziglio 
---
 src/spice-streaming-agent.cpp | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 9547e49..7ad8c42 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -81,6 +81,7 @@ static int have_something_to_read(int timeout)
 
 static int read_stream_capabilities(uint32_t len);
 static int read_stream_start_stop(uint32_t len);
+static int read_stream_error(uint32_t len);
 
 static int read_command_from_device(void)
 {
@@ -101,6 +102,8 @@ static int read_command_from_device(void)
 switch (hdr.type) {
 case STREAM_TYPE_CAPABILITIES:
 return read_stream_capabilities(hdr.size);
+case STREAM_TYPE_NOTIFY_ERROR:
+return read_stream_error(hdr.size);
 case STREAM_TYPE_START_STOP:
 return read_stream_start_stop(hdr.size);
 }
@@ -155,6 +158,13 @@ static int read_stream_capabilities(uint32_t len)
 return 1;
 }
 
+static int read_stream_error(uint32_t len)
+{
+// TODO read message and use it
+throw std::runtime_error("got an error message from server");
+return -1;
+}
+
 static int read_command(bool blocking)
 {
 int timeout = blocking?-1:0;
-- 
2.14.3

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

[Spice-devel] [PATCH spice-streaming-agent v2 1/5] Be more restrictive about error handling

[Frediano Ziglio]

There's no much point in ignoring the error if these errors cause the
communication to be out of sync.
Ignoring them just change the state to indeterminate.

Signed-off-by: Frediano Ziglio 
---
 src/spice-streaming-agent.cpp | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index 45a92b9..1f41a6f 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -94,17 +94,17 @@ static int read_command_from_device(void)
 if (hdr.protocol_version != STREAM_DEVICE_PROTOCOL) {
 syslog(LOG_WARNING, "BAD VERSION %d (expected is %d)\n", 
hdr.protocol_version,
STREAM_DEVICE_PROTOCOL);
-return 0; // return -1; -- fail over this ?
+return -1;
 }
 if (hdr.type != STREAM_TYPE_START_STOP) {
 syslog(LOG_WARNING, "UNKNOWN msg of type %d\n", hdr.type);
-return 0; // return -1;
+return -1;
 }
 if (hdr.size >= sizeof(msg)) {
 syslog(LOG_WARNING,
"msg size (%d) is too long (longer than %lu)\n",
hdr.size, sizeof(msg));
-return 0; // return -1;
+return -1;
 }
 n = read(streamfd, , hdr.size);
 if (n != hdr.size) {
-- 
2.14.3

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

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

[Lukáš Hrázký]

On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> 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.

All in all, a nice way to encapsulate the sending of messages. What I
worked on, actually, was the receiving of messages, as that is actually
more important for separating the code (as seen later in the problem
you had with client_codecs and streaming_requested static variables).

I am now wondering how to merge it with your changes and whether the
same Message class hierarchy could be used (without making a mess out
of it). We should discuss it later.

> 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;
> +}

Do you purposefully avoid throwing exceptions here, returning a bool?
The error and exception could actually be checked as low as at the end
of the write_all() method, avoiding all the latter size returning and
checking?

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

"Info" is quite an unimaginative name :) I understand it's the message
itself here and also find it curiously hard to come up with something
better :) "TheMessage" not very good, is it? Still better than "Info"?
Maybe a comment referencing the CRTP to help readers understand?

> +struct Message

Any reason to not stick with the "class" keyword instead of "struct"?

> +{
> +template 
> +Message(PayloadArgs... payload)

"PayloadArgs... payload_args", we have something called "Payload" here
too, getting a bit confusing.

> +: 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...))
> +{ }

I find it redundant that you pass the "payload..." (the args :)) to
Info::size() and Info::make() here and then also to Info::write() in
Stream::send(). In the three messages below, you also showcase three
distinct ways of serializing them, caused by this redundancy (I
understand it is partially given by the payload of the messages).

See comments under each class, which all relate to this (hope it's not
too confusing).

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

Could the type actually be part of the template? As in:
struct FormatMessage : Message

> +static size_t size(unsigned width, unsigned height, uint8_t codec)
> +{
> +return sizeof(FormatMessage);
> +}
> +static StreamMsgFormat make(unsigned w, 

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

[Christophe de Dinechin]

> On 19 Feb 2018, at 18:29, Lukáš Hrázký  wrote:
> 
> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
>> 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.
> 
> Christophe already knows, but in case anyone will benefit from this,
> it's the incomplete frame bug, fix should be on the ML here:
> 
> [Spice-devel] [PATCH spice-server 8/8] stream-channel: Send the full
> frame in a single message
> 
> Christophe, I've tested your patches, it's streaming, but the cursor
> disappears after a while, so something's not entirely right.

So there is probably an issue with the cursor message… I will look into it.

What guest type (e.g. f25, rhel…)? I’ve not seen that. I’ll try again and see 
if I can reproduce.

> 
>> 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
> 
> I'd like to point out the classes are not entirely isolated, there is
> still the static bool streaming_requested (besides the necessary
> quit_requested, although I have an idea for that), which ties together
> the ConcreteAgent::CaptureLoop and Stream::read_command_from_device.

Interesting. That will force me to re-review the whole thing, because I 
definitely remember moving streaming_requested within the Stream class, but I 
don’t see that anymore. Unless I ran into some issue and decided to postpone 
that?

> 
> I have an unfinished patch for this, which will need heavy rebasing. In
> case you are/will be looking into it, Christophe, let me know.

My reading is that you have a patch for the coupled case (quit). Is it OK if I 
add one for streaming_requested to the next spin, or do you plan to do that?

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

Re: [Spice-devel] [PATCH spice-streaming-agent 2/6] Separate ERROR macro in a different utility header

[Christophe de Dinechin]

> On 19 Feb 2018, at 17:47, Frediano Ziglio  wrote:
> 
>> 
>> On Mon, 2018-02-19 at 15:52 +, Frediano Ziglio wrote:
>>> Allows to reuse it.
>>> 
>>> Signed-off-by: Frediano Ziglio 
>>> ---
>>> src/Makefile.am|  1 +
>>> src/mjpeg-fallback.cpp |  7 +--
>>> src/utils.hpp  | 18 ++
>>> 3 files changed, 20 insertions(+), 6 deletions(-)
>>> create mode 100644 src/utils.hpp
>>> 
>>> diff --git a/src/Makefile.am b/src/Makefile.am
>>> index 3717b5c..ba3b1bf 100644
>>> --- a/src/Makefile.am
>>> +++ b/src/Makefile.am
>>> @@ -55,4 +55,5 @@ spice_streaming_agent_SOURCES = \
>>> mjpeg-fallback.hpp \
>>> jpeg.cpp \
>>> jpeg.hpp \
>>> +   utils.hpp \
>>> $(NULL)
>>> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
>>> index cf704c6..0f31834 100644
>>> --- a/src/mjpeg-fallback.cpp
>>> +++ b/src/mjpeg-fallback.cpp
>>> @@ -6,6 +6,7 @@
>>> 
>>> #include 
>>> #include "mjpeg-fallback.hpp"
>>> +#include "utils.hpp"
>>> 
>>> #include 
>>> #include 
>>> @@ -19,12 +20,6 @@
>>> 
>>> using namespace spice::streaming_agent;
>>> 
>>> -#define ERROR(args) do { \
>>> -std::ostringstream _s; \
>>> -_s << args; \
>>> -throw std::runtime_error(_s.str()); \
>>> -} while(0)
>>> -
>>> static inline uint64_t get_time()
>>> {
>>> timespec now;
>>> diff --git a/src/utils.hpp b/src/utils.hpp
>>> new file mode 100644
>>> index 000..1e43eff
>>> --- /dev/null
>>> +++ b/src/utils.hpp
>>> @@ -0,0 +1,18 @@
>>> +/* Miscellaneous utilities
>>> + *
>>> + * \copyright
>>> + * Copyright 2018 Red Hat Inc. All rights reserved.
>>> + */
>>> +#ifndef SPICE_STREAMING_AGENT_UTILS_HPP
>>> +#define SPICE_STREAMING_AGENT_UTILS_HPP
>>> +
>>> +#include 
>>> +#include 
>>> +
>>> +#define ERROR(args) do { \
>>> +std::ostringstream _s; \
>>> +_s << args; \
>>> +throw std::runtime_error(_s.str()); \
>>> +} while(0)
>>> +
>>> +#endif // SPICE_STREAMING_AGENT_UTILS_HPP
>> 
>> Can we please not do this :) It isn't such a chore to throw the
>> exceptions directly. This adds a level of indirection (code-wise) and
>> macros are (personal opinion alert) best avoided in C++ unless you
>> absolutely have to...
>> 
>> Lukas
>> 
> 
> Yes, I'm taking to much shortcuts. I was considering a format library like
> https://github.com/fmtlib/fmt. Did you even used a format library?

No, I would not do that.

When you are about to throw an exception, you should probably avoid anything 
that may fail in a different way, e.g. run out of memory. This is the reason 
std::runtime_error::what returns a const char * and not a string.

If you want to do some formatting, may I suggest deferred formatting, which 
addresses that problem. In other words, instead of:

throw std::runtime_error(std::string(“Flobnic error “) + 
std::to_string(error_id) + “ when doing “ + operation”);

consider:

struct flobnic_error : std::runtime_error
{
flobnic_error(const char *message, unsigned error_id, const 
char *operation)
: std::runtime_error(message), error_id(error_id), 
operation(operation) {}
// Optional
const char *what() { /* format here, but risky */ }
unsigned error_id;
const char *operation;
}

and then whoever catches can then properly filter errors:

catch(flobnic_error ) {
// Emit message here that can use error_id and operation
// If you defined your own “what”, you may use it here
// (and need to decide where the formatted “what” was built
// i.e. whether you need to free it)
}

It’s a little more work, but it:
- Passes more information around (both exception type and the original arguments
- Avoids additional runtime errors that might arise when formatting before 
throwing.


Does that make sense?


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

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

Re: [Spice-devel] [PATCH spice-streaming-agent] Remove usage of ERROR macro

[Frediano Ziglio]

> 
> > On 20 Feb 2018, at 13:13, Frediano Ziglio  wrote:
> > 
> > Use more simple syntax for throwing errors.
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> > src/mjpeg-fallback.cpp | 8 +---
> > 1 file changed, 1 insertion(+), 7 deletions(-)
> > 
> > diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> > index cf704c6..fd37167 100644
> > --- a/src/mjpeg-fallback.cpp
> > +++ b/src/mjpeg-fallback.cpp
> > @@ -19,12 +19,6 @@
> > 
> > using namespace spice::streaming_agent;
> > 
> > -#define ERROR(args) do { \
> > -std::ostringstream _s; \
> > -_s << args; \
> > -throw std::runtime_error(_s.str()); \
> > -} while(0)
> > -
> > static inline uint64_t get_time()
> > {
> > timespec now;
> > @@ -65,7 +59,7 @@ MjpegFrameCapture::MjpegFrameCapture(const MjpegSettings&
> > settings):
> > {
> > dpy = XOpenDisplay(NULL);
> > if (!dpy)
> > -ERROR("Unable to initialize X11");
> > +throw std::runtime_error("Unable to initialize X11”);
> 
> That reminds me of a question I had at some point… Do we need to address
> localization?
> 

We never speaks about so even if the answer is yes the priority
is for next century (very low).

> > }
> > 
> > MjpegFrameCapture::~MjpegFrameCapture()

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

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

[Lukáš Hrázký]

On Tue, 2018-02-20 at 10:47 +0100, Christophe de Dinechin wrote:
> > On 20 Feb 2018, at 10:43, Lukáš Hrázký  wrote:
> > 
> > On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> > > 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)
> > 
> > I would like to name the class something more descriptive for what it
> > is becoming. Class named Stream in a file named "stream.{cpp,hpp}"
> > could be almost anything.
> 
> But it’s not named “Stream”, it’s called “spice::streaming_agent::Stream” ;-)
> 
> I chose short names because I was in that namespace. Otherwise, I agree with 
> you.
> 
> Do you think that the name is still too vague even within the namespace?

I think so. Everything in streaming agent is in that namespace, even if
it wasn't, you know you're looking at the streaming agent code and
think of the types in that context. Stream is still a pretty generic
name, I suppose you could imagine a number of things under it.

So what is this class in our case. It handles the socket communication
over the streaming channel to the server. Is it accurate to call it a
channel here? If so, maybe StreamingChannel?

> > My best name is StreamDispatcher so far, not
> > entirely happy about it :)
> 
> 
> > 
> > > {
> > > -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);
> > 
> > This method could also use a better name. write_bytes()?
> > write_buffer()?
> 
> I intended to do a rename in a follow up. My current choice was 
> “write_packet”, because precisely, it’s not writing bytes or a buffer, it’s 
> making sure the whole packet gets sent.

What do you mean by packet here? Does it have a specific meaning in
this context? It just sends an array of binary data, doesn't it? Like
later on you use it to write the header and message body separately.

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

Re: [Spice-devel] [PATCH spice-streaming-agent] Remove usage of ERROR macro

[Christophe de Dinechin]

> On 20 Feb 2018, at 13:13, Frediano Ziglio  wrote:
> 
> Use more simple syntax for throwing errors.
> 
> Signed-off-by: Frediano Ziglio 
> ---
> src/mjpeg-fallback.cpp | 8 +---
> 1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> index cf704c6..fd37167 100644
> --- a/src/mjpeg-fallback.cpp
> +++ b/src/mjpeg-fallback.cpp
> @@ -19,12 +19,6 @@
> 
> using namespace spice::streaming_agent;
> 
> -#define ERROR(args) do { \
> -std::ostringstream _s; \
> -_s << args; \
> -throw std::runtime_error(_s.str()); \
> -} while(0)
> -
> static inline uint64_t get_time()
> {
> timespec now;
> @@ -65,7 +59,7 @@ MjpegFrameCapture::MjpegFrameCapture(const MjpegSettings& 
> settings):
> {
> dpy = XOpenDisplay(NULL);
> if (!dpy)
> -ERROR("Unable to initialize X11");
> +throw std::runtime_error("Unable to initialize X11”);

That reminds me of a question I had at some point… Do we need to address 
localization?

> }
> 
> MjpegFrameCapture::~MjpegFrameCapture()
> -- 
> 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-streaming-agent] Remove usage of ERROR macro

[Lukáš Hrázký]

On Tue, 2018-02-20 at 12:13 +, Frediano Ziglio wrote:
> Use more simple syntax for throwing errors.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  src/mjpeg-fallback.cpp | 8 +---
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> index cf704c6..fd37167 100644
> --- a/src/mjpeg-fallback.cpp
> +++ b/src/mjpeg-fallback.cpp
> @@ -19,12 +19,6 @@
>  
>  using namespace spice::streaming_agent;
>  
> -#define ERROR(args) do { \
> -std::ostringstream _s; \
> -_s << args; \
> -throw std::runtime_error(_s.str()); \
> -} while(0)
> -
>  static inline uint64_t get_time()
>  {
>  timespec now;
> @@ -65,7 +59,7 @@ MjpegFrameCapture::MjpegFrameCapture(const MjpegSettings& 
> settings):
>  {
>  dpy = XOpenDisplay(NULL);
>  if (!dpy)
> -ERROR("Unable to initialize X11");
> +throw std::runtime_error("Unable to initialize X11");
>  }
>  
>  MjpegFrameCapture::~MjpegFrameCapture()

Acked-by: Lukáš Hrázký 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Frediano Ziglio]

> 
> Hi,
> 
> On Tue, Feb 20, 2018 at 10:00:41AM +0100, Lukáš Hrázký wrote:
> > On Mon, 2018-02-19 at 21:19 +0200, Uri Lublin wrote:
> > > On 02/19/2018 06:47 PM, Lukáš Hrázký wrote:
> > > > On Mon, 2018-02-19 at 18:29 +0200, Uri Lublin wrote:
> > > > > On 02/14/2018 06:37 PM, Christophe Fergeau wrote:
> > > > > > On Wed, Feb 14, 2018 at 10:40:58AM -0500, Frediano Ziglio wrote:
> > > > > > > > 
> > > > > > > > > On 14 Feb 2018, at 13:34, Lukáš Hrázký 
> > > > > > > > > wrote:
> > > > > > > > > 
> > > > > > > > > Introduce a unit test framework (Catch) to the codebase and a
> > > > > > > > > simple
> > > > > > > > > unit test for parsing the options of the mjpeg plugin.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Lukáš Hrázký 
> > > > > > > > > ---
> > > > > > > > > configure.ac  |  3 ++
> > > > > > > > > src/mjpeg-fallback.cpp|  5 +++
> > > > > > > > > src/mjpeg-fallback.hpp|  1 +
> > > > > > > > > src/unittests/.gitignore  |  5 +--
> > > > > > > > > src/unittests/Makefile.am | 15 +
> > > > > > > > > src/unittests/test-mjpeg-fallback.cpp | 58
> > > > > > > > > +++
> > > > > > > > > 6 files changed, 85 insertions(+), 2 deletions(-)
> > > > > > > > > create mode 100644 src/unittests/test-mjpeg-fallback.cpp
> > > > > > > > > 
> > > > > > > > > diff --git a/configure.ac b/configure.ac
> > > > > > > > > index 8795dae..5aab662 100644
> > > > > > > > > --- a/configure.ac
> > > > > > > > > +++ b/configure.ac
> > > > > > > > > @@ -17,6 +17,7 @@ if test x"$ac_cv_prog_cc_c99" = xno; then
> > > > > > > > >   AC_MSG_ERROR([C99 compiler is required.])
> > > > > > > > > fi
> > > > > > > > > AC_PROG_CXX
> > > > > > > > > +AC_LANG(C++)
> > > > > > > > > AX_CXX_COMPILE_STDCXX_11
> > > > > > > > > AC_PROG_INSTALL
> > > > > > > > > AC_CANONICAL_HOST
> > > > > > > > > @@ -49,6 +50,8 @@ AC_CHECK_LIB(jpeg, jpeg_destroy_decompress,
> > > > > > > > >   AC_MSG_ERROR([libjpeg not found]))
> > > > > > > > > AC_SUBST(JPEG_LIBS)
> > > > > > > > > 
> > > > > > > > > +AC_CHECK_HEADER([catch/catch.hpp],,[AC_MSG_ERROR([Could not
> > > > > > > > > find Catch
> > > > > > > > > dependency header (catch/catch.hpp)])])
> > > > > > > > 
> > > > > > > > Instead of an error, shouldn’t we instead fallback to not
> > > > > > > > compiling the unit
> > > > > > > > tests? Possibly a warning?
> > > > > > > > 
> > > > > > > 
> > > > > > > Good point but I would suggest a follow up and an explicit
> > > > > > > --I-dont-really-want-unittests
> > > > > > > option, I agree people should run tests.
> > > > > > > Another follow up is a patch for the spec file.
> > > > > > 
> > > > > > Alternatively, this could go with a --with-catch flag (or
> > > > > > --enable-unittest or any names which fits you), and
> > > > > > 1) if there is nothing specified, then we silently enable/disable
> > > > > > tests
> > > > > > depending on the availability of the catch
> > > > > > 2) if --with-catch is specified, then we error out if it's not
> > > > > > found
> > > > > > 3) if --without-catch is used, then we never use it.
> > > > > > 
> > > > > > Then we add --with-catch to autogen.sh, and all developers will
> > > > > > have to
> > > > > > go through extra hoops not to use it.
> > > > > > 
> > > > > > Christophe
> > > > > > 
> > > > > 
> > > > > I agree that's a good alternative.
> > > > 
> > > > I prefer Frediano's variant. Nobody is forced to use autogen.sh and
> > > > also I don't think anybody would expect autogen.sh to modify the
> > > > default configure behaviour. I don't think it's a good idea.
> > > 
> > > If users prefer to not run autogen.sh that's ok.
> > > It provides defaults options for developers.
> > > For example, I do not expect users to run configure with
> > >   --enable-maintainer-mode too.
> > > Most users will use configure directly from the tarball.
> > > 
> > > Users can choose what options they want to enable/disable
> > > 
> > > > 
> > > > For example, on Gentoo, which doesn't care for autogen.sh and runs
> > > > autotools build automatically, the default behaviour (unless the person
> > > > writing the ebuild would notice) would be dependent on Catch being
> > > > present in the system. This can create subtle inconsistencies, which
> > > > aren't critical in this case, but still unnecessary.
> > > 
> > > So if on Gentoo (or another distribution) there exists no catch
> > > package, they are forced to either add this package or search
> > > for how to disable it.
> > 
> > Yes. The packager is forced to resolve the issue by either providing
> > the dependency or explicitly disabling the tests. In contrast to the
> > tests being "quietly" skipped, if Catch is not present, unless he would
> > notice it. If he isn't dilligent and doesn't notice, the behaviour of
> > the package will depend on the presence of Catch on the target system (
> > on Gentoo packages are compiled on 

[Spice-devel] [PATCH spice-streaming-agent] Remove usage of ERROR macro

[Frediano Ziglio]

Use more simple syntax for throwing errors.

Signed-off-by: Frediano Ziglio 
---
 src/mjpeg-fallback.cpp | 8 +---
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
index cf704c6..fd37167 100644
--- a/src/mjpeg-fallback.cpp
+++ b/src/mjpeg-fallback.cpp
@@ -19,12 +19,6 @@
 
 using namespace spice::streaming_agent;
 
-#define ERROR(args) do { \
-std::ostringstream _s; \
-_s << args; \
-throw std::runtime_error(_s.str()); \
-} while(0)
-
 static inline uint64_t get_time()
 {
 timespec now;
@@ -65,7 +59,7 @@ MjpegFrameCapture::MjpegFrameCapture(const MjpegSettings& 
settings):
 {
 dpy = XOpenDisplay(NULL);
 if (!dpy)
-ERROR("Unable to initialize X11");
+throw std::runtime_error("Unable to initialize X11");
 }
 
 MjpegFrameCapture::~MjpegFrameCapture()
-- 
2.14.3

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

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

[Victor Toso]

On Thu, Jan 18, 2018 at 10:31:26AM +0100, Christophe Fergeau wrote:
> At least on X.org, malicious code could run the equivalent of "watch
> xsel -o --clipboard" in a VM, and would then be able to track all the
> clipboard content, even when the spice-gtk widget is not focused.
> 
> At the moment, applications call spice_set_session_option(), and then
> set SpiceGtkSession::auto-clipboard to TRUE (or to its saved state).
> This commit adds a --spice-disable-clipboard option, and if it's set,
> SpiceGtkSession::auto-clipboard will not be changeable and will always
> be FALSE.
> The only side effect I noticed is that enabling "clipboard sharing" in
> GNOME Boxes VM preferences will appear to work, but will not enable
> clipboard, and will be reset to off next time the preferences dialog is
> open.

You mean running gnome-boxes --spice-disable-clipboard still
shows clipboard enabled? Any bug filed already? If yes, I think
we could add to the commit log.

> https://bugzilla.redhat.com/show_bug.cgi?id=1320263
> ---
>  man/spice-client.pod |  4 
>  src/spice-gtk-session.c  | 16 +++-
>  src/spice-option.c   |  6 ++
>  src/spice-session-priv.h |  1 +
>  src/spice-session.c  | 35 +++
>  5 files changed, 61 insertions(+), 1 deletion(-)
> 
> diff --git a/man/spice-client.pod b/man/spice-client.pod
> index 7288b84e..76a7d207 100644
> --- a/man/spice-client.pod
> +++ b/man/spice-client.pod
> @@ -111,6 +111,10 @@ SPICE_DEBUG environment variable, or using 
> G_MESSAGES_DEBUG=all
>  
>  Disable audio support
>  
> +=item --spice-disable-clipboard
> +
> +Disable clipboard sharing between client OS and guest OS
> +
>  =item --spice-disable-usbredir
>  
>  Disable USB redirection support
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index 31f60dc4..c3546209 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -309,6 +309,20 @@ static void spice_gtk_session_finalize(GObject *gobject)
>  G_OBJECT_CLASS(spice_gtk_session_parent_class)->finalize(gobject);
>  }
>  
> +static void spice_gtk_session_set_auto_clipboard(SpiceGtkSession *self,
> + gboolean auto_clipboard)
> +{
> +gboolean disable_clipboard;
> +g_object_get(self->priv->session,
> + "disable-clipboard", _clipboard,
> + NULL);
> +if (disable_clipboard && auto_clipboard) {
> +g_debug("not enabling clipboard sharing as it was disabled on the 
> command-line");
> +auto_clipboard = FALSE;
> +}
> +self->priv->auto_clipboard_enable = auto_clipboard;
> +}
> +
>  static void spice_gtk_session_get_property(GObject*gobject,
> guint   prop_id,
> GValue *value,
> @@ -352,7 +366,7 @@ static void spice_gtk_session_set_property(GObject  
> *gobject,
>  s->session = g_value_get_object(value);
>  break;
>  case PROP_AUTO_CLIPBOARD:
> -s->auto_clipboard_enable = g_value_get_boolean(value);
> +spice_gtk_session_set_auto_clipboard(self, 
> g_value_get_boolean(value));
>  break;
>  case PROP_AUTO_USBREDIR: {
>  SpiceDesktopIntegration *desktop_int;
> diff --git a/src/spice-option.c b/src/spice-option.c
> index 6b400bca..0f2f795d 100644
> --- a/src/spice-option.c
> +++ b/src/spice-option.c
> @@ -21,6 +21,7 @@
>  #include 
>  #include 
>  #include "spice-session.h"
> +#include "spice-session-priv.h"
>  #include "spice-util.h"
>  #include "spice-channel-priv.h"
>  #include "usb-device-manager.h"
> @@ -35,6 +36,7 @@ static char *usbredir_auto_redirect_filter = NULL;
>  static char *usbredir_redirect_on_connect = NULL;
>  static gboolean smartcard = FALSE;
>  static gboolean disable_audio = FALSE;
> +static gboolean disable_clipboard = FALSE;
>  static gboolean disable_usbredir = FALSE;
>  static gint cache_size = 0;
>  static gint glz_window_size = 0;
> @@ -203,6 +205,8 @@ GOptionGroup* spice_get_option_group(void)
>N_("Subject of the host certificate (field=value pairs separated 
> by commas)"), N_("") },
>  { "spice-disable-audio", '\0', 0, G_OPTION_ARG_NONE, _audio,
>N_("Disable audio support"), NULL },
> +{ "spice-disable-clipboard", '\0', 0, G_OPTION_ARG_NONE, 
> _clipboard,
> +  N_("Disable client/guest clipboard sharing"), NULL },
>  { "spice-smartcard", '\0', 0, G_OPTION_ARG_NONE, ,
>N_("Enable smartcard support"), NULL },
>  { "spice-smartcard-certificates", '\0', 0, G_OPTION_ARG_STRING, 
> _certificates,
> @@ -312,6 +316,8 @@ void spice_set_session_option(SpiceSession *session)
>  g_object_set(session, "enable-usbredir", FALSE, NULL);
>  if (disable_audio)
>  g_object_set(session, "enable-audio", FALSE, NULL);
> +if (disable_clipboard)
> +spice_session_set_disable_clipboard(session, TRUE);
>   

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

[Victor Toso]

On Tue, Feb 20, 2018 at 12:22:52PM +0100, Lukáš Hrázký wrote:
> On Tue, 2018-02-20 at 11:59 +0100, Victor Toso wrote:
> > On Tue, Feb 20, 2018 at 11:45:33AM +0100, Lukáš Hrázký wrote:
> > > > That tests are not enabled by default. If we enable it by default
> > > > here I would expect to do the same for other Spice components.
> > > 
> > > Ok, but I'm somewhat confused here. Enabling the tests is during
> > > packaging - you run `make check`. Which means we are talking about...
> > > Fedora packaging? Or am I getting something wrong?
> > 
> > I'm talking about releases and the configure script with default
> > options. Today we don't have tests enabled by default in our
> > tarballs.
> > 
> > 1) wget https://www.spice-space.org/download/gtk/spice-gtk-LATEST.tar.bz2
> > 2) tar -xvf spice-gtk-LATEST.tar.bz2
> > 3) cd spice-gtk-0.34
> > 4) ./configure
> > 5) make
> > # no tests, no extra dependencies.
> 
> Thanks for the example, made it clear. I think you are mistaken here,
> though:
> 
> wget https://www.spice-space.org/download/gtk/spice-gtk-LATEST.tar.bz2
> tar -xvf spice-gtk-LATEST.tar.bz2
> cd spice-gtk-0.34
> ./configure
> make check
> 
> (the only difference being calling `make check` at the end)
> 
> Here you have the tests.

For spice-common (submodule) not spice-gtk. For spice-gtk,
you need --enable-static and then, yes, make check will run tests
for it.

> the tests. Many packaging tools automatically detect there is a make
> check target and call it at the appropriate point during the packaging.
> 
> So you don't need to explicitly enable the tests in the tarball in any
> way. The only issue, which we are discussing here, are extra testing
> dependencies, that the tests may have and that you check for in
> configure.
> 
> So if someone builds the package and does not intend to run the tests,
> the configure script may still require him to install the tests
> dependencies.
> 
> And as I break it down here for myself as well, I think I was wrong
> with my previous argumentation for the different behaviour based on
> presence of Catch in the system. Because the make check target will be
> there anyway, it will just be broken if the dependency isn't there.
> Correct? So in the unlikely case I'm not missing anything anymore, I
> suppose Christophe's suggestion is ok. Sorry for the noise :)

Yes, warning is fine by me too.


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 07/17] Get rid of C-style memset initializations, use C++ style aggregates

[Frediano Ziglio]

> > On 20 Feb 2018, at 10:39, Lukáš Hrázký  wrote:
> > 
> > On Tue, 2018-02-20 at 10:18 +0100, Christophe de Dinechin wrote:
> >>> On Feb 19, 2018, at 7:19 PM, Lukáš Hrázký  wrote:
> >>> 
> >>> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
>  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 = {}
>  +};
> >>> 
> >>> So, someone should find out if we can use the designated initializers,
> >>> I suppose it depends on the compilers on all platforms we care about
> >>> supporting them?
> >>> 
> >>> I wasn't able to find much useful information so far. Anyone knows in
> >>> which version of gcc it was introduced?
> >> 
> >> My experience is that clang has no issue with it in any version.
> >> 
> >> GCC supports it in C++ since at least 4.8.5, as tested on RHEL 7.4 with
> >> the following short example:
> >> 
> >> struct Thing { int x; float y; };
> >> 
> >> int foo()
> >> {
> >>  Thing x = { x: 10, y:20 };
> >>  Thing y = { .x = 10, .y = 20 };
> >>  Thing z { 10, 20 };
> >>  Thing t { .x = 10, .y = 20 };
> >> }
> >> 
> >> It has, however, trouble with out-of-order initializations, with the same
> >> message in 4.8.5 as on Fedora 25 (6.4.1):
> >> 
> >> glop.cpp:9:30: sorry, unimplemented: non-trivial designated initializers
> >> not supported
> >>   Thing a { .y = 10, .x = 20 };
> >> 
> >> The “not implemented” message is a bit scary, but the fact that the code
> >> as written has been supported as far back as 4.8.5 makes me confident
> >> that we are not in some experimental section of the compiler.
> > 
> > I've found this on the matter:
> > 
> > https://gcc.gnu.org/ml/gcc/2014-09/msg00207.html
> > 
> > That doesn't give much confidence... Is this documented anywhere? I
> > mean I've only used Google and haven't found anything…
> 
> I think it’s a low priority thing because the “fix” in source code is so easy
> (reorder the fields), and the fix complicated. According to the message you
> referenced, presently, GCC just checks that the annotations match the names,
> but otherwise ignores them and proceeds like for a POD init. If it were to
> be 

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

[Lukáš Hrázký]

On Tue, 2018-02-20 at 11:59 +0100, Victor Toso wrote:
> On Tue, Feb 20, 2018 at 11:45:33AM +0100, Lukáš Hrázký wrote:
> > > That tests are not enabled by default. If we enable it by default
> > > here I would expect to do the same for other Spice components.
> > 
> > Ok, but I'm somewhat confused here. Enabling the tests is during
> > packaging - you run `make check`. Which means we are talking about...
> > Fedora packaging? Or am I getting something wrong?
> 
> I'm talking about releases and the configure script with default
> options. Today we don't have tests enabled by default in our
> tarballs.
> 
> 1) wget https://www.spice-space.org/download/gtk/spice-gtk-LATEST.tar.bz2
> 2) tar -xvf spice-gtk-LATEST.tar.bz2
> 3) cd spice-gtk-0.34
> 4) ./configure
> 5) make
> # no tests, no extra dependencies.

Thanks for the example, made it clear. I think you are mistaken here,
though:

wget https://www.spice-space.org/download/gtk/spice-gtk-LATEST.tar.bz2
tar -xvf spice-gtk-LATEST.tar.bz2
cd spice-gtk-0.34
./configure
make check

(the only difference being calling `make check` at the end)

Here you have the tests. It is up to the packaging to explicitly call
the tests. Many packaging tools automatically detect there is a make
check target and call it at the appropriate point during the packaging.

So you don't need to explicitly enable the tests in the tarball in any
way. The only issue, which we are discussing here, are extra testing
dependencies, that the tests may have and that you check for in
configure.

So if someone builds the package and does not intend to run the tests,
the configure script may still require him to install the tests
dependencies.

And as I break it down here for myself as well, I think I was wrong
with my previous argumentation for the different behaviour based on
presence of Catch in the system. Because the make check target will be
there anyway, it will just be broken if the dependency isn't there.
Correct? So in the unlikely case I'm not missing anything anymore, I
suppose Christophe's suggestion is ok. Sorry for the noise :)

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

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

[Christophe de Dinechin]

> On Feb 20, 2018, at 12:05 PM, Frediano Ziglio  wrote:
> 
>> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
>>> 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"
>> 
>> Acked-by: Lukáš Hrázký 
> 
> Hi,
>  question for the author, can this patch or others be
> merged separately?

Yes, why not?

(I don’t know if you are suggesting that I could / should have submitted that 
as separate patches?)

> 
> Frediano

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

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

[Frediano Ziglio]

> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> > 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"
> 
> Acked-by: Lukáš Hrázký 

Hi,
  question for the author, can this patch or others be
merged separately?

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

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

[Victor Toso]

On Tue, Feb 20, 2018 at 11:45:33AM +0100, Lukáš Hrázký wrote:
> > That tests are not enabled by default. If we enable it by default
> > here I would expect to do the same for other Spice components.
> 
> Ok, but I'm somewhat confused here. Enabling the tests is during
> packaging - you run `make check`. Which means we are talking about...
> Fedora packaging? Or am I getting something wrong?

I'm talking about releases and the configure script with default
options. Today we don't have tests enabled by default in our
tarballs.

1) wget https://www.spice-space.org/download/gtk/spice-gtk-LATEST.tar.bz2
2) tar -xvf spice-gtk-LATEST.tar.bz2
3) cd spice-gtk-0.34
4) ./configure
5) make
# no tests, no extra dependencies.


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 v3 3/3] mjpeg-fallback: unittest for the options parsing

[Lukáš Hrázký]

On Tue, 2018-02-20 at 11:34 +0100, Victor Toso wrote:
> On Tue, Feb 20, 2018 at 11:21:47AM +0100, Lukáš Hrázký wrote:
> > On Tue, 2018-02-20 at 11:02 +0100, Victor Toso wrote:
> > > IMHO, tests are a must for development and should be optional
> > > on tarballs from releases. That means that packagers can
> > > enable it and deal with whatever we do for testing if they
> > > want to.
> > 
> > Thing is, there shouldn't be much (if really anything) you have
> > to deal with for unittests... It's certainly the case atm.
> 
> For us. If we request Catch and Catch is not there for DISTROX,
> they have to disable it in order to have build working or install
> Catch. IMHO, for released tarballs one should enable testing if
> they want to, not disable if they must.

Let's agree we disagree on this one :)

> > > More importantly is that we have tests running successfully on
> > > some Linux distros (IMO, Fedora and Debian should be enough) and
> > > that's why gitlab-ci is there for.
> > 
> > You have different versions of dependencies on different
> > distros, so Fedora and Debian are not entirely enough. And if
> > you do it properly, having the benefit of the tests being run
> > on each distro it's packaged for doesn't cost much... (I'd say
> > the benefit is more for the distros, we are basically helping
> > them by forcing the tests on them :D)
> 
> I say Fedora and Debian due .rpm and .deb which several distros
> might follow so we benefit on testing those two... but if we
> care, then we should enable some unit test in the CI.

Well, that's what CI is for, so we definitely should...

> > > Also IMO, if we change the behavior of unit tests here, we
> > > should do for all Spice components in order to be consistent.
> > 
> > You mean the fact that we run them during packaging?
> 
> That tests are not enabled by default. If we enable it by default
> here I would expect to do the same for other Spice components.

Ok, but I'm somewhat confused here. Enabling the tests is during
packaging - you run `make check`. Which means we are talking about...
Fedora packaging? Or am I getting something wrong?

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

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

[Lukáš Hrázký]

On Tue, 2018-02-20 at 11:02 +0100, Victor Toso wrote:
> Hi,
> 
> On Tue, Feb 20, 2018 at 10:00:41AM +0100, Lukáš Hrázký wrote:
> > On Mon, 2018-02-19 at 21:19 +0200, Uri Lublin wrote:
> > > On 02/19/2018 06:47 PM, Lukáš Hrázký wrote:
> > > > On Mon, 2018-02-19 at 18:29 +0200, Uri Lublin wrote:
> > > > > On 02/14/2018 06:37 PM, Christophe Fergeau wrote:
> > > > > > On Wed, Feb 14, 2018 at 10:40:58AM -0500, Frediano Ziglio wrote:
> > > > > > > > 
> > > > > > > > > On 14 Feb 2018, at 13:34, Lukáš Hrázký  
> > > > > > > > > wrote:
> > > > > > > > > 
> > > > > > > > > Introduce a unit test framework (Catch) to the codebase and a 
> > > > > > > > > simple
> > > > > > > > > unit test for parsing the options of the mjpeg plugin.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Lukáš Hrázký 
> > > > > > > > > ---
> > > > > > > > > configure.ac  |  3 ++
> > > > > > > > > src/mjpeg-fallback.cpp|  5 +++
> > > > > > > > > src/mjpeg-fallback.hpp|  1 +
> > > > > > > > > src/unittests/.gitignore  |  5 +--
> > > > > > > > > src/unittests/Makefile.am | 15 +
> > > > > > > > > src/unittests/test-mjpeg-fallback.cpp | 58
> > > > > > > > > +++
> > > > > > > > > 6 files changed, 85 insertions(+), 2 deletions(-)
> > > > > > > > > create mode 100644 src/unittests/test-mjpeg-fallback.cpp
> > > > > > > > > 
> > > > > > > > > diff --git a/configure.ac b/configure.ac
> > > > > > > > > index 8795dae..5aab662 100644
> > > > > > > > > --- a/configure.ac
> > > > > > > > > +++ b/configure.ac
> > > > > > > > > @@ -17,6 +17,7 @@ if test x"$ac_cv_prog_cc_c99" = xno; then
> > > > > > > > >   AC_MSG_ERROR([C99 compiler is required.])
> > > > > > > > > fi
> > > > > > > > > AC_PROG_CXX
> > > > > > > > > +AC_LANG(C++)
> > > > > > > > > AX_CXX_COMPILE_STDCXX_11
> > > > > > > > > AC_PROG_INSTALL
> > > > > > > > > AC_CANONICAL_HOST
> > > > > > > > > @@ -49,6 +50,8 @@ AC_CHECK_LIB(jpeg, jpeg_destroy_decompress,
> > > > > > > > >   AC_MSG_ERROR([libjpeg not found]))
> > > > > > > > > AC_SUBST(JPEG_LIBS)
> > > > > > > > > 
> > > > > > > > > +AC_CHECK_HEADER([catch/catch.hpp],,[AC_MSG_ERROR([Could not 
> > > > > > > > > find Catch
> > > > > > > > > dependency header (catch/catch.hpp)])])
> > > > > > > > 
> > > > > > > > Instead of an error, shouldn’t we instead fallback to not 
> > > > > > > > compiling the unit
> > > > > > > > tests? Possibly a warning?
> > > > > > > > 
> > > > > > > 
> > > > > > > Good point but I would suggest a follow up and an explicit 
> > > > > > > --I-dont-really-want-unittests
> > > > > > > option, I agree people should run tests.
> > > > > > > Another follow up is a patch for the spec file.
> > > > > > 
> > > > > > Alternatively, this could go with a --with-catch flag (or
> > > > > > --enable-unittest or any names which fits you), and
> > > > > > 1) if there is nothing specified, then we silently enable/disable 
> > > > > > tests
> > > > > > depending on the availability of the catch
> > > > > > 2) if --with-catch is specified, then we error out if it's not found
> > > > > > 3) if --without-catch is used, then we never use it.
> > > > > > 
> > > > > > Then we add --with-catch to autogen.sh, and all developers will 
> > > > > > have to
> > > > > > go through extra hoops not to use it.
> > > > > > 
> > > > > > Christophe
> > > > > > 
> > > > > 
> > > > > I agree that's a good alternative.
> > > > 
> > > > I prefer Frediano's variant. Nobody is forced to use autogen.sh and
> > > > also I don't think anybody would expect autogen.sh to modify the
> > > > default configure behaviour. I don't think it's a good idea.
> > > 
> > > If users prefer to not run autogen.sh that's ok.
> > > It provides defaults options for developers.
> > > For example, I do not expect users to run configure with
> > >   --enable-maintainer-mode too.
> > > Most users will use configure directly from the tarball.
> > > 
> > > Users can choose what options they want to enable/disable
> > > 
> > > > 
> > > > For example, on Gentoo, which doesn't care for autogen.sh and runs
> > > > autotools build automatically, the default behaviour (unless the person
> > > > writing the ebuild would notice) would be dependent on Catch being
> > > > present in the system. This can create subtle inconsistencies, which
> > > > aren't critical in this case, but still unnecessary.
> > > 
> > > So if on Gentoo (or another distribution) there exists no catch
> > > package, they are forced to either add this package or search
> > > for how to disable it.
> > 
> > Yes. The packager is forced to resolve the issue by either providing
> > the dependency or explicitly disabling the tests. In contrast to the
> > tests being "quietly" skipped, if Catch is not present, unless he would
> > notice it. If he isn't dilligent and doesn't notice, the behaviour of
> > the package will depend on the presence of Catch on the target 

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

[Victor Toso]

Hi,

On Tue, Feb 20, 2018 at 10:00:41AM +0100, Lukáš Hrázký wrote:
> On Mon, 2018-02-19 at 21:19 +0200, Uri Lublin wrote:
> > On 02/19/2018 06:47 PM, Lukáš Hrázký wrote:
> > > On Mon, 2018-02-19 at 18:29 +0200, Uri Lublin wrote:
> > > > On 02/14/2018 06:37 PM, Christophe Fergeau wrote:
> > > > > On Wed, Feb 14, 2018 at 10:40:58AM -0500, Frediano Ziglio wrote:
> > > > > > > 
> > > > > > > > On 14 Feb 2018, at 13:34, Lukáš Hrázký  
> > > > > > > > wrote:
> > > > > > > > 
> > > > > > > > Introduce a unit test framework (Catch) to the codebase and a 
> > > > > > > > simple
> > > > > > > > unit test for parsing the options of the mjpeg plugin.
> > > > > > > > 
> > > > > > > > Signed-off-by: Lukáš Hrázký 
> > > > > > > > ---
> > > > > > > > configure.ac  |  3 ++
> > > > > > > > src/mjpeg-fallback.cpp|  5 +++
> > > > > > > > src/mjpeg-fallback.hpp|  1 +
> > > > > > > > src/unittests/.gitignore  |  5 +--
> > > > > > > > src/unittests/Makefile.am | 15 +
> > > > > > > > src/unittests/test-mjpeg-fallback.cpp | 58
> > > > > > > > +++
> > > > > > > > 6 files changed, 85 insertions(+), 2 deletions(-)
> > > > > > > > create mode 100644 src/unittests/test-mjpeg-fallback.cpp
> > > > > > > > 
> > > > > > > > diff --git a/configure.ac b/configure.ac
> > > > > > > > index 8795dae..5aab662 100644
> > > > > > > > --- a/configure.ac
> > > > > > > > +++ b/configure.ac
> > > > > > > > @@ -17,6 +17,7 @@ if test x"$ac_cv_prog_cc_c99" = xno; then
> > > > > > > >   AC_MSG_ERROR([C99 compiler is required.])
> > > > > > > > fi
> > > > > > > > AC_PROG_CXX
> > > > > > > > +AC_LANG(C++)
> > > > > > > > AX_CXX_COMPILE_STDCXX_11
> > > > > > > > AC_PROG_INSTALL
> > > > > > > > AC_CANONICAL_HOST
> > > > > > > > @@ -49,6 +50,8 @@ AC_CHECK_LIB(jpeg, jpeg_destroy_decompress,
> > > > > > > >   AC_MSG_ERROR([libjpeg not found]))
> > > > > > > > AC_SUBST(JPEG_LIBS)
> > > > > > > > 
> > > > > > > > +AC_CHECK_HEADER([catch/catch.hpp],,[AC_MSG_ERROR([Could not 
> > > > > > > > find Catch
> > > > > > > > dependency header (catch/catch.hpp)])])
> > > > > > > 
> > > > > > > Instead of an error, shouldn’t we instead fallback to not 
> > > > > > > compiling the unit
> > > > > > > tests? Possibly a warning?
> > > > > > > 
> > > > > > 
> > > > > > Good point but I would suggest a follow up and an explicit 
> > > > > > --I-dont-really-want-unittests
> > > > > > option, I agree people should run tests.
> > > > > > Another follow up is a patch for the spec file.
> > > > > 
> > > > > Alternatively, this could go with a --with-catch flag (or
> > > > > --enable-unittest or any names which fits you), and
> > > > > 1) if there is nothing specified, then we silently enable/disable 
> > > > > tests
> > > > > depending on the availability of the catch
> > > > > 2) if --with-catch is specified, then we error out if it's not found
> > > > > 3) if --without-catch is used, then we never use it.
> > > > > 
> > > > > Then we add --with-catch to autogen.sh, and all developers will have 
> > > > > to
> > > > > go through extra hoops not to use it.
> > > > > 
> > > > > Christophe
> > > > > 
> > > > 
> > > > I agree that's a good alternative.
> > > 
> > > I prefer Frediano's variant. Nobody is forced to use autogen.sh and
> > > also I don't think anybody would expect autogen.sh to modify the
> > > default configure behaviour. I don't think it's a good idea.
> > 
> > If users prefer to not run autogen.sh that's ok.
> > It provides defaults options for developers.
> > For example, I do not expect users to run configure with
> >   --enable-maintainer-mode too.
> > Most users will use configure directly from the tarball.
> > 
> > Users can choose what options they want to enable/disable
> > 
> > > 
> > > For example, on Gentoo, which doesn't care for autogen.sh and runs
> > > autotools build automatically, the default behaviour (unless the person
> > > writing the ebuild would notice) would be dependent on Catch being
> > > present in the system. This can create subtle inconsistencies, which
> > > aren't critical in this case, but still unnecessary.
> > 
> > So if on Gentoo (or another distribution) there exists no catch
> > package, they are forced to either add this package or search
> > for how to disable it.
> 
> Yes. The packager is forced to resolve the issue by either providing
> the dependency or explicitly disabling the tests. In contrast to the
> tests being "quietly" skipped, if Catch is not present, unless he would
> notice it. If he isn't dilligent and doesn't notice, the behaviour of
> the package will depend on the presence of Catch on the target system (
> on Gentoo packages are compiled on users' systems) and since the
> packager didn't test one of the variants, it could potentially fail on
> the user.
> 
> Packagers at least on Gentoo are used to enabling/disabling things in
> configure. I don't 

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

[Lukáš Hrázký]

On Tue, 2018-02-20 at 10:31 +0100, Christophe de Dinechin wrote:
> > On 20 Feb 2018, at 10:00, Lukáš Hrázký  wrote:
> > 
> > On Mon, 2018-02-19 at 21:19 +0200, Uri Lublin wrote:
> > > On 02/19/2018 06:47 PM, Lukáš Hrázký wrote:
> > > > On Mon, 2018-02-19 at 18:29 +0200, Uri Lublin wrote:
> > > > > On 02/14/2018 06:37 PM, Christophe Fergeau wrote:
> > > > > > On Wed, Feb 14, 2018 at 10:40:58AM -0500, Frediano Ziglio wrote:
> > > > > > > > 
> > > > > > > > > On 14 Feb 2018, at 13:34, Lukáš Hrázký  
> > > > > > > > > wrote:
> > > > > > > > > 
> > > > > > > > > Introduce a unit test framework (Catch) to the codebase and a 
> > > > > > > > > simple
> > > > > > > > > unit test for parsing the options of the mjpeg plugin.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Lukáš Hrázký 
> > > > > > > > > ---
> > > > > > > > > configure.ac  |  3 ++
> > > > > > > > > src/mjpeg-fallback.cpp|  5 +++
> > > > > > > > > src/mjpeg-fallback.hpp|  1 +
> > > > > > > > > src/unittests/.gitignore  |  5 +--
> > > > > > > > > src/unittests/Makefile.am | 15 +
> > > > > > > > > src/unittests/test-mjpeg-fallback.cpp | 58
> > > > > > > > > +++
> > > > > > > > > 6 files changed, 85 insertions(+), 2 deletions(-)
> > > > > > > > > create mode 100644 src/unittests/test-mjpeg-fallback.cpp
> > > > > > > > > 
> > > > > > > > > diff --git a/configure.ac b/configure.ac
> > > > > > > > > index 8795dae..5aab662 100644
> > > > > > > > > --- a/configure.ac
> > > > > > > > > +++ b/configure.ac
> > > > > > > > > @@ -17,6 +17,7 @@ if test x"$ac_cv_prog_cc_c99" = xno; then
> > > > > > > > >  AC_MSG_ERROR([C99 compiler is required.])
> > > > > > > > > fi
> > > > > > > > > AC_PROG_CXX
> > > > > > > > > +AC_LANG(C++)
> > > > > > > > > AX_CXX_COMPILE_STDCXX_11
> > > > > > > > > AC_PROG_INSTALL
> > > > > > > > > AC_CANONICAL_HOST
> > > > > > > > > @@ -49,6 +50,8 @@ AC_CHECK_LIB(jpeg, jpeg_destroy_decompress,
> > > > > > > > >  AC_MSG_ERROR([libjpeg not found]))
> > > > > > > > > AC_SUBST(JPEG_LIBS)
> > > > > > > > > 
> > > > > > > > > +AC_CHECK_HEADER([catch/catch.hpp],,[AC_MSG_ERROR([Could not 
> > > > > > > > > find Catch
> > > > > > > > > dependency header (catch/catch.hpp)])])
> > > > > > > > 
> > > > > > > > Instead of an error, shouldn’t we instead fallback to not 
> > > > > > > > compiling the unit
> > > > > > > > tests? Possibly a warning?
> > > > > > > > 
> > > > > > > 
> > > > > > > Good point but I would suggest a follow up and an explicit 
> > > > > > > --I-dont-really-want-unittests
> > > > > > > option, I agree people should run tests.
> > > > > > > Another follow up is a patch for the spec file.
> > > > > > 
> > > > > > Alternatively, this could go with a --with-catch flag (or
> > > > > > --enable-unittest or any names which fits you), and
> > > > > > 1) if there is nothing specified, then we silently enable/disable 
> > > > > > tests
> > > > > > depending on the availability of the catch
> > > > > > 2) if --with-catch is specified, then we error out if it's not found
> > > > > > 3) if --without-catch is used, then we never use it.
> > > > > > 
> > > > > > Then we add --with-catch to autogen.sh, and all developers will 
> > > > > > have to
> > > > > > go through extra hoops not to use it.
> > > > > > 
> > > > > > Christophe
> > > > > > 
> > > > > 
> > > > > I agree that's a good alternative.
> > > > 
> > > > I prefer Frediano's variant. Nobody is forced to use autogen.sh and
> > > > also I don't think anybody would expect autogen.sh to modify the
> > > > default configure behaviour. I don't think it's a good idea.
> > > 
> > > If users prefer to not run autogen.sh that's ok.
> > > It provides defaults options for developers.
> > > For example, I do not expect users to run configure with
> > >  --enable-maintainer-mode too.
> > > Most users will use configure directly from the tarball.
> > > 
> > > Users can choose what options they want to enable/disable
> > > 
> > > > 
> > > > For example, on Gentoo, which doesn't care for autogen.sh and runs
> > > > autotools build automatically, the default behaviour (unless the person
> > > > writing the ebuild would notice) would be dependent on Catch being
> > > > present in the system. This can create subtle inconsistencies, which
> > > > aren't critical in this case, but still unnecessary.
> > > 
> > > So if on Gentoo (or another distribution) there exists no catch
> > > package, they are forced to either add this package or search
> > > for how to disable it.
> > 
> > Yes. The packager is forced to resolve the issue by either providing
> > the dependency or explicitly disabling the tests. In contrast to the
> > tests being "quietly" skipped, if Catch is not present, unless he would
> > notice it.
> 
> I’d be more comfortable with a warning at the end of configure stating that 
> we did not find catch, so unit 

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

[Christophe de Dinechin]

> On 20 Feb 2018, at 10:43, Lukáš Hrázký  wrote:
> 
> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
>> 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)
> 
> I would like to name the class something more descriptive for what it
> is becoming. Class named Stream in a file named "stream.{cpp,hpp}"
> could be almost anything.

But it’s not named “Stream”, it’s called “spice::streaming_agent::Stream” ;-)

I chose short names because I was in that namespace. Otherwise, I agree with 
you.

Do you think that the name is still too vague even within the namespace?

> My best name is StreamDispatcher so far, not
> entirely happy about it :)


> 
>> {
>> -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);
> 
> This method could also use a better name. write_bytes()?
> write_buffer()?

I intended to do a rename in a follow up. My current choice was “write_packet”, 
because precisely, it’s not writing bytes or a buffer, it’s making sure the 
whole packet gets sent.

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

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

[Christophe Fergeau]

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

NB: My understanding of
https://blogs.gnome.org/desrt/2011/09/08/am_maintainer_mode-is-not-cool/
is that nobody should need to use --enable-maintainer-mode.

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 07/17] Get rid of C-style memset initializations, use C++ style aggregates

[Christophe de Dinechin]

> On 20 Feb 2018, at 10:39, Lukáš Hrázký  wrote:
> 
> On Tue, 2018-02-20 at 10:18 +0100, Christophe de Dinechin wrote:
>>> On Feb 19, 2018, at 7:19 PM, Lukáš Hrázký  wrote:
>>> 
>>> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
 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 = {}
 +};
>>> 
>>> So, someone should find out if we can use the designated initializers,
>>> I suppose it depends on the compilers on all platforms we care about
>>> supporting them?
>>> 
>>> I wasn't able to find much useful information so far. Anyone knows in
>>> which version of gcc it was introduced?
>> 
>> My experience is that clang has no issue with it in any version.
>> 
>> GCC supports it in C++ since at least 4.8.5, as tested on RHEL 7.4 with the 
>> following short example:
>> 
>> struct Thing { int x; float y; };
>> 
>> int foo()
>> {
>>  Thing x = { x: 10, y:20 };
>>  Thing y = { .x = 10, .y = 20 };
>>  Thing z { 10, 20 };
>>  Thing t { .x = 10, .y = 20 };
>> }
>> 
>> It has, however, trouble with out-of-order initializations, with the same 
>> message in 4.8.5 as on Fedora 25 (6.4.1):
>> 
>> glop.cpp:9:30: sorry, unimplemented: non-trivial designated initializers not 
>> supported
>>   Thing a { .y = 10, .x = 20 };
>> 
>> The “not implemented” message is a bit scary, but the fact that the code as 
>> written has been supported as far back as 4.8.5 makes me confident that we 
>> are not in some experimental section of the compiler.
> 
> I've found this on the matter:
> 
> https://gcc.gnu.org/ml/gcc/2014-09/msg00207.html
> 
> That doesn't give much confidence... Is this documented anywhere? I
> mean I've only used Google and haven't found anything…

I think it’s a low priority thing because the “fix” in source code is so easy 
(reorder the fields), and the fix complicated. According to the message you 
referenced, presently, GCC just checks that the annotations match the names, 
but otherwise ignores them and proceeds like for a POD init. If it were to be 
implemented, it would have to support a lot of corner cases mentioned in the 
message, so this makes the fix non-trivial.

> 
> OTOH, if the extension is robust against random mistakes and typos, and
> GCC and clang are the only compilers we 

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

[Lukáš Hrázký]

On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> 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)

I would like to name the class something more descriptive for what it
is becoming. Class named Stream in a file named "stream.{cpp,hpp}"
could be almost anything. My best name is StreamDispatcher so far, not
entirely happy about it :)

>  {
> -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);

This method could also use a better name. write_bytes()?
write_buffer()?

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

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

[Lukáš Hrázký]

On Tue, 2018-02-20 at 10:18 +0100, Christophe de Dinechin wrote:
> > On Feb 19, 2018, at 7:19 PM, Lukáš Hrázký  wrote:
> > 
> > On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> > > 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 = {}
> > > +};
> > 
> > So, someone should find out if we can use the designated initializers,
> > I suppose it depends on the compilers on all platforms we care about
> > supporting them?
> > 
> > I wasn't able to find much useful information so far. Anyone knows in
> > which version of gcc it was introduced?
> 
> My experience is that clang has no issue with it in any version.
> 
> GCC supports it in C++ since at least 4.8.5, as tested on RHEL 7.4 with the 
> following short example:
> 
> struct Thing { int x; float y; };
> 
> int foo()
> {
>   Thing x = { x: 10, y:20 };
>   Thing y = { .x = 10, .y = 20 };
>   Thing z { 10, 20 };
>   Thing t { .x = 10, .y = 20 };
> }
> 
> It has, however, trouble with out-of-order initializations, with the same 
> message in 4.8.5 as on Fedora 25 (6.4.1):
> 
> glop.cpp:9:30: sorry, unimplemented: non-trivial designated initializers not 
> supported
>Thing a { .y = 10, .x = 20 };
> 
> The “not implemented” message is a bit scary, but the fact that the code as 
> written has been supported as far back as 4.8.5 makes me confident that we 
> are not in some experimental section of the compiler.

I've found this on the matter:

https://gcc.gnu.org/ml/gcc/2014-09/msg00207.html

That doesn't give much confidence... Is this documented anywhere? I
mean I've only used Google and haven't found anything...

OTOH, if the extension is robust against random mistakes and typos, and
GCC and clang are the only compilers we care about, I think it should
be fine?

Lukas

> The alternatives are:
> - Not tagging fields at all
> - Tagging them “the old way”, i.e. field:value, but that has been deprecated 
> since 2.5 and actually has the same bug as .field = value, so no benefit.
> 
> 
> > 
> > Lukas
> > 
> > > -memset(, 0, msgsize);
> > > -msg.hdr.protocol_version = STREAM_DEVICE_PROTOCOL;
> > > -msg.hdr.type = STREAM_TYPE_DATA;
> > > -msg.hdr.size = size; /* includes only the body? 

Re: [Spice-devel] [PATCH spice-streaming-agent] Add gitlab-ci.yml ci file

[Victor Toso]

Hi,

On Tue, Feb 20, 2018 at 11:28:47AM +0200, Snir Sheriber wrote:
> Hi,
> 
> On 02/20/2018 11:20 AM, Victor Toso wrote:
> > Hi,
> > 
> > On Tue, Feb 20, 2018 at 11:10:37AM +0200, Snir Sheriber wrote:
> > > Signed-off-by: Snir Sheriber 
> > > ---
> > >   .gitlab-ci.yml | 13 +
> > >   1 file changed, 13 insertions(+)
> > >   create mode 100644 .gitlab-ci.yml
> > > 
> > > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> > > new file mode 100644
> > > index 000..b74e74f
> > > --- /dev/null
> > > +++ b/.gitlab-ci.yml
> > > @@ -0,0 +1,13 @@
> > > +image: fedora:latest
> > > +
> > > +before_script:
> > > +- dnf install 'dnf-command(copr)' automake autoconf libtool gcc-c++ 
> > > -y
> > > +- dnf copr enable @spice/nightly -y
> > > +- dnf builddep spice-streaming-agent -y
> > > +
> > > +build_and_test:
> > > +  script:
> > > +- autoreconf -fi
> > > +- ./configure
> > > +- make
> > > +- make check
> > Looks fine. Do you have a link to a job with this running
> > successfully?
> 
> Running now (tried it on private branch before)
> 
> https://gitlab.com/sheriber/spice-streaming-agent/pipelines/17763303

Acked-by: Victor Toso 


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 v3 3/3] mjpeg-fallback: unittest for the options parsing

[Christophe de Dinechin]

> On 20 Feb 2018, at 10:00, Lukáš Hrázký  wrote:
> 
> On Mon, 2018-02-19 at 21:19 +0200, Uri Lublin wrote:
>> On 02/19/2018 06:47 PM, Lukáš Hrázký wrote:
>>> On Mon, 2018-02-19 at 18:29 +0200, Uri Lublin wrote:
 On 02/14/2018 06:37 PM, Christophe Fergeau wrote:
> On Wed, Feb 14, 2018 at 10:40:58AM -0500, Frediano Ziglio wrote:
>>> 
 On 14 Feb 2018, at 13:34, Lukáš Hrázký  wrote:
 
 Introduce a unit test framework (Catch) to the codebase and a simple
 unit test for parsing the options of the mjpeg plugin.
 
 Signed-off-by: Lukáš Hrázký 
 ---
 configure.ac  |  3 ++
 src/mjpeg-fallback.cpp|  5 +++
 src/mjpeg-fallback.hpp|  1 +
 src/unittests/.gitignore  |  5 +--
 src/unittests/Makefile.am | 15 +
 src/unittests/test-mjpeg-fallback.cpp | 58
 +++
 6 files changed, 85 insertions(+), 2 deletions(-)
 create mode 100644 src/unittests/test-mjpeg-fallback.cpp
 
 diff --git a/configure.ac b/configure.ac
 index 8795dae..5aab662 100644
 --- a/configure.ac
 +++ b/configure.ac
 @@ -17,6 +17,7 @@ if test x"$ac_cv_prog_cc_c99" = xno; then
  AC_MSG_ERROR([C99 compiler is required.])
 fi
 AC_PROG_CXX
 +AC_LANG(C++)
 AX_CXX_COMPILE_STDCXX_11
 AC_PROG_INSTALL
 AC_CANONICAL_HOST
 @@ -49,6 +50,8 @@ AC_CHECK_LIB(jpeg, jpeg_destroy_decompress,
  AC_MSG_ERROR([libjpeg not found]))
 AC_SUBST(JPEG_LIBS)
 
 +AC_CHECK_HEADER([catch/catch.hpp],,[AC_MSG_ERROR([Could not find Catch
 dependency header (catch/catch.hpp)])])
>>> 
>>> Instead of an error, shouldn’t we instead fallback to not compiling the 
>>> unit
>>> tests? Possibly a warning?
>>> 
>> 
>> Good point but I would suggest a follow up and an explicit 
>> --I-dont-really-want-unittests
>> option, I agree people should run tests.
>> Another follow up is a patch for the spec file.
> 
> Alternatively, this could go with a --with-catch flag (or
> --enable-unittest or any names which fits you), and
> 1) if there is nothing specified, then we silently enable/disable tests
> depending on the availability of the catch
> 2) if --with-catch is specified, then we error out if it's not found
> 3) if --without-catch is used, then we never use it.
> 
> Then we add --with-catch to autogen.sh, and all developers will have to
> go through extra hoops not to use it.
> 
> Christophe
> 
 
 I agree that's a good alternative.
>>> 
>>> I prefer Frediano's variant. Nobody is forced to use autogen.sh and
>>> also I don't think anybody would expect autogen.sh to modify the
>>> default configure behaviour. I don't think it's a good idea.
>> 
>> If users prefer to not run autogen.sh that's ok.
>> It provides defaults options for developers.
>> For example, I do not expect users to run configure with
>>  --enable-maintainer-mode too.
>> Most users will use configure directly from the tarball.
>> 
>> Users can choose what options they want to enable/disable
>> 
>>> 
>>> For example, on Gentoo, which doesn't care for autogen.sh and runs
>>> autotools build automatically, the default behaviour (unless the person
>>> writing the ebuild would notice) would be dependent on Catch being
>>> present in the system. This can create subtle inconsistencies, which
>>> aren't critical in this case, but still unnecessary.
>> 
>> So if on Gentoo (or another distribution) there exists no catch
>> package, they are forced to either add this package or search
>> for how to disable it.
> 
> Yes. The packager is forced to resolve the issue by either providing
> the dependency or explicitly disabling the tests. In contrast to the
> tests being "quietly" skipped, if Catch is not present, unless he would
> notice it.

I’d be more comfortable with a warning at the end of configure stating that we 
did not find catch, so unit tests can’t be run, like we do for example for a 
few other optional missing components such as codecs. A warning is also more 
friendly than a hard failure.

To summarize, like Christophe, I would like to see a —with-catch option, but I 
would like to see it default to yes, unless we cannot find catch, in which case:
- If the option —with-catch was specified, configure fails
- If the option was set to default, we get a warning, we can build, we can’t 
test.

(Maybe this is really what Christophe had in mind too, not sure).


> If he isn't dilligent and doesn't notice, the behaviour of
> the package will depend on the presence of Catch on the target system (
> on Gentoo packages are compiled on users' systems) and since the
> packager didn't 

Re: [Spice-devel] [PATCH spice-streaming-agent] Add gitlab-ci.yml ci file

[Snir Sheriber]

Hi,

On 02/20/2018 11:20 AM, Victor Toso wrote:

Hi,

On Tue, Feb 20, 2018 at 11:10:37AM +0200, Snir Sheriber wrote:

Signed-off-by: Snir Sheriber 
---
  .gitlab-ci.yml | 13 +
  1 file changed, 13 insertions(+)
  create mode 100644 .gitlab-ci.yml

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
new file mode 100644
index 000..b74e74f
--- /dev/null
+++ b/.gitlab-ci.yml
@@ -0,0 +1,13 @@
+image: fedora:latest
+
+before_script:
+- dnf install 'dnf-command(copr)' automake autoconf libtool gcc-c++ -y
+- dnf copr enable @spice/nightly -y
+- dnf builddep spice-streaming-agent -y
+
+build_and_test:
+  script:
+- autoreconf -fi
+- ./configure
+- make
+- make check

Looks fine. Do you have a link to a job with this running
successfully?


Running now (tried it on private branch before)

https://gitlab.com/sheriber/spice-streaming-agent/pipelines/17763303


Cheers,
 toso



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

Re: [Spice-devel] [PATCH spice-streaming-agent] Add gitlab-ci.yml ci file

[Victor Toso]

Hi,

On Tue, Feb 20, 2018 at 11:10:37AM +0200, Snir Sheriber wrote:
> Signed-off-by: Snir Sheriber 
> ---
>  .gitlab-ci.yml | 13 +
>  1 file changed, 13 insertions(+)
>  create mode 100644 .gitlab-ci.yml
> 
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> new file mode 100644
> index 000..b74e74f
> --- /dev/null
> +++ b/.gitlab-ci.yml
> @@ -0,0 +1,13 @@
> +image: fedora:latest
> +
> +before_script:
> +- dnf install 'dnf-command(copr)' automake autoconf libtool gcc-c++ -y
> +- dnf copr enable @spice/nightly -y
> +- dnf builddep spice-streaming-agent -y
> +
> +build_and_test:
> +  script:
> +- autoreconf -fi
> +- ./configure
> +- make
> +- make check

Looks fine. Do you have a link to a job with this running
successfully?

Cheers,
toso



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 07/17] Get rid of C-style memset initializations, use C++ style aggregates

[Christophe de Dinechin]

> On Feb 19, 2018, at 7:19 PM, Lukáš Hrázký  wrote:
> 
> On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
>> 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 = {}
>> +};
> 
> So, someone should find out if we can use the designated initializers,
> I suppose it depends on the compilers on all platforms we care about
> supporting them?
> 
> I wasn't able to find much useful information so far. Anyone knows in
> which version of gcc it was introduced?

My experience is that clang has no issue with it in any version.

GCC supports it in C++ since at least 4.8.5, as tested on RHEL 7.4 with the 
following short example:

struct Thing { int x; float y; };

int foo()
{
  Thing x = { x: 10, y:20 };
  Thing y = { .x = 10, .y = 20 };
  Thing z { 10, 20 };
  Thing t { .x = 10, .y = 20 };
}

It has, however, trouble with out-of-order initializations, with the same 
message in 4.8.5 as on Fedora 25 (6.4.1):

glop.cpp:9:30: sorry, unimplemented: non-trivial designated initializers not 
supported
   Thing a { .y = 10, .x = 20 };

The “not implemented” message is a bit scary, but the fact that the code as 
written has been supported as far back as 4.8.5 makes me confident that we are 
not in some experimental section of the compiler.

The alternatives are:
- Not tagging fields at all
- Tagging them “the old way”, i.e. field:value, but that has been deprecated 
since 2.5 and actually has the same bug as .field = value, so no benefit.


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

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

[Spice-devel] [PATCH spice-streaming-agent] Add gitlab-ci.yml ci file

[Snir Sheriber]

Signed-off-by: Snir Sheriber 
---
 .gitlab-ci.yml | 13 +
 1 file changed, 13 insertions(+)
 create mode 100644 .gitlab-ci.yml

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
new file mode 100644
index 000..b74e74f
--- /dev/null
+++ b/.gitlab-ci.yml
@@ -0,0 +1,13 @@
+image: fedora:latest
+
+before_script:
+- dnf install 'dnf-command(copr)' automake autoconf libtool gcc-c++ -y
+- dnf copr enable @spice/nightly -y
+- dnf builddep spice-streaming-agent -y
+
+build_and_test:
+  script:
+- autoreconf -fi
+- ./configure
+- make
+- make check
-- 
2.14.3

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

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

[Lukáš Hrázký]

On Fri, 2018-02-16 at 17:15 +0100, Christophe de Dinechin wrote:
> 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

Acked-by: Lukáš Hrázký 
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

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

[Lukáš Hrázký]

On Mon, 2018-02-19 at 21:19 +0200, Uri Lublin wrote:
> On 02/19/2018 06:47 PM, Lukáš Hrázký wrote:
> > On Mon, 2018-02-19 at 18:29 +0200, Uri Lublin wrote:
> > > On 02/14/2018 06:37 PM, Christophe Fergeau wrote:
> > > > On Wed, Feb 14, 2018 at 10:40:58AM -0500, Frediano Ziglio wrote:
> > > > > > 
> > > > > > > On 14 Feb 2018, at 13:34, Lukáš Hrázký  wrote:
> > > > > > > 
> > > > > > > Introduce a unit test framework (Catch) to the codebase and a 
> > > > > > > simple
> > > > > > > unit test for parsing the options of the mjpeg plugin.
> > > > > > > 
> > > > > > > Signed-off-by: Lukáš Hrázký 
> > > > > > > ---
> > > > > > > configure.ac  |  3 ++
> > > > > > > src/mjpeg-fallback.cpp|  5 +++
> > > > > > > src/mjpeg-fallback.hpp|  1 +
> > > > > > > src/unittests/.gitignore  |  5 +--
> > > > > > > src/unittests/Makefile.am | 15 +
> > > > > > > src/unittests/test-mjpeg-fallback.cpp | 58
> > > > > > > +++
> > > > > > > 6 files changed, 85 insertions(+), 2 deletions(-)
> > > > > > > create mode 100644 src/unittests/test-mjpeg-fallback.cpp
> > > > > > > 
> > > > > > > diff --git a/configure.ac b/configure.ac
> > > > > > > index 8795dae..5aab662 100644
> > > > > > > --- a/configure.ac
> > > > > > > +++ b/configure.ac
> > > > > > > @@ -17,6 +17,7 @@ if test x"$ac_cv_prog_cc_c99" = xno; then
> > > > > > >   AC_MSG_ERROR([C99 compiler is required.])
> > > > > > > fi
> > > > > > > AC_PROG_CXX
> > > > > > > +AC_LANG(C++)
> > > > > > > AX_CXX_COMPILE_STDCXX_11
> > > > > > > AC_PROG_INSTALL
> > > > > > > AC_CANONICAL_HOST
> > > > > > > @@ -49,6 +50,8 @@ AC_CHECK_LIB(jpeg, jpeg_destroy_decompress,
> > > > > > >   AC_MSG_ERROR([libjpeg not found]))
> > > > > > > AC_SUBST(JPEG_LIBS)
> > > > > > > 
> > > > > > > +AC_CHECK_HEADER([catch/catch.hpp],,[AC_MSG_ERROR([Could not find 
> > > > > > > Catch
> > > > > > > dependency header (catch/catch.hpp)])])
> > > > > > 
> > > > > > Instead of an error, shouldn’t we instead fallback to not compiling 
> > > > > > the unit
> > > > > > tests? Possibly a warning?
> > > > > > 
> > > > > 
> > > > > Good point but I would suggest a follow up and an explicit 
> > > > > --I-dont-really-want-unittests
> > > > > option, I agree people should run tests.
> > > > > Another follow up is a patch for the spec file.
> > > > 
> > > > Alternatively, this could go with a --with-catch flag (or
> > > > --enable-unittest or any names which fits you), and
> > > > 1) if there is nothing specified, then we silently enable/disable tests
> > > > depending on the availability of the catch
> > > > 2) if --with-catch is specified, then we error out if it's not found
> > > > 3) if --without-catch is used, then we never use it.
> > > > 
> > > > Then we add --with-catch to autogen.sh, and all developers will have to
> > > > go through extra hoops not to use it.
> > > > 
> > > > Christophe
> > > > 
> > > 
> > > I agree that's a good alternative.
> > 
> > I prefer Frediano's variant. Nobody is forced to use autogen.sh and
> > also I don't think anybody would expect autogen.sh to modify the
> > default configure behaviour. I don't think it's a good idea.
> 
> If users prefer to not run autogen.sh that's ok.
> It provides defaults options for developers.
> For example, I do not expect users to run configure with
>   --enable-maintainer-mode too.
> Most users will use configure directly from the tarball.
> 
> Users can choose what options they want to enable/disable
> 
> > 
> > For example, on Gentoo, which doesn't care for autogen.sh and runs
> > autotools build automatically, the default behaviour (unless the person
> > writing the ebuild would notice) would be dependent on Catch being
> > present in the system. This can create subtle inconsistencies, which
> > aren't critical in this case, but still unnecessary.
> 
> So if on Gentoo (or another distribution) there exists no catch
> package, they are forced to either add this package or search
> for how to disable it.

Yes. The packager is forced to resolve the issue by either providing
the dependency or explicitly disabling the tests. In contrast to the
tests being "quietly" skipped, if Catch is not present, unless he would
notice it. If he isn't dilligent and doesn't notice, the behaviour of
the package will depend on the presence of Catch on the target system (
on Gentoo packages are compiled on users' systems) and since the
packager didn't test one of the variants, it could potentially fail on
the user.

Packagers at least on Gentoo are used to enabling/disabling things in
configure. I don't think it's a hurdle for them to disable the tests if
indeed they don't have the package (and I think most distros have it -
Gentoo does).

> This package is only required if the user wants
> to run the tests.
> Those tests do not run by default either.
> One have to "make check" for the tests to run.

It is