commit bea4ad66b9603f4d733da42ea0ad28cb7cf8d1de
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
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@xxxxxxxxxx>
commit c4b4b967fb98e4b551a08b9d6150d64356038b27
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
This patch updates the build system (meson+automake+gitlab CI).
The patch looks good to me.
Acked-by: Kevin Pouget <kpouget@xxxxxxxxxx>
commit 7b3637417062fe657c0bf1fced5ef59a489dbdc3
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
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@xxxxxxxxxx>
commit 81c04623ead52d9b9be949348fc498a9e1d9d89c
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
Date: Sat May 18 20:20:20 2019 +0100
Avoid to use reserved C++ keywords
Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
commit 48adb67c13711838e0279e12e8dfb598f2c0db79
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
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@xxxxxxxxxx>
commit 505b90e1792d86a9ed13d279f765209c94a0062a
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
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@xxxxxxxxxx>
commit 238f53d7028cc0ff8a5798d18871ea82271d981d
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
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@xxxxxxxxxx>
commit e0b395fb6865d3d988ceaa4d548dc259d90920ab
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
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@xxxxxxxxxx>
commit 2f7474427bee63dbc94ee323bc46076485c7a7e5
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
This patch explicitely initalizes 'fields in the middle' of
structures.
The patch looks good to me.
Acked-by: Kevin Pouget <kpouget@xxxxxxxxxx>
commit 6db395dc74f329e04dbe539495c1c34a9d2fdb89
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
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@xxxxxxxxxx>
commit e54224730e41332920964537893740b438245d1f
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
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@xxxxxxxxxx>
commit 2561f1db94ff1c03e5859ed17ab5cbdc44e2d4c0
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
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@xxxxxxxxxx>
commit d8b1519c3531c298f0d42a802f3ecfbcbb6609b1
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
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@xxxxxxxxxx>
commit 17a7251e561912de936c5578c6ea38a720a64c3b
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
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@xxxxxxxxxx>
commit e788e6e4762fc1c9a13a44b05907fb109bef9cde
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
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@xxxxxxxxxx>
commit e6e6ded681ff154f236f260f071389c4c8c0d944
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
Date: Sun May 26 15:10:28 2019 +0100
Use C++ IS-A relationship for RedChannelClient and RedChannel
Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
commit e204677a43fb582d026f2b9f692e3f47537a3c97
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
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@xxxxxxxxxx>
commit 3720e5a6863881e29cc7f8efe49345676bff59eb
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
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@xxxxxxxxxx>
commit 08fab74cd702f9d62e7d2653d0f90801b3e5c837
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
Date: Mon May 27 04:08:48 2019 +0100
Avoid useless downcast to RedChannelClient or RedChannel
Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
commit 2b078c44d7acf2fd26af789eda0869cd337f6621
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
Date: Sat Feb 29 17:42:56 2020 +0000
Remove COMMON_GRAPHICS_CHANNEL where possible
Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
commit 874e7450887375730a4b3675b9b1b7002b440ea8
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
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@xxxxxxxxxx>
commit 552c26b0a84fad37ca57575c1a15edba582c8a33
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
Date: Tue May 28 11:10:30 2019 +0100
Move all red_channel_* functions in header as methods
Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
commit 8671f659f4dc9b3ae6fd368d5bb12e0d785003d1
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
Date: Sun May 19 00:04:26 2019 +0100
Constify some methods
Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
commit 77af39d6b551a9988313287cbef4f1ff06efad09
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
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@xxxxxxxxxx>
commit 3a05720eaad93a46b1a4cd3db889b94d7657c724
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
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@xxxxxxxxxx>
commit 69fbef6fff925fa86671db7878121af39c2476fc
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
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@xxxxxxxxxx>
commit 6e14b6bc99a3285d1fdc89c077972f0cab9bfceb
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
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@xxxxxxxxxx>
commit 38cd152952968e37d0cc96e95e3e5e47a2f66c2f
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
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@xxxxxxxxxx>
commit 176970f3f18e26ef52f16155bc5fa6edbc60705a
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxx>
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@xxxxxxxxxx>
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@xxxxxxxxxx>
commit 78193dff16b117f78f678822f5ed670ce30c05f8
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
This patch removes utility functions and declarations related to
`RedChannelCapabilities`.
The patch looks good to me.
Acked-by: Kevin Pouget <kpouget@xxxxxxxxxx>
commit d8e583805449a627470f005ea7886a23fef8b9dd
Author: Frediano Ziglio <fziglio@xxxxxxxxxx>
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@xxxxxxxxxx>
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@xxxxxxxxxx>
On Mon, Jun 8, 2020 at 5:10 PM Kevin Pouget <kpouget@xxxxxxxxxx> 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel