WIP: Refactor the streaming agent towards a more standard C++ style

Submitted by Christophe de Dinechin on Feb. 16, 2018, 4:15 p.m.

Details

Reviewer None
Submitted Feb. 16, 2018, 4:15 p.m.
Last Updated Feb. 21, 2018, 5:47 p.m.
Revision 2

Cover Letter(s)

Revision 1
      From: Christophe de Dinechin <dinechin@redhat.com>

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

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

Reasons for marking this WIP:

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

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

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

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

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

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

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

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

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

 src/concrete-agent.cpp        |   1 +
 src/concrete-agent.hpp        |   4 +
 src/mjpeg-fallback.cpp        |   2 +-
 src/spice-streaming-agent.cpp | 521 +++++++++++++++++++++++++-----------------
 4 files changed, 311 insertions(+), 217 deletions(-)
    
Revision 2
      From: Christophe de Dinechin <dinechin@redhat.com>

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.

Changes since v1:

- Applied three different fixes to the X11Cursor case, one seen by Lukas,
  one seen by Frediano, one seen by me.

- This prompted me to decide that the way I was sending the packets
  was insufficiently clear (not robust to copy-pasting), so I
  addressed that.

- Switch to exceptions to report write errors, on a suggestion of
  Lukash. I used that as an opportunity to show what I had in mind
  with respect to "delayed formatting" of exceptions.

- Added a 'catch all' clause at the top, and catch all std::exception,
  so that we don't go to 'terminate()' if a plugin throws
  std::bad_alloc or something like that.

- Still WIP, because I could not test (although for a different reason).
  I had some guest X11 configuration issues after installing the P4 card.
  I have been trying to fix it since this morning, and decided to not
  delay the sending of this series despite having not really tested it,
  because a few people are "blocked" on it

Christophe de Dinechin (24):
  Add missing <string> 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
  Catch all std::exception derivatives (including std::bad_alloc, not a
    std::bad_alloc)
  Make streaming_requested a method in Stream
  Throw an exception in case we can't write a complete packet.
  Reformat 'if' statments according to style guide
  Throw exception in case of write failure
  Rename 'streamfd' variable to 'stream', no longer a file descriptor
  Rename 'Stream' to 'IOChannel'

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

Revisions

Patches download mbox

# Name Submitter State A F R T
[01/17] Add missing <string> header Christophe de Dinechin Superseded 1
[02/17] log_binary is really a boolean Christophe de Dinechin Superseded
[03/17] Eliminate signed/unsigned warning Christophe de Dinechin New
[04/17] Do not create std::string for constants Christophe de Dinechin Accepted
[05/17] Use RAII to cleanup stream in case of exception or return Christophe de Dinechin New
[06/17] Replace inefficient C-style initialization with C++-style Christophe de Dinechin Accepted
[07/17] Get rid of C-style memset initializations, use C++ style aggregates Christophe de Dinechin New
[08/17] Use C++ style for cursor message initialization instead of C style Christophe de Dinechin New
[09/17] Reorder headers according to style guide Christophe de Dinechin New
[10/17] Since we use a namespace, simplify name of local classes Christophe de Dinechin New 1
[11/17] Move read, write and locking into the 'Stream' class Christophe de Dinechin New
[12/17] Convert message writing from C style to C++ style Christophe de Dinechin New
[13/17] Add more meaningful syslog reporting Christophe de Dinechin New
[14/17] Create a class encapsulating the X11 display cursor capture Christophe de Dinechin New
[15/17] Create FrameLog class to abstract logging of frames Christophe de Dinechin New
[16/17] Remove client_codecs global variable, moved inside the 'Stream' class Christophe de Dinechin New
[17/17] Move the capture loop in the ConcreteAgent, get rid of global agent variable Christophe de Dinechin New

Patches download mbox

# Name Submitter State A F R T
[v2,01/24] Add missing <string> header Christophe de Dinechin Accepted
[v2,02/24] log_binary is really a boolean Christophe de Dinechin Superseded
[v2,03/24] Eliminate signed/unsigned warning Christophe de Dinechin New
[v2,04/24] Do not create std::string for constants Christophe de Dinechin Accepted
[v2,05/24] Use RAII to cleanup stream in case of exception or return Christophe de Dinechin New
[v2,06/24] Replace inefficient C-style initialization with C++-style Christophe de Dinechin New
[v2,07/24] Get rid of C-style memset initializations, use C++ style aggregates Christophe de Dinechin New
[v2,08/24] Use C++ style for cursor message initialization instead of C style Christophe de Dinechin New
[v2,09/24] Reorder headers according to style guide Christophe de Dinechin New
[v2,10/24] Since we use a namespace, simplify name of local classes Christophe de Dinechin New
[v2,11/24] Move read, write and locking into the 'Stream' class Christophe de Dinechin New
[v2,12/24] Convert message writing from C style to C++ style Christophe de Dinechin New
[v2,13/24] Add more meaningful syslog reporting Christophe de Dinechin New
[v2,14/24] Create a class encapsulating the X11 display cursor capture Christophe de Dinechin New
[v2,15/24] Create FrameLog class to abstract logging of frames Christophe de Dinechin New
[v2,16/24] Remove client_codecs global variable, moved inside the 'Stream' class Christophe de Dinechin New
[v2,17/24] Move the capture loop in the ConcreteAgent, get rid of global agent variable Christophe de Dinechin New
[v2,18/24] Catch all std::exception derivatives (including std::bad_alloc, not a std::bad_alloc) Christophe de Dinechin New
[v2,19/24] Make streaming_requested a method in Stream Christophe de Dinechin New
[v2,20/24] Throw an exception in case we can't write a complete packet. Christophe de Dinechin New
[v2,21/24] Reformat 'if' statments according to style guide Christophe de Dinechin New
[v2,22/24] Throw exception in case of write failure Christophe de Dinechin New
[v2,23/24] Rename 'streamfd' variable to 'stream', no longer a file descriptor Christophe de Dinechin New
[v2,24/24] Rename 'Stream' to 'IOChannel' Christophe de Dinechin New