>
> As the tokens counter were not being reset you could enter in a
> situation where client thinks it has more tokens then server which
> would eventually lead to client's disconnection from 0c5eca97f16ec6
> onwards (before it was crashing).
>
> It is easy to check the above situation if you track the amount of
> tokens you have in the client and simply kill and restart the agent
> while doing some file transfer: the client could reach more then 13
> tokens which should not really be possible.
>
> Based on patch from Frediano Ziglio <fziglio@xxxxxxxxxx>
> ---
> server/char-device.c | 6 ++++++
> server/reds.c | 17 +++++------------
> 2 files changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/server/char-device.c b/server/char-device.c
> index cb35aa2..957fb26 100644
> --- a/server/char-device.c
> +++ b/server/char-device.c
> @@ -851,6 +851,12 @@ void red_char_device_reset(RedCharDevice *dev)
> dev_client->num_send_tokens +=
> g_queue_get_length(dev_client->send_queue);
> g_queue_foreach(dev_client->send_queue, (GFunc)red_pipe_item_unref,
> NULL);
> g_queue_clear(dev_client->send_queue);
> +
> + /* If device is reset, we must reset the tokens counters as well as
> we
> + * don't hold any data from client and upon agent's reconnection we
> send
> + * SPICE_MSG_MAIN_AGENT_CONNECTED_TOKENS with all free tokens we
> have */
> + dev_client->num_client_tokens += dev_client->num_client_tokens_free;
> + dev_client->num_client_tokens_free = 0;
> }
> red_char_device_reset_dev_instance(dev, NULL);
> }
> diff --git a/server/reds.c b/server/reds.c
> index 4fd1d35..27a5957 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -542,23 +542,16 @@ static void reds_reset_vdp(RedsState *reds)
> dev->priv->write_filter.discard_all = TRUE;
> dev->priv->client_agent_started = FALSE;
>
> - /* resetting and not destroying the dev as a workaround for a bad
> - * tokens management in the vdagent protocol:
> - * The client tokens' are set only once, when the main channel is
> initialized.
> - * Instead, it would have been more appropriate to reset them upon
> AGENT_CONNECT.
> + /* The client's tokens are set once when the main channel is
> initialized
> + * and once upon agent's connection with
> SPICE_MSG_MAIN_AGENT_CONNECTED_TOKENS.
> * The client tokens are tracked as part of the RedCharDeviceClient.
> Thus,
> * in order to be backward compatible with the client, we need to track
> the tokens
> * even if the agent is detached. We don't destroy the char_device, and
> * instead we just reset it.
> - * In addition, there used to be a misshandling of AGENT_TOKENS message
> in spice-gtk: it
> - * overrides the amount of tokens, instead of adding the given amount.
> + * The tokens are also reset to avoid mismatch in upon agent
> reconnection.
> */
> - if (red_channel_test_remote_cap(&reds->main_channel->base,
> - SPICE_MAIN_CAP_AGENT_CONNECTED_TOKENS))
> {
> - dev->priv->agent_attached = FALSE;
> - } else {
> - red_char_device_reset(RED_CHAR_DEVICE(dev));
> - }
> + dev->priv->agent_attached = FALSE;
> + red_char_device_reset(RED_CHAR_DEVICE(dev));
>
> sif = spice_char_device_get_interface(reds->vdagent);
> if (sif->state) {
> --
> 2.5.5
>
>
I think this patch is an improve, fix an issue and work with very old clients.
I would say
Acked-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
Note that this do not mean I consider the patch that introduced all these
issues good. Having an object structure that represent an object (in this
case a Qemu character device) with entirely different life span and
multiplicity is confusing.
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel