[Spice-devel] Review of the C++ patches

Mon Jun 8 15:10:47 UTC 2020

commit bea4ad66b9603f4d733da42ea0ad28cb7cf8d1de
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Thu May 30 06:33:16 2019 +0100

  char-device: Remove pool handling

  Memory pool handling does not give much but make code more complex.
  Current implementation tends to only increasing buffer sizes
  leading to potential cases where most of the allocated memory is
  not used.

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

This patch removes the char-device memory pool.

>From what I can understand, the "memory pool" was a queue of buffers
where buffers could be pushed instead of freeing/unreferencing them,
and pull from instead of allocating. Only the 'last' buffer of the
pool could be pulled. If not big enough, it was freed and a bigger one
was reallocated.

The pool was emptied when the client was disconnected.

The new behavior is to allocate when needed, unref when not needed
anymore.

The patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit c4b4b967fb98e4b551a08b9d6150d64356038b27
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sat Aug 29 05:35:46 2015 +0100

  Allows C++ to be used in sources

  Enable C++ compiler

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

This patch updates the build system (meson+automake+gitlab CI).

The patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit 7b3637417062fe657c0bf1fced5ef59a489dbdc3
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Mon Mar 9 10:04:24 2020 +0000

  Adjust some warnings

  Remove -Werror and add -fpermissive, this will allow to compile C code with
  a GNU C++ compiler.

  Ignore warnings as our code use some feature like empty arrays.

  Remove warnings not available in C++.

  Bump GLIB_VERSION_MAX_ALLOWED to reduce the warning, looks like the
  GLib headers for C++ are not able to handle them correctly.

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

This patch adds multiple warning to the build systems.

I understand that they ease the transition from C to 'C + C++', but
without a deeper look it's hard to tell which ones should be
eventually removed and which ones (if any) must stay because of the
C/C++ cooperation.

Minor: the explicit-cast fix in `server/red-client.c` doesn't seem to
belong to this patch. I wonder if it's a patch split issue or if there
is a link with build system patch.

Apart from this minor comment, the patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit 81c04623ead52d9b9be949348fc498a9e1d9d89c
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sat May 18 20:20:20 2019 +0100

  Avoid to use reserved C++ keywords

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

This patch remove 'class' and 'template' from variable names, as they
are forbidden in C++.

The patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit 48adb67c13711838e0279e12e8dfb598f2c0db79
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sat May 18 20:21:46 2019 +0100

  Fix order of initializers

  C++ complaints if they are not in the order of
  declaration.

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

The patch solves issues like this one by following the declaration
order.

    error: designator order for field
‘SpiceMsgDisplayGlScanoutUnix::drm_fourcc_format’ does not match
declaration order in ‘SpiceMsgDisplayGlScanoutUnix’

The patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit 505b90e1792d86a9ed13d279f765209c94a0062a
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sat May 18 20:23:44 2019 +0100

  Avoid double definitions

  In C++ that code would define variable twice giving error.
  Define only one.

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

This patch refactors the definition of the `full_header_wrapper` and
`mini_header_wrapper` structures. It was relying on C forward
definitions that are not allowed in C++, so the patch uses function
prototypes instead.

The patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit 238f53d7028cc0ff8a5798d18871ea82271d981d
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sat May 18 20:19:50 2019 +0100

  Declare public exported functions as C

  Allows to link externally from a C program.
  This will allow C++ code to respect C ABI.

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

This patch encloses public header function prototypes with
SPICE_BEGIN_DECLS/SPICE_END_DECLS (aka `extern "C" {` ... `}` from
`spice-protocol/spice/macros.h`), so that they can be linked with C
ABI.

The files touched by this patch are `spice-server` public headers, ie
they end-up in `/usr/include/spice-server/` after installation.

The patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit e0b395fb6865d3d988ceaa4d548dc259d90920ab
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Thu May 23 05:43:55 2019 +0100

  Declare exported functions as C

  Allows to be used by both C and C++ code.
  So to leave part of the code in C and part move to C++.

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

Like the previous patch, this patch encloses public header function
prototypes with SPICE_BEGIN_DECLS/SPICE_END_DECLS (aka `extern "C" {`
... `}` from `spice-protocol/spice/macros.h`), so that they can be
linked with C ABI.

