RedVmcChannel can have only one client so check directly it
(the RedChannel version just iterate all clients).
Even if the channel would support multiple clients in this case
we would need to check only the one we are sending the message to.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
"RedChannelClient::on_disconnect", which for SpiceVMC is bound to
"spicevmc_red_channel_client_on_disconnect", is called while the
RedChannel object is disconnecting. More precisely is called after
the object has been removed from RedChannel causing
"red_channel_client_is_connected" to be false.
Calling "red_channel_client_destroy" this will lead
to a no-op as function will set "destroying" and call
"red_channel_client_disconnect" which will just exit finding
"red_channel_client_is_connected" false.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
FALSE is the default value, no need to set explicitly.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
"self-tokens" property is 64 bit and must be passed as 64 bit on
32 bit machines to avoid memory corruptions.
This was introduced by 01de3b8922 ("spicevmc: Avoids DoS if
guest device is not able to get data faster enough"), detected by CI.
It caused this error (split into multiple lines):
(./test-leaks:15879): GLib-GObject-CRITICAL **: 14:03:59.650: \
g_object_new_is_valid_property: object class 'RedCharDeviceSpiceVmc' has \
no property named '\xb0/@\xf3\u0001'
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
This fix half (one direction) of
https://gitlab.freedesktop.org/spice/spice/issues/29.
Specifically if you have attempt to transfer a file from the client
using WebDAV.
Previously the queue to the device was unbound. If device was not
getting data fast enough the server started queuing data.
To easily test this you can suspend the WebDAV daemon while transferring
a big file from the client.
To simplify the code and reduce the changes server buffers are
used. This as client token implementation is written with agent
in mind. While when we exhaust server tokens RedCharDevice doesn't
close the client when we exhaust client tokens RedCharDevice closes
the client which in this case it's not wanted.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
This fix half (one direction) of
https://gitlab.freedesktop.org/spice/spice/issues/29.
Specifically if you have attempt to transfer a file to the client
using WebDAV.
Previously the queue to the client was unbound. If client was not
getting data fast enough the server started queuing data.
To easily test this you can use a tool to limit bandwidth and
transfer a big file (I used a "dd if=/dev/zero bs=1M of=test") from
the guest.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
As we don't use any token there's no reason to not queue directly instead
of passing through RedCharDevice.
This will make easier to limit the queue which currently is unlimited.
RedCharDevice flow control has some problems:
- it's really designed with VDI in mind. This for SpiceVMC would cause
code implementation to be confusing having to satisfy the agent token
handling;
- it's using items as unit not allowing counting bytes;
- it duplicates some queue management between RedChannelClient;
- it's broken (see comments in char-device.h);
- it forces "clients" to behave in some way not altering for instance the
queued items (which is very useful if items can be collapsed together);
- it replicates the token handling on the device queue too. This could
seems right but is not that if you have a network card going @ 1 GBit/s
you are able to upload more than 1 Gbit/s just using multiple
connections. The kernel will use a single queue for the network interface
limiting and sharing de facto the various buffers between the multiple
connections.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Unused.
Also the devices should be able to release themselves.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
This should always be defined and including config.h is a requirement.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Victor Toso <victortoso@redhat.com>
Rather than having an API to register client callbacks for each channel
type, make them vfuncs.
Since the client callbacks are registered identically for each channel
of the same type, it doesn't make sense for to require these to be
registered separately for each object. It's cleaner to have these be
per-class properties, so they've been converted to virtual functions.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Only RedCharDeviceVDIPortClass implements this vfunc, rather than
forcing every classes deriving from RedCharDeviceClass to implement it,
red_char_device_send_tokens_to_client() can deal with it.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Uri Lublin <uril@redhat.com>
Instead of having a single red_char_device_write_buffer_get function to
get both client and server buffers and decide by testing client == NULL,
have separate function for a client and for a server. The situation
should always be clear (you're either on the client or on the server
side) and you shouldn't need to parametrize that.
For the server case, add a use_token parameter instead of a separate
red_char_device_write_buffer_get_server_no_token function, as you may
want to parametrize that.
Signed-off-by: Lukáš Hrázký <lhrazky@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Depending on the context, we want to output a warning or just a debug
log.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
These calls seem to have been added for debugging for a very specific
purpose. At the very least, they should have been using g_debug() rather
than spice_printerr(). This commit removes these.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
It was probably meant to be used as a "user_data" argument for the
various callbacks, but turns out not to be used.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This commit adds red_channel_{debug,warning,printerr}() helpers which
will prepend the log message with "channel-name:id (%p)". It also
changes various locations which were doing this manually.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The objects RedsStream and RedsSASL are currently using the namespace
"Reds" rather than the standard "Red" namespace used throughout the rest
of the project. Change these to be consistent. This also means changing
method names and some related enumeration types.
The files were also renamed to reflect the change:
reds-stream.[ch] -> red-stream.[ch]
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This function does not make much sense anymore.
Is called by RedVmcChannel which doesn't use RedChannelClient ACKs
so the variable changed are not used.
Moreover, at red_vmc_channel_constructed() time, there will be no
clients yet, so red_channel_init_outgoing_messages() will be a no-op.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This patch allocates VMC IDs by finding the first ID not used
instead of using a global variable and incrementing the value
for each channel created.
This solves some potential issues:
- remove the global state potentially making possible
to use multiple SpiceServer on the same process;
- don't potentially overflow the variable. This can happen if
channels are allocated/deallocated multiple times
(currently not done by Qemu).
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
This vfunc only has a RedChannelClient * argument, and most of the time,
it operates on RedChannelClient, not on RedChannel. Moreover, the only
time it's used is from RedChannelClient. This commit moves the vfunc to
RedChannelClient, which seems like a better fit for it.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Mostly of red_channel_destroy calls were preceded by
a call to unregister the channel.
The only exception was the main channel as this channel is
always present and its initialisation is a bit different.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
From spice_server_port_event API you can send port events to
any char device. Although currently this is used only for "port"
devices implemented in spicevmc.c this will allow to support
such events using different objects.
This will be used for instance for a streaming device which
will be a specific SpicePort implementation.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
rcc is already deferenced in red_channel_client_get_client so
checking for NULL after that is uselss.
Also this call is generated from red_channel_client_disconnect
which requires the rcc pointer to be valid.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
This is the default value, and the property is marked as
_CONSTRUCT_ONLY, so we don't need to explicitly force the default when
instantiating a RedChannelClient.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
A few dispose/finalize implementations were not chaining up to their
parent classes, causing memory leaks.
This fixes leaks like the following:
==16240== Memcheck, a memory error detector
==16240== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==16240== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
==16240== Command: ./test-leaks
==16240==
==16240==
==16240== HEAP SUMMARY:
==16240== in use at exit: 245,490 bytes in 3,638 blocks
==16240== total heap usage: 5,418 allocs, 1,780 frees, 17,676,968 bytes allocated
==16240==
==16240== 16 bytes in 1 blocks are definitely lost in loss record 203 of 771
==16240== at 0x4C2DADE: malloc (vg_replace_malloc.c:298)
==16240== by 0x4C2FC91: realloc (vg_replace_malloc.c:785)
==16240== by 0x52864E: dispatcher_register_handler (dispatcher.c:374)
==16240== by 0x5293E0: main_dispatcher_constructed (main-dispatcher.c:315)
==16240== by 0x7F873DB: g_object_new_internal (gobject.c:1823)
==16240== by 0x7F87EE4: g_object_new_valist (gobject.c:2042)
==16240== by 0x7F86E90: g_object_new (gobject.c:1626)
==16240== by 0x5292A5: main_dispatcher_new (main-dispatcher.c:295)
==16240== by 0x429A0A: do_spice_init (reds.c:3416)
==16240== by 0x42A3F5: spice_server_init (reds.c:3663)
==16240== by 0x4095B1: server_leaks (test-leaks.c:45)
==16240== by 0x844C60A: test_case_run (gtestutils.c:2161)
==16240== by 0x844C9CA: g_test_run_suite_internal (gtestutils.c:2244)
==16240== by 0x844CA73: g_test_run_suite_internal (gtestutils.c:2256)
==16240== by 0x844CC8A: g_test_run_suite (gtestutils.c:2332)
==16240== by 0x844B92C: g_test_run (gtestutils.c:1599)
==16240== by 0x409A0B: main (test-leaks.c:126)
==16240==
==16240== 16 bytes in 1 blocks are definitely lost in loss record 204 of 771
==16240== at 0x4C2DADE: malloc (vg_replace_malloc.c:298)
==16240== by 0x4C2FC91: realloc (vg_replace_malloc.c:785)
==16240== by 0x52864E: dispatcher_register_handler (dispatcher.c:374)
==16240== by 0x5293E0: main_dispatcher_constructed (main-dispatcher.c:315)
==16240== by 0x7F873DB: g_object_new_internal (gobject.c:1823)
==16240== by 0x7F87EE4: g_object_new_valist (gobject.c:2042)
==16240== by 0x7F86E90: g_object_new (gobject.c:1626)
==16240== by 0x5292A5: main_dispatcher_new (main-dispatcher.c:295)
==16240== by 0x429A0A: do_spice_init (reds.c:3416)
==16240== by 0x42A3F5: spice_server_init (reds.c:3663)
==16240== by 0x40BFD4: test_new (test-display-base.c:902)
==16240== by 0x40979D: vmc_leaks (test-leaks.c:92)
==16240== by 0x844C60A: test_case_run (gtestutils.c:2161)
==16240== by 0x844C9CA: g_test_run_suite_internal (gtestutils.c:2244)
==16240== by 0x844CA73: g_test_run_suite_internal (gtestutils.c:2256)
==16240== by 0x844CC8A: g_test_run_suite (gtestutils.c:2332)
==16240== by 0x844B92C: g_test_run (gtestutils.c:1599)
==16240== by 0x409A0B: main (test-leaks.c:126)
==16240==
==16240== 96 bytes in 1 blocks are definitely lost in loss record 638 of 771
==16240== at 0x4C2FA50: calloc (vg_replace_malloc.c:711)
==16240== by 0x8427D3C: g_malloc0 (gmem.c:124)
==16240== by 0x842801F: g_malloc0_n (gmem.c:355)
==16240== by 0x527B44: dispatcher_constructed (dispatcher.c:141)
==16240== by 0x529321: main_dispatcher_constructed (main-dispatcher.c:307)
==16240== by 0x7F873DB: g_object_new_internal (gobject.c:1823)
==16240== by 0x7F87EE4: g_object_new_valist (gobject.c:2042)
==16240== by 0x7F86E90: g_object_new (gobject.c:1626)
==16240== by 0x5292A5: main_dispatcher_new (main-dispatcher.c:295)
==16240== by 0x429A0A: do_spice_init (reds.c:3416)
==16240== by 0x42A3F5: spice_server_init (reds.c:3663)
==16240== by 0x4095B1: server_leaks (test-leaks.c:45)
==16240== by 0x844C60A: test_case_run (gtestutils.c:2161)
==16240== by 0x844C9CA: g_test_run_suite_internal (gtestutils.c:2244)
==16240== by 0x844CA73: g_test_run_suite_internal (gtestutils.c:2256)
==16240== by 0x844CC8A: g_test_run_suite (gtestutils.c:2332)
==16240== by 0x844B92C: g_test_run (gtestutils.c:1599)
==16240== by 0x409A0B: main (test-leaks.c:126)
==16240==
==16240== 96 bytes in 1 blocks are definitely lost in loss record 639 of 771
==16240== at 0x4C2FA50: calloc (vg_replace_malloc.c:711)
==16240== by 0x8427D3C: g_malloc0 (gmem.c:124)
==16240== by 0x842801F: g_malloc0_n (gmem.c:355)
==16240== by 0x527B44: dispatcher_constructed (dispatcher.c:141)
==16240== by 0x529321: main_dispatcher_constructed (main-dispatcher.c:307)
==16240== by 0x7F873DB: g_object_new_internal (gobject.c:1823)
==16240== by 0x7F87EE4: g_object_new_valist (gobject.c:2042)
==16240== by 0x7F86E90: g_object_new (gobject.c:1626)
==16240== by 0x5292A5: main_dispatcher_new (main-dispatcher.c:295)
==16240== by 0x429A0A: do_spice_init (reds.c:3416)
==16240== by 0x42A3F5: spice_server_init (reds.c:3663)
==16240== by 0x40BFD4: test_new (test-display-base.c:902)
==16240== by 0x40979D: vmc_leaks (test-leaks.c:92)
==16240== by 0x844C60A: test_case_run (gtestutils.c:2161)
==16240== by 0x844C9CA: g_test_run_suite_internal (gtestutils.c:2244)
==16240== by 0x844CA73: g_test_run_suite_internal (gtestutils.c:2256)
==16240== by 0x844CC8A: g_test_run_suite (gtestutils.c:2332)
==16240== by 0x844B92C: g_test_run (gtestutils.c:1599)
==16240== by 0x409A0B: main (test-leaks.c:126)
==16240==
==16240== LEAK SUMMARY:
==16240== definitely lost: 224 bytes in 4 blocks
==16240== indirectly lost: 0 bytes in 0 blocks
==16240== possibly lost: 0 bytes in 0 blocks
==16240== still reachable: 207,718 bytes in 3,312 blocks
==16240== of which reachable via heuristic:
==16240== newarray : 1,536 bytes in 16 blocks
==16240== suppressed: 34,548 bytes in 302 blocks
==16240== Reachable blocks (those to which a pointer was found) are not shown.
==16240== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==16240==
==16240== For counts of detected and suppressed errors, rerun with: -v
==16240== ERROR SUMMARY: 4 errors from 4 contexts (suppressed: 20 from 20)
FAIL test-leaks (exit status: 1)
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This commit changes all functions returning TRUE/FALSE from having an
'int' return value to 'bool'.
This way it's obvious that such a function is not going to return
anything else than TRUE or FALSE.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
These vfuncs are more appropriate in RedChannelClient.
The buffer they allocated are related to the client stream
which is managed directly by RedChannelClient.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
For each channel there are two set of capabilities, one
for the common ones and one for the specific ones.
A single set were almost always passed using 2 arguments,
a number of elements and an array but then before using
these were converted to a GArray.
Use a single structure (already available) to pass all
channel capabilites using a single argument.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
spice_new is a bit more safe as return a properly typed pointer.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Most channels don't need to do specific settings for the client socket
so avoid the need to do this setting making easier to setup the client
channnel.
Some improvements and commit subject suggested by Christophe Fergeau.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
TCP_NODELAY flag is set by default for all connection inside
reds.c so there's no need to set again for the single
client channel.
Note that there are still some calls to setsockopt to change this
option.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
red_channel_client_parse() currently does roughly:
if (klass->parser) {
parsed = klass->parser(msg, msg_size, &parsed_size);
klass->handle_parsed(rcc, parsed_size, msg_type, parsed);
} else {
klass->handle_message(rcc, msg_type, msg, msg_size);
}
The handle_parsed implementation expects a void * 'parsed' argument,
which will then be cast to the correct type. There is not really a need
to provide distinct handle_parsed/handle_message vfuncs, instead we can
say that if a RedChannel subclass provides a 'parser' vfunc, then it's
'handle_message' vfunc will be called with the parsed message, otherwise
it will be called with unparsed data (ie what handle_message currently
expects).
This makes the code slightly easier to follow as messages will always be
handled by the same vfunc.
Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
This third argument (and the 'item' member of
RedChannelClient::priv::send_data) was a somewhat roundabout way to keep
the RedPipeItem alive until a message is sent, just in case some data
owned by that pipeitem was added to the marshaller by reference. This
was a rather confusing mechanism, however, since it did not have any
obvious connection to the _add_by_ref() call. It was never very clear
whether you needed to pass an item to this function or not. The previous
series of patches made this parameter unnecessary since the referencing
of the pipe item (or other related structure) is now more explicitly
connected to the calls to spice_marshaller_add_by_ref_full().
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The only time that the pipe item needs to be passed as the third
argument to red_channel_client_init_send_data() is when the pipe item
holds a data buffer that has been added to the marshaller by reference
(spice_marshaller_add_by_ref()) and needs to be kept alive until the
data has been sent. In all other cases, the item does not need to be
kept alive, so we can safely pass NULL for this third parameter.
Acked-by: Frediano Ziglio <fziglio@redhat.com>
The spice_marshaller_add_ref() family of functions is confusing since it
sounds like you're incrementing a reference on the marshaller. What it
is actually doing is adding a data buffer to the marshaller by reference
rather than by value. Changing the function names to _add_by_ref() makes
this clearer.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Pavel Grunt <pgrunt@redhat.com>
These cause compiler errors using RHEL6.
These typedefs are defined in the same file some lines above.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Acked-by: Christophe Fergeau <cfergeau@redhat.com>
Add a 'self' parameter to all of the char device virtual functions so
that we don't have to play games with the 'opaque' pointer.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
Instead of having channel and device object create one the other
create the objects at the beginning and then join them.
This make explicit the code that links the two objects and separate
the objects creation from the linking.
Also remove some boilerplate code.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Jonathon Jongsma <jjongsma@redhat.com>
Acked-by: Frediano Ziglio <fziglio@redhat.com>