The files touched by this patch are private to `spice-server`.

The patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit 2f7474427bee63dbc94ee323bc46076485c7a7e5
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Mon May 20 21:53:57 2019 +0100

  Do not leave not initialized fields in the middle

  Some old GNU compilers does not allow this (like CentOS 7 one)

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

This patch explicitely initalizes 'fields in the middle' of
structures.

The patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit 6db395dc74f329e04dbe539495c1c34a9d2fdb89
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Wed May 22 13:42:21 2019 +0100

  Make sure empty structure are ABI compatible

  Empty structure are not really portable however adding
  an zero size array seems to be the way to have a zero
  size structure portably.

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

This patch makes sures that every structure has at least a
(zero-length) field:

    uint8_t dummy_empty_field[0]; /* C/C++ compatibility */

a Stackoverflow answer [1] confirms that C++ allows it, but that empty
structures are not fully portable in C:

> C++: A class with an empty sequence of members and base class objects
  is an empty class.

> in C it is rather more murky since the c99 standard has some
  language which implies that truly empty structures aren't allowed
  (see TrayMan's answer) but many compilers do allow it (e.g gcc)

1: https://stackoverflow.com/questions/755305/empty-structure-in-c

The patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit e54224730e41332920964537893740b438245d1f
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sat May 18 20:24:21 2019 +0100

  Change string check

  In C++ GLib add function declaration (function parameters).

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

This patch changes the function name pattern in `g_test_expect_message`
from '*spice_server_remove_interface: ...' to
'*spice_server_remove_interface*:'.

This is related to the fact that in C, two functions cannot have the
same name, whereas in C++, two functions may have the same name but
different parameters.

The patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit 2561f1db94ff1c03e5859ed17ab5cbdc44e2d4c0
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sat May 25 07:34:32 2019 +0100

  Avoid conversion warnings calling Windows sockets

  Some Windows socket functions accepts char* instead of classic void*
  causing some warning. Force the cast.

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

This patch fixes compilation warnings by correctly casting buffers to
`[const] char *` on Windows (`#ifdef _WIN32`).

The patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit d8b1519c3531c298f0d42a802f3ecfbcbb6609b1
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Tue May 21 06:42:50 2019 +0100

  Remove -Wliteral-suffix warnings

  Avoids warnings like

  main-channel-client.cpp:538:27: warning: invalid suffix on literal;
C++11 requires a space between literal and string macro
[-Wliteral-suffix]
      "net test: latency %f ms, bitrate %"G_GUINT64_FORMAT" bps (%f Mbps)%s",
      ^

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

This patch fixes a C++11 compiler warning when concatenating string
leterals and string macros: `"test: %"G_GUINT64_FORMAT"\n"` should read
`"test: %" G_GUINT64_FORMAT "\n"`.

The patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit 17a7251e561912de936c5578c6ea38a720a64c3b
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Tue May 21 06:42:24 2019 +0100

  Remove warnings about combining GParamFlags types

  C++ thread enum a bit more safely than C not allowing to combine
  them. Avoid warning combining them and passing to functions
  expecting enums.

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

This patch explicitely allows '|' combination of GParamFlags
enums. Otherwise, such combinations raise compiler warnings.

Minor: typo in the commit message: s/thread/treats/.

The patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit e788e6e4762fc1c9a13a44b05907fb109bef9cde
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Wed May 22 07:45:30 2019 +0100

  Remove conversion warnings

  Use casts to avoid all that warning.
  They should go away once you use more type safe types.

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

This patch adds explicit casting in many (many many) places.

Minor style inconsistency: pointer cast types are sometimes written `(type
*)`, sometimes `(type*)`, and once `(type* )`.

There is a type changed from `RedChannelClient *rcc` to
`CursorChannelClient *ccc` in function
`cursor_channel_client_reset_cursor_cache`. I'm not sure this belonged
to this patch. But the change seems to be consistent, with `ccc =
CURSOR_CHANNEL_CLIENT(rcc)`

Besides that, the patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit e6e6ded681ff154f236f260f071389c4c8c0d944
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sun May 26 15:10:28 2019 +0100

  Use C++ IS-A relationship for RedChannelClient and RedChannel

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

The code modifications of this patch uses inheritance for structure
types with a parent:

    struct CommonGraphicsChannel
    {
        RedChannel parent;

        ...
    };

becomes:

    struct CommonGraphicsChannel: public RedChannel
    {
        CommonGraphicsChannelPrivate *priv;
    };

Minor: this patch renames 19 files from C to C++ without any
modification, they could have been in a dedicated patch. Likewise,
some modifications are purly C-to-C++ transition (eg,
`s/inttypes.h/ctypes/`).

Minor: some of the modifications could have been included in the
previous patch (casts) or elsewhere (eg, assert-before-ref check).

Besides that, the patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit e204677a43fb582d026f2b9f692e3f47537a3c97
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sun May 26 15:49:05 2019 +0100

  Remove RED_CHANNEL_CLIENT where possible

  Used Coccinelle:

  @@
  expression E;
  @@

  -RED_CHANNEL_CLIENT(E)
  +E

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

This patch removes the calls to `RED_CHANNEL_CLIENT()`, as the types
now have inheritance relationships, so the type conversion is now
implicit.

The patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit 3720e5a6863881e29cc7f8efe49345676bff59eb
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sun May 26 16:01:21 2019 +0100

  Remove RED_CHANNEL where possible

  Used Coccinelle:

  @@
  expression E;
  @@

  -RED_CHANNEL(E)
  +E

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

Like in the previous patch, this patch removes the calls to
`RED_CHANNEL()`, as the types now have inheritance relationships, so
the type conversion is now implicit.

The patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit 08fab74cd702f9d62e7d2653d0f90801b3e5c837
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Mon May 27 04:08:48 2019 +0100

  Avoid useless downcast to RedChannelClient or RedChannel

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

This patch changes multiple variable types/names for more appropriate
types (all related to Channel/ChannelClient and their specialization,
eg `RedChannelClient` -> `DisplayChannelClient`). This on the C++
class inheritance defined in the previous patches, as a
`DisplayChannelClient` is now also a `RedChannelClient`.

Minor: A hunk from e788e6e4762fc1c9a13a44b05907fb109bef9cde certainly
belonged to this patch.

The patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit 2b078c44d7acf2fd26af789eda0869cd337f6621
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sat Feb 29 17:42:56 2020 +0000

  Remove COMMON_GRAPHICS_CHANNEL where possible

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

This patch removes the calls to `COMMON_GRAPHICS_CHANNEL`, thanks to
C++ class inheritance.

The patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit 874e7450887375730a4b3675b9b1b7002b440ea8
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Tue May 28 05:03:01 2019 +0100

  Move all red_channel_client_* functions in header as methods

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

This (big) patch rewrites `red_channel_client_*(rcc, ...)` functions
and calls into object methods.

The patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit 552c26b0a84fad37ca57575c1a15edba582c8a33
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Tue May 28 11:10:30 2019 +0100

  Move all red_channel_* functions in header as methods

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

Like the previous patch, this (big) patch rewrites
`red_channel_*(...)` functions and calls into object methods.

The patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit 8671f659f4dc9b3ae6fd368d5bb12e0d785003d1
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sun May 19 00:04:26 2019 +0100

  Constify some methods

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

This patch adds the `const` keyword to several methods.

It also does a few trivial type changes (`s/gboolean/bool/`, `s/int
blocked/bool blocked`).

The patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit 77af39d6b551a9988313287cbef4f1ff06efad09
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sun May 19 14:46:09 2019 +0100

  Update header style

  Avoid typedef useless in C++.
  Remove useless forward declarations.

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

As described in the commit message, the patch removes typedef and
forward declarations from header files.

The patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit 3a05720eaad93a46b1a4cd3db889b94d7657c724
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Tue Mar 10 02:52:40 2020 +0000

  Introduce some utilities for C++

  red::add_ref will be used to increment a reference counter
  and return the object pointer.

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

This patch adds `utils.hpp` file, with a generic helper function
`red:add_ref(o)` calling `p->ref()` if the object exists.

The patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit 69fbef6fff925fa86671db7878121af39c2476fc
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Mon Mar 2 16:40:59 2020 +0000

  Add RedChannel::(un)ref for reference counting and use them

  This will reduce code to avoid gobject and make code less
  type unsafe.

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

This patch moves `g_object_(un)ref` calls to `RedChannel::(un)ref`
methods and updates the code to use these methods, or `ref::add_ref`
when the object might be `nul`.

See my remark earlier regarding the use of `ref()` vs `this->ref()`.

The patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit 6e14b6bc99a3285d1fdc89c077972f0cab9bfceb
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Tue Mar 3 11:04:05 2020 +0000

  Reimplement video-codes update with no GObject signals

  This is in preparation to remove GObjects, as signals require them.

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

This patch removes the usage of GObject signals to notify the
`DisplayChannelClient` that the list of video-codec allowed has
changed. The update function is now called directly from
`display_channel_set_video_codecs()`.

Minor: typo in the commit message, `s/codes/codecs`.

The patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit 38cd152952968e37d0cc96e95e3e5e47a2f66c2f
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sun Mar 1 19:38:38 2020 +0000

  automake: Link with C++ linker

  If automake sees no C++ files in the source it assumes have to
  use C linker settings not linking C++ library.
  This was not a problem as code did not use C++ libraries but next
  patch will use pure virtual function call.
  It could be provided but as later we will use RTTI use C++ library.

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

This patch adds a `dummy.cpp` in the SOURCES files in order to trick
the linker to use C++ linking. The linker is aware of this trick and
doesn't complain about the missing file.

The patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit 176970f3f18e26ef52f16155bc5fa6edbc60705a
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Sun May 19 22:00:39 2019 +0100

  red-channel-client: Remove GObject type

Huge patch, too hard to review for now.

commit 6e58ea33698d57a4094714375cd7449ff4470a46
Author: Frediano Ziglio <freddy77 at gmail.com>
Date:   Wed Apr 29 17:08:24 2020 +0100

  build: Remove GIO dependency

  It was used for GInitable, now used anymore.

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

This patch removes the GIO dependency from the build systems (meson
and autotools).

Minor: typo in the commit message: `s/now used/not used/`.

The patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit 78193dff16b117f78f678822f5ed670ce30c05f8
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Thu May 23 11:48:13 2019 +0100

  red-channel-capabilities: Removed unused stuff from RedChannelCapabilities

  Not used anymore

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

This patch removes utility functions and declarations related to
`RedChannelCapabilities`.

The patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

commit d8e583805449a627470f005ea7886a23fef8b9dd
Author: Frediano Ziglio <fziglio at redhat.com>
Date:   Mon May 20 15:39:15 2019 +0100

  red-channel-client: Move all functions to methods

  Improve incapsulation.
  The only not mechanical change is the addition of timer_add to
  make timer settings a bit more type safe.

  Signed-off-by: Frediano Ziglio <fziglio at redhat.com>

This patch rewrites static `red_channel_client_*` functions into
`RedChannelClientPrivate` methods.

I cannot see what changed with `timer_add` code. Maybe the message
refers to another patch?

Minor: typo in the commit message: `s/incapsulation/encapsulation/`.

The patch looks good to me.

Acked-by: Kevin Pouget <kpouget at redhat.com>

On Mon, Jun 8, 2020 at 5:10 PM Kevin Pouget <kpouget at redhat.com> wrote:
>
> Hello spice-devel
>
> I worked on the review of Frediano's C++ patches during the last weeks,
> I tried to understand the gist of the commits and summarize it in the review message.
> It's not an in-depth review, and I only spotted minor details.
>
> A few patches are not acked, mostly because they were too big for me to study carefully enough.
>
> Overall, I really appreciate the effort made to adapt the code to C++, from my point of view it is greatly beneficial for the code as it reduces a lot the amount of code duplication (boilerplate code) by leveraging C++ inheritance, polymorphism, virtual methods, etc.
> Likewise, the ref-counting is made simpler and safer with custom classes (share_ptr). These custom classes mimic existing ones AFAICT, but they are more "C-safe" as they cannot throw exceptions.
>
> 3 mails will follow with the reviews.
>
> Best regards,
>
> Kevin