Re: [Spice-devel] [PATCH 01/16] UpgradeItem: use base PipeItem for refcounting
>
> No need to re-implement refcounting in this subclass. However, I needed
> to add a new 'dcc' member to UpgradeItem to be able to unref properly.
> ---
> Change since last patch: take ref on dcc
>
> server/dcc.c | 18 +-
> server/display-channel.c | 4 +---
> server/display-channel.h | 2 +-
> server/stream.c | 18 --
> 4 files changed, 19 insertions(+), 23 deletions(-)
>
> diff --git a/server/dcc.c b/server/dcc.c
> index 91c3f82..5193c17 100644
> --- a/server/dcc.c
> +++ b/server/dcc.c
> @@ -1591,29 +1591,15 @@ int dcc_handle_migrate_data(DisplayChannelClient
> *dcc, uint32_t size, void *mess
> return TRUE;
> }
>
> -static void upgrade_item_unref(DisplayChannel *display, UpgradeItem *item)
> -{
> -if (--item->refs != 0)
> -return;
> -
> -display_channel_drawable_unref(display, item->drawable);
> -free(item->rects);
> -free(item);
> -}
> -
> static void release_item_after_push(DisplayChannelClient *dcc, PipeItem
> *item)
> {
> -DisplayChannel *display = DCC_TO_DC(dcc);
> -
> switch (item->type) {
> case PIPE_ITEM_TYPE_DRAW:
> case PIPE_ITEM_TYPE_IMAGE:
> case PIPE_ITEM_TYPE_STREAM_CLIP:
> case PIPE_ITEM_TYPE_MONITORS_CONFIG:
> -pipe_item_unref(item);
> -break;
> case PIPE_ITEM_TYPE_UPGRADE:
> -upgrade_item_unref(display, (UpgradeItem *)item);
> +pipe_item_unref(item);
> break;
> case PIPE_ITEM_TYPE_GL_SCANOUT:
> case PIPE_ITEM_TYPE_GL_DRAW:
> @@ -1651,8 +1637,6 @@ static void
> release_item_before_push(DisplayChannelClient *dcc, PipeItem *item)
> }
> case PIPE_ITEM_TYPE_STREAM_CLIP:
> case PIPE_ITEM_TYPE_UPGRADE:
> -upgrade_item_unref(display, (UpgradeItem *)item);
> -break;
> case PIPE_ITEM_TYPE_IMAGE:
> case PIPE_ITEM_TYPE_MONITORS_CONFIG:
> pipe_item_unref(item);
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 88dbc74..98b4a43 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -1973,10 +1973,8 @@ static void hold_item(RedChannelClient *rcc, PipeItem
> *item)
> case PIPE_ITEM_TYPE_DRAW:
> case PIPE_ITEM_TYPE_IMAGE:
> case PIPE_ITEM_TYPE_STREAM_CLIP:
> -pipe_item_ref(item);
> -break;
> case PIPE_ITEM_TYPE_UPGRADE:
> -((UpgradeItem *)item)->refs++;
> +pipe_item_ref(item);
> break;
> default:
> spice_warn_if_reached();
> diff --git a/server/display-channel.h b/server/display-channel.h
> index 6b053de..8944f06 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -244,7 +244,7 @@ typedef struct SurfaceDestroyItem {
>
> typedef struct UpgradeItem {
> PipeItem base;
> -int refs;
> +DisplayChannelClient *dcc;
Here a display channel is enough I think.
> Drawable *drawable;
> SpiceClipRects *rects;
> } UpgradeItem;
> diff --git a/server/stream.c b/server/stream.c
> index ae37a62..80cfe2e 100644
> --- a/server/stream.c
> +++ b/server/stream.c
> @@ -758,6 +758,18 @@ void stream_agent_stop(StreamAgent *agent)
> }
> }
>
> +static void upgrade_item_free(UpgradeItem *item)
> +{
> +g_return_if_fail(item != NULL);
> +DisplayChannel *display = DCC_TO_DC(item->dcc);
> +g_return_if_fail(item->base.refcount != 0);
> +
> +display_channel_drawable_unref(display, item->drawable);
I'm still pondering this.
My initial question was mainly a question (even to me) than a
proposal. Here the problem is that this can create a circular
reference counting. The problem is quite hidden by the fact that
we don't still have a way to free part of our stuff (DisplayChannel
is one). This make hard to verify this, I should have pushed the
inclusion of my cleanup branch when was refused.
Jonathon... did you test for leaks?
> +free(item->rects);
> +red_channel_client_unref(RED_CHANNEL_CLIENT(item->dcc));
> +free(item);
> +}
> +
> /*
> * after dcc_detach_stream_gracefully is called for all the display channel
> clients,
> * detach_stream should be called. See comment (1).
> @@ -798,10 +810,12 @@ static void
> dcc_detach_stream_gracefully(DisplayChannelClient *dcc,
> rect_debug(&stream->current->red_drawable->bbox);
> rcc = RED_CHANNEL_CLIENT(dcc);
> upgrade_item = spice_new(UpgradeItem, 1);
> -upgrade_item->refs = 1;
> -pipe_item_init(&upgrade_item->base, PIPE_ITEM_TYPE_UPGRADE);
> +pipe_item_init_full(&upgrade_item->base, PIPE_ITEM_TYPE_UPGRADE,
> +(GDestroyNotify)upgrade_item_free);
> upgrade_item->drawable = stream->current;
> upgrade_item->drawable->refs++;
> +upgrade_item->dcc = dcc;
> +red_channel_client_ref(RED_CHANNEL_CLIENT(upgrade_item->dcc));
> n_rects =
> pixman_region32_n_rects(&upgrade_item->drawable->tree_item.base.rgn);
> upgrade_item->rects =
Re: [Spice-devel] [PATCH 02/16] Replace RedCharDevice::on_free_self_token with a signal
>
> From: Christophe Fergeau
>
> It's more natural to do things this way with glib, and this allows to
> remove some coupling between Reds and RedCharDeviceVDIPort. Before this
> commit, the RedCharDeviceVDIPort has to provide a on_free_self_token()
> because Reds needs to know about it. With a signal, RedCharDeviceVDIPort
> does not have to do anything special, and Reds can just connect to the
> signal it's interested in.
> ---
> Frediano didn't like the signal approach. I think it's OK. Still needs
> resolution.
Let me clarify this, I think there are some pending issues/comments
1) signal and callback
This patch change the callback and use a glib signal instead. I don't
like the dynamic binding and I prefer previous static one but Christophe
and Jonathon prefer the signal (which is dynamic) and we are moving to
the unsafe world of glib so it's not a problem
2) reasoning
Moving from callback to signal does not remove the dependency, the
dependency is just changed to a signal, specifically
" this allows to remove some coupling between Reds and RedCharDeviceVDIPort"
no, you just changed char_dev_class->on_free_self_token assignment
to a g_signal_connect.
"With a signal, RedCharDeviceVDIPort does not have to do anything special"
no, RedCharDeviceVDIPort has to register a signal and emit it.
I would just change the comment to
It's more natural to do things this way with glib.
With a signal Reds can just connect to the signal it's interested in.
this was my strong argument
3) why the single callback was changed to multiple signals?
(this was raised by Jonathon)
Frediano
>
> server/char-device.c | 47 ---
> server/char-device.h | 4
> server/reds.c| 7 +--
> 3 files changed, 41 insertions(+), 17 deletions(-)
>
> diff --git a/server/char-device.c b/server/char-device.c
> index ebe7633..f3e16da 100644
> --- a/server/char-device.c
> +++ b/server/char-device.c
> @@ -94,6 +94,14 @@ enum {
> PROP_OPAQUE
> };
>
> +enum {
> + RED_CHAR_DEVICE_SELF_TOKEN_CONSUMED,
> + RED_CHAR_DEVICE_SELF_TOKEN_RELEASED,
> + RED_CHAR_DEVICE_LAST_SIGNAL
> +};
> +
> +static guint signals[RED_CHAR_DEVICE_LAST_SIGNAL];
> +
> static void red_char_device_write_buffer_unref(RedCharDeviceWriteBuffer
> *write_buf);
> static void red_char_device_write_retry(void *opaque);
>
> @@ -131,16 +139,6 @@ red_char_device_send_tokens_to_client(RedCharDevice
> *dev,
> }
>
> static void
> -red_char_device_on_free_self_token(RedCharDevice *dev)
> -{
> - RedCharDeviceClass *klass = RED_CHAR_DEVICE_GET_CLASS(dev);
> -
> - if (klass->on_free_self_token != NULL) {
> - klass->on_free_self_token(dev->priv->opaque);
> - }
> -}
> -
> -static void
> red_char_device_remove_client(RedCharDevice *dev, RedClient *client)
> {
> RedCharDeviceClass *klass = RED_CHAR_DEVICE_GET_CLASS(dev);
> @@ -624,6 +622,9 @@ static RedCharDeviceWriteBuffer
> *__red_char_device_write_buffer_get(
> }
> } else if (origin == WRITE_BUFFER_ORIGIN_SERVER) {
> dev->priv->num_self_tokens--;
> +g_signal_emit(G_OBJECT(dev),
> + signals[RED_CHAR_DEVICE_SELF_TOKEN_CONSUMED],
> + 0);
> }
>
> ret->token_price = migrated_data_tokens ? migrated_data_tokens : 1;
> @@ -711,7 +712,9 @@ void red_char_device_write_buffer_release(RedCharDevice
> *dev,
> red_char_device_client_tokens_add(dev, dev_client, buf_token_price);
> } else if (buf_origin == WRITE_BUFFER_ORIGIN_SERVER) {
> dev->priv->num_self_tokens++;
> -red_char_device_on_free_self_token(dev);
> +g_signal_emit(G_OBJECT(dev),
> + signals[RED_CHAR_DEVICE_SELF_TOKEN_RELEASED],
> + 0);
> }
> }
>
> @@ -1215,6 +1218,28 @@ red_char_device_class_init(RedCharDeviceClass *klass)
> "User data to pass
> to callbacks",
>G_PARAM_STATIC_STRINGS
>|
>G_PARAM_READWRITE));
> +signals[RED_CHAR_DEVICE_SELF_TOKEN_CONSUMED] =
> + g_signal_new("self-token-consumed",
> +G_OBJECT_CLASS_TYPE(object_class),
> +G_SIGNAL_RUN_FIRST,
> +0, NULL, NULL,
> +g_cclosure_marshal_VOID__VOID,
> +G_TYPE_NONE,
> +0);
> +
> +/* FIXME: find a better name for the signal? It replaces a
> + * on_free_self_token vfunc whose description was:
> + * « The cb is called when a server (self) message that was addressed to
> the device,
> + * has been completely written to it »
> + */
> +signals[RED_CHAR_DEVICE_SELF_TOKEN_RELEASED] =
> + g_signal_new("self-token-released",
> +
Re: [Spice-devel] [PATCH] Disable IME to allow receiving all keys
My understanding is, Virt-Viewer with ImmDisableIME(-1) can send key events even if the title bar is focused when IME is enabled. I attached the screenshot of the problem when ImmDisableIME(-1) is not applied. https://bugzilla.redhat.com/show_bug.cgi?id=1311858#c42 But spice-gtk can receives all the key events and if ImmDisableIME(-1) is applied, users cannot switch IMEs during running Virt-Viewer. I tested native client with Internet Explorer 10 in Windows 7 and I couldn't see any other differences when ImmDisableIME(-1) is applied. If ImmDisableIME(-1) fixes only when the title bar is focused, I'd think the customer's bug is different and Virt-Viewer without ImmDisableIME(-1) might be more useful because it can switch IMEs during running Virt-Viewer. I asked the customer if all the bugs can be fixed without ImmDisableIME(-1). Thanks, Fujiwara On 04/19/16 23:31, Fabiano Fidêncio-san wrote: On Tue, Apr 19, 2016 at 4:11 PM, Fabiano Fidêncio wrote: On Tue, Apr 19, 2016 at 4:00 PM, Frediano Ziglio wrote: On Tue, Apr 19, 2016 at 3:28 PM, Pavel Grunt wrote: Ack, I am going to push it. Do we really need this patch upstream? It's a half-solution, at most, that doesn't work on newer Windows. Are you referring to Window 11? It works on Windows 10. Hmm. I was basing my comment on https://bugzilla.redhat.com/show_bug.cgi?id=1297640#c12 but I didn't realize your comment https://bugzilla.redhat.com/show_bug.cgi?id=1297640#c13 So, yeah, you have my ACK as well, but a better/more descriptive commit message is needed. Patch got applied already, so, nevermind. Frediano Also, a better commit message would be more than appreciated in case we really decide to have it upstream. Thanks, Pavel On Mon, 2016-04-18 at 14:55 +0100, Frediano Ziglio wrote: From: Christophe Fergeau This fixes https://bugzilla.redhat.com/show_bug.cgi?id=1297640 Acked-by: Frediano Ziglio --- src/virt-viewer-util.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/src/virt-viewer-util.c b/src/virt-viewer-util.c index 279f9a5..8cf52ec 100644 --- a/src/virt-viewer-util.c +++ b/src/virt-viewer-util.c @@ -30,6 +30,7 @@ #ifdef G_OS_WIN32 #include #include +#include #endif #include @@ -272,6 +273,12 @@ void virt_viewer_util_init(const char *appname) dup2(fileno(stdout), STDOUT_FILENO); dup2(fileno(stderr), STDERR_FILENO); } + +/* Disable input method handling so that the Zenkaku_Hankaku can be passed + * to VMs rather than being captured by Windows. + * https://bugzilla.redhat.com/show_bug.cgi?id=1297640 + */ +ImmDisableIME(-1); #endif setlocale(LC_ALL, ""); ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel -- Fabiano Fidêncio -- Fabiano Fidêncio ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 04/16] Use GQueue for RedCharDevice::send_queue
>
> From: Christophe Fergeau
>
> There was an extra RedCharDeviceMsgToClientItem type whose only
> purpose was to manage a linked list of items to send. GQueue has the
> same purpose as this type in addition to being generic. As the length of
> the send queue is tracked, a GQueue is more appropriate than a GList and
> allow to remove RedCharDevice::send_queue_size.
> ---
> Changes since last version:
> - red_char_device_client_send_queue_free(): update tokens before clearing
> queue
> - red_char_device_client_send_queue_push(): change GList* variable to
> PipeItem*
>
> server/char-device.c | 69
> ++--
> 1 file changed, 23 insertions(+), 46 deletions(-)
>
> diff --git a/server/char-device.c b/server/char-device.c
> index f3e16da..2206c21 100644
> --- a/server/char-device.c
> +++ b/server/char-device.c
> @@ -41,8 +41,7 @@ struct RedCharDeviceClient {
> uint64_t num_send_tokens; /* send to client */
> SpiceTimer *wait_for_tokens_timer;
> int wait_for_tokens_started;
> -Ring send_queue;
> -uint32_t send_queue_size;
> +GQueue *send_queue;
> uint32_t max_send_queue_size;
> };
>
> @@ -105,11 +104,6 @@ static guint signals[RED_CHAR_DEVICE_LAST_SIGNAL];
> static void red_char_device_write_buffer_unref(RedCharDeviceWriteBuffer
> *write_buf);
> static void red_char_device_write_retry(void *opaque);
>
> -typedef struct RedCharDeviceMsgToClientItem {
> -RingItem link;
> -PipeItem *msg;
> -} RedCharDeviceMsgToClientItem;
> -
> static PipeItem *
> red_char_device_read_one_msg_from_device(RedCharDevice *dev)
> {
> @@ -187,19 +181,10 @@ static void
> red_char_device_write_buffer_pool_add(RedCharDevice *dev,
> static void red_char_device_client_send_queue_free(RedCharDevice *dev,
> RedCharDeviceClient
> *dev_client)
> {
> -spice_debug("send_queue_empty %d",
> ring_is_empty(&dev_client->send_queue));
> -while (!ring_is_empty(&dev_client->send_queue)) {
> -RingItem *item = ring_get_tail(&dev_client->send_queue);
> -RedCharDeviceMsgToClientItem *msg_item = SPICE_CONTAINEROF(item,
> -
> RedCharDeviceMsgToClientItem,
> - link);
> -
> -ring_remove(item);
> -pipe_item_unref(msg_item->msg);
> -free(msg_item);
> -}
> -dev_client->num_send_tokens += dev_client->send_queue_size;
> -dev_client->send_queue_size = 0;
> +spice_debug("send_queue_empty %d",
> g_queue_is_empty(dev_client->send_queue));
> +dev_client->num_send_tokens +=
> g_queue_get_length(dev_client->send_queue);
> +g_queue_free_full(dev_client->send_queue, pipe_item_unref);
> +g_queue_clear(dev_client->send_queue);
> }
>
> static void red_char_device_client_free(RedCharDevice *dev,
> @@ -303,17 +288,14 @@ static void
> red_char_device_add_msg_to_client_queue(RedCharDeviceClient *dev_cli
> PipeItem *msg)
> {
> RedCharDevice *dev = dev_client->dev;
> -RedCharDeviceMsgToClientItem *msg_item;
>
> -if (dev_client->send_queue_size >= dev_client->max_send_queue_size) {
> +if (g_queue_get_length(dev_client->send_queue) >=
> dev_client->max_send_queue_size) {
> red_char_device_handle_client_overflow(dev_client);
> return;
> }
>
> -msg_item = spice_new0(RedCharDeviceMsgToClientItem, 1);
> -msg_item->msg = pipe_item_ref(msg);
> -ring_add(&dev_client->send_queue, &msg_item->link);
> -dev_client->send_queue_size++;
> +pipe_item_ref(msg);
> +g_queue_push_head(dev_client->send_queue, msg);
> if (!dev_client->wait_for_tokens_started) {
> reds_core_timer_start(dev->priv->reds,
> dev_client->wait_for_tokens_timer,
>RED_CHAR_DEVICE_WAIT_TOKENS_TIMEOUT);
> @@ -332,7 +314,7 @@ static void
> red_char_device_send_msg_to_clients(RedCharDevice *dev,
> dev_client = SPICE_CONTAINEROF(item, RedCharDeviceClient, link);
> if (red_char_device_can_send_to_client(dev_client)) {
> dev_client->num_send_tokens--;
> -spice_assert(ring_is_empty(&dev_client->send_queue));
> +spice_assert(g_queue_is_empty(dev_client->send_queue));
> red_char_device_send_msg_to_client(dev, msg,
> dev_client->client);
>
> /* don't refer to dev_client anymore, it may have been released
> */
> @@ -394,21 +376,17 @@ static int
> red_char_device_read_from_device(RedCharDevice *dev)
>
> static void red_char_device_client_send_queue_push(RedCharDeviceClient
> *dev_client)
> {
> -RingItem *item;
> -while ((item = ring_get_tail(&dev_client->send_queue)) &&
> - red_char_device_can_send_to_client(dev_client)) {
> -RedCharDeviceMsgToClientItem *msg_item;
> -
> -msg_i
Re: [Spice-devel] [PATCH 07/16] char device: use 'device' rather than 'state' for var names
>
> Since the type name was changed, use variable names / debug statements
> that are consistent with the new name.
> ---
> server/char-device.c | 20 ++--
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/server/char-device.c b/server/char-device.c
> index 90eb3d4..799a818 100644
> --- a/server/char-device.c
> +++ b/server/char-device.c
> @@ -700,13 +700,13 @@ void red_char_device_write_buffer_release(RedCharDevice
> *dev,
> * char_device_state management *
> /
>
> -void red_char_device_reset_dev_instance(RedCharDevice *state,
> +void red_char_device_reset_dev_instance(RedCharDevice *dev,
> SpiceCharDeviceInstance *sin)
> {
> -spice_debug("sin %p dev_state %p", sin, state);
> -state->priv->sin = sin;
> -sin->st = state;
> -g_object_notify(G_OBJECT(state), "sin");
> +spice_debug("sin %p, char device %p", sin, dev);
> +dev->priv->sin = sin;
> +sin->st = dev;
> +g_object_notify(G_OBJECT(dev), "sin");
> }
>
> void *red_char_device_opaque_get(RedCharDevice *dev)
> @@ -771,7 +771,7 @@ int red_char_device_client_add(RedCharDevice *dev,
>
> dev->priv->wait_for_migrate_data = wait_for_migrate_data;
>
> -spice_debug("dev_state %p client %p", dev, client);
> +spice_debug("char device %p, client %p", dev, client);
> dev_client = red_char_device_client_new(client, do_flow_control,
> max_send_queue_size,
> num_client_tokens,
> @@ -789,7 +789,7 @@ void red_char_device_client_remove(RedCharDevice *dev,
> {
> RedCharDeviceClient *dev_client;
>
> -spice_debug("dev_state %p client %p", dev, client);
> +spice_debug("char device %p, client %p", dev, client);
> dev_client = red_char_device_client_find(dev, client);
>
> if (!dev_client) {
> @@ -818,7 +818,7 @@ int red_char_device_client_exists(RedCharDevice *dev,
>
> void red_char_device_start(RedCharDevice *dev)
> {
> -spice_debug("dev_state %p", dev);
> +spice_debug("char device %p", dev);
> dev->priv->running = TRUE;
> g_object_ref(dev);
> while (red_char_device_write_to_device(dev) ||
> @@ -828,7 +828,7 @@ void red_char_device_start(RedCharDevice *dev)
>
> void red_char_device_stop(RedCharDevice *dev)
> {
> -spice_debug("dev_state %p", dev);
> +spice_debug("char device %p", dev);
> dev->priv->running = FALSE;
> dev->priv->active = FALSE;
> if (dev->priv->write_to_dev_timer) {
> @@ -842,7 +842,7 @@ void red_char_device_reset(RedCharDevice *dev)
>
> red_char_device_stop(dev);
> dev->priv->wait_for_migrate_data = FALSE;
> -spice_debug("dev_state %p", dev);
> +spice_debug("char device %p", dev);
> while (!ring_is_empty(&dev->priv->write_queue)) {
> RingItem *item = ring_get_tail(&dev->priv->write_queue);
> RedCharDeviceWriteBuffer *buf;
Acked-by: Frediano Ziglio
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 06/16] char-device: notify when device instance is changed
>
> Since the device instance ("sin") is a gobject property, we should make
> sure to notify when it changes, particularly since we do some
> initialization in response to the "notify::sin" signal.
> ---
> server/char-device.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/server/char-device.c b/server/char-device.c
> index 3f20831..90eb3d4 100644
> --- a/server/char-device.c
> +++ b/server/char-device.c
> @@ -706,6 +706,7 @@ void red_char_device_reset_dev_instance(RedCharDevice
> *state,
> spice_debug("sin %p dev_state %p", sin, state);
> state->priv->sin = sin;
> sin->st = state;
> +g_object_notify(G_OBJECT(state), "sin");
> }
>
> void *red_char_device_opaque_get(RedCharDevice *dev)
> @@ -865,6 +866,7 @@ void red_char_device_reset(RedCharDevice *dev)
> red_char_device_client_send_queue_free(dev, dev_client);
> }
> dev->priv->sin = NULL;
> +g_object_notify(G_OBJECT(dev), "sin");
> }
>
> void red_char_device_wakeup(RedCharDevice *dev)
Acked-by: Frediano Ziglio
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 08/16] char device: use _reset_dev_instance() to set 'sin'
>
> Internally, use the method to set the 'sin' member variable so that we
> don't have to duplicate the g_object_notify() calls, and there are
> consistent debug statements whenever this value is modified. This also
> means that we need to handle NULL arguments to this function.
> ---
> server/char-device.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/server/char-device.c b/server/char-device.c
> index 799a818..9f5877b 100644
> --- a/server/char-device.c
> +++ b/server/char-device.c
> @@ -705,7 +705,8 @@ void red_char_device_reset_dev_instance(RedCharDevice
> *dev,
> {
> spice_debug("sin %p, char device %p", sin, dev);
> dev->priv->sin = sin;
> -sin->st = dev;
> +if (sin)
> +sin->st = dev;
> g_object_notify(G_OBJECT(dev), "sin");
> }
>
> @@ -865,8 +866,7 @@ void red_char_device_reset(RedCharDevice *dev)
> dev_client = SPICE_CONTAINEROF(client_item, RedCharDeviceClient,
> link);
> red_char_device_client_send_queue_free(dev, dev_client);
> }
> -dev->priv->sin = NULL;
> -g_object_notify(G_OBJECT(dev), "sin");
> +red_char_device_reset_dev_instance(dev, NULL);
> }
>
> void red_char_device_wakeup(RedCharDevice *dev)
> @@ -1089,7 +1089,7 @@ red_char_device_set_property(GObject *object,
> switch (property_id)
> {
> case PROP_CHAR_DEV_INSTANCE:
> -self->priv->sin = g_value_get_pointer(value);
> +red_char_device_reset_dev_instance(self,
> g_value_get_pointer(value));
> break;
> case PROP_SPICE_SERVER:
> self->priv->reds = g_value_get_pointer(value);
Acked-by: Frediano Ziglio
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] spice-xpi deprecated soon
On Tue, Apr 19, 2016 at 07:46:14PM +0530, RK RK wrote: > Thanks for your reply Christophe, I am using oVirt 3.6.3 in my production > environment with 250 Virtual Desktops accessed from within the office > premises. > > Our users will have thin client devices running trimmed down version of > CentOS 7 with spice-xpi installed via yum. > > Please let me know if you need more details on this. And do you have issues using "Native console" (.vv files) in this environment? This "deprecation" message in bugzilla has probably been there for a very long time, and was more a "would be nice" notice than some official plan ;) However, when browsers stop loading npapi plugins, I expect that we'll push strongly for switching to .vv files rather than working on an alternative extension/plugin. Christophe signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] Disable IME to allow receiving all keys
> > My understanding is, Virt-Viewer with ImmDisableIME(-1) can send key events > even if the title bar is focused when IME is enabled. > I attached the screenshot of the problem when ImmDisableIME(-1) is not > applied. > https://bugzilla.redhat.com/show_bug.cgi?id=1311858#c42 > > But spice-gtk can receives all the key events and if ImmDisableIME(-1) is > applied, users cannot switch IMEs during running Virt-Viewer. > I tested native client with Internet Explorer 10 in Windows 7 and I couldn't > see any other differences when ImmDisableIME(-1) is applied. > > If ImmDisableIME(-1) fixes only when the title bar is focused, I'd think the > customer's bug is different and Virt-Viewer without ImmDisableIME(-1) > might be more useful because it can switch IMEs during running Virt-Viewer. > I asked the customer if all the bugs can be fixed without ImmDisableIME(-1). > > Thanks, > Fujiwara > From my point of view is an improvement and fix a bug. However we are not Japanese users and perhaps we don't get your point. Can you explain a bit the user case issues having this patch in? Frediano > On 04/19/16 23:31, Fabiano Fidêncio-san wrote: > > On Tue, Apr 19, 2016 at 4:11 PM, Fabiano Fidêncio > > wrote: > >> On Tue, Apr 19, 2016 at 4:00 PM, Frediano Ziglio > >> wrote: > > On Tue, Apr 19, 2016 at 3:28 PM, Pavel Grunt wrote: > > Ack, I am going to push it. > > Do we really need this patch upstream? > It's a half-solution, at most, that doesn't work on newer Windows. > > >>> > >>> Are you referring to Window 11? It works on Windows 10. > >> > >> Hmm. I was basing my comment on > >> https://bugzilla.redhat.com/show_bug.cgi?id=1297640#c12 but I didn't > >> realize your comment > >> https://bugzilla.redhat.com/show_bug.cgi?id=1297640#c13 > >> > >> So, yeah, you have my ACK as well, but a better/more descriptive > >> commit message is needed. > >> > > > > Patch got applied already, so, nevermind. > > > >>> > >>> Frediano > >>> > Also, a better commit message would be more than appreciated in case > we really decide to have it upstream. > > > > > Thanks, > > Pavel > > > > On Mon, 2016-04-18 at 14:55 +0100, Frediano Ziglio wrote: > >> From: Christophe Fergeau > >> > >> This fixes https://bugzilla.redhat.com/show_bug.cgi?id=1297640 > >> > >> Acked-by: Frediano Ziglio > >> --- > >> src/virt-viewer-util.c | 7 +++ > >> 1 file changed, 7 insertions(+) > >> > >> diff --git a/src/virt-viewer-util.c b/src/virt-viewer-util.c > >> index 279f9a5..8cf52ec 100644 > >> --- a/src/virt-viewer-util.c > >> +++ b/src/virt-viewer-util.c > >> @@ -30,6 +30,7 @@ > >> #ifdef G_OS_WIN32 > >> #include > >> #include > >> +#include > >> #endif > >> > >> #include > >> @@ -272,6 +273,12 @@ void virt_viewer_util_init(const char *appname) > >> dup2(fileno(stdout), STDOUT_FILENO); > >> dup2(fileno(stderr), STDERR_FILENO); > >> } > >> + > >> +/* Disable input method handling so that the Zenkaku_Hankaku can > >> be passed > >> + * to VMs rather than being captured by Windows. > >> + * https://bugzilla.redhat.com/show_bug.cgi?id=1297640 > >> + */ > >> +ImmDisableIME(-1); > >> #endif > >> > >> setlocale(LC_ALL, ""); > > ___ > > Spice-devel mailing list > > Spice-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/spice-devel > > > > -- > Fabiano Fidêncio > > >> > >> > >> > >> -- > >> Fabiano Fidêncio > > > > > > > > ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [virt-tools-list] [PATCH] Disable IME to allow receiving all keys
On Wed, Apr 20, 2016 at 05:37:52PM +0900, Takao Fujiwara wrote: > My understanding is, Virt-Viewer with ImmDisableIME(-1) can send key > events even if the title bar is focused when IME is enabled. > I attached the screenshot of the problem when ImmDisableIME(-1) is not > applied. > https://bugzilla.redhat.com/show_bug.cgi?id=1311858#c42 > > But spice-gtk can receives all the key events and if ImmDisableIME(-1) is > applied, users cannot switch IMEs during running Virt-Viewer. > I tested native client with Internet Explorer 10 in Windows 7 and I > couldn't see any other differences when ImmDisableIME(-1) is applied. This patch was needed for https://bugzilla.redhat.com/show_bug.cgi?id=1297640 Setup was Windows client connecting to a Windows VM. Without this patch, when remote-viewer is focused and the user is interacting with the VM, it was not possible to press the Zenkaku_Hankaku in order to switch input method within the VM. Are you saying this is not needed, and that the situation I describe is working even without calling ImmDisableIME? Christophe signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 04/16] Use GQueue for RedCharDevice::send_queue
On Wed, 2016-04-20 at 04:38 -0400, Frediano Ziglio wrote:
> >
> > From: Christophe Fergeau
> >
> > There was an extra RedCharDeviceMsgToClientItem type whose only
> > purpose was to manage a linked list of items to send. GQueue has the
> > same purpose as this type in addition to being generic. As the length of
> > the send queue is tracked, a GQueue is more appropriate than a GList and
> > allow to remove RedCharDevice::send_queue_size.
> > ---
> > Changes since last version:
> > - red_char_device_client_send_queue_free(): update tokens before clearing
> > queue
> > - red_char_device_client_send_queue_push(): change GList* variable to
> > PipeItem*
> >
> > server/char-device.c | 69
> > ++--
> > 1 file changed, 23 insertions(+), 46 deletions(-)
> >
> > diff --git a/server/char-device.c b/server/char-device.c
> > index f3e16da..2206c21 100644
> > --- a/server/char-device.c
> > +++ b/server/char-device.c
> > @@ -41,8 +41,7 @@ struct RedCharDeviceClient {
> > uint64_t num_send_tokens; /* send to client */
> > SpiceTimer *wait_for_tokens_timer;
> > int wait_for_tokens_started;
> > -Ring send_queue;
> > -uint32_t send_queue_size;
> > +GQueue *send_queue;
> > uint32_t max_send_queue_size;
> > };
> >
> > @@ -105,11 +104,6 @@ static guint signals[RED_CHAR_DEVICE_LAST_SIGNAL];
> > static void red_char_device_write_buffer_unref(RedCharDeviceWriteBuffer
> > *write_buf);
> > static void red_char_device_write_retry(void *opaque);
> >
> > -typedef struct RedCharDeviceMsgToClientItem {
> > -RingItem link;
> > -PipeItem *msg;
> > -} RedCharDeviceMsgToClientItem;
> > -
> > static PipeItem *
> > red_char_device_read_one_msg_from_device(RedCharDevice *dev)
> > {
> > @@ -187,19 +181,10 @@ static void
> > red_char_device_write_buffer_pool_add(RedCharDevice *dev,
> > static void red_char_device_client_send_queue_free(RedCharDevice *dev,
> > RedCharDeviceClient
> > *dev_client)
> > {
> > -spice_debug("send_queue_empty %d",
> > ring_is_empty(&dev_client->send_queue));
> > -while (!ring_is_empty(&dev_client->send_queue)) {
> > -RingItem *item = ring_get_tail(&dev_client->send_queue);
> > -RedCharDeviceMsgToClientItem *msg_item = SPICE_CONTAINEROF(item,
> > -
> > RedCharDeviceMsgToClientItem,
> > - link);
> > -
> > -ring_remove(item);
> > -pipe_item_unref(msg_item->msg);
> > -free(msg_item);
> > -}
> > -dev_client->num_send_tokens += dev_client->send_queue_size;
> > -dev_client->send_queue_size = 0;
> > +spice_debug("send_queue_empty %d",
> > g_queue_is_empty(dev_client->send_queue));
> > +dev_client->num_send_tokens +=
> > g_queue_get_length(dev_client->send_queue);
> > +g_queue_free_full(dev_client->send_queue, pipe_item_unref);
> > +g_queue_clear(dev_client->send_queue);
> > }
> >
> > static void red_char_device_client_free(RedCharDevice *dev,
> > @@ -303,17 +288,14 @@ static void
> > red_char_device_add_msg_to_client_queue(RedCharDeviceClient *dev_cli
> > PipeItem *msg)
> > {
> > RedCharDevice *dev = dev_client->dev;
> > -RedCharDeviceMsgToClientItem *msg_item;
> >
> > -if (dev_client->send_queue_size >= dev_client->max_send_queue_size) {
> > +if (g_queue_get_length(dev_client->send_queue) >=
> > dev_client->max_send_queue_size) {
> > red_char_device_handle_client_overflow(dev_client);
> > return;
> > }
> >
> > -msg_item = spice_new0(RedCharDeviceMsgToClientItem, 1);
> > -msg_item->msg = pipe_item_ref(msg);
> > -ring_add(&dev_client->send_queue, &msg_item->link);
> > -dev_client->send_queue_size++;
> > +pipe_item_ref(msg);
> > +g_queue_push_head(dev_client->send_queue, msg);
> > if (!dev_client->wait_for_tokens_started) {
> > reds_core_timer_start(dev->priv->reds,
> > dev_client->wait_for_tokens_timer,
> >RED_CHAR_DEVICE_WAIT_TOKENS_TIMEOUT);
> > @@ -332,7 +314,7 @@ static void
> > red_char_device_send_msg_to_clients(RedCharDevice *dev,
> > dev_client = SPICE_CONTAINEROF(item, RedCharDeviceClient, link);
> > if (red_char_device_can_send_to_client(dev_client)) {
> > dev_client->num_send_tokens--;
> > -spice_assert(ring_is_empty(&dev_client->send_queue));
> > +spice_assert(g_queue_is_empty(dev_client->send_queue));
> > red_char_device_send_msg_to_client(dev, msg,
> > dev_client->client);
> >
> > /* don't refer to dev_client anymore, it may have been released
> > */
> > @@ -394,21 +376,17 @@ static int
> > red_char_device_read_from_device(RedCharDevice *dev)
> >
> > static void red_char_devi
Re: [Spice-devel] [PATCH 14/20] Use GQueue for RedCharDevice::send_queue
On Po, 2016-04-18 at 05:17 -0400, Frediano Ziglio wrote:
> > On Thu, 2016-04-14 at 16:50 -0500, Jonathon Jongsma wrote:
> > > From: Christophe Fergeau
> > >
> > > There was an extra RedCharDeviceMsgToClientItem type whose only
> > > purpose was to manage a linked list of items to send. GQueue has the
> > > same purpose as this type in addition to being generic. As the length of
> > > the send queue is tracked, a GQueue is more appropriate than a GList and
> > > allow to remove RedCharDevice::send_queue_size.
> > > ---
> > > server/char-device.c | 69
> > > ++-
> > > -
> > > 1 file changed, 23 insertions(+), 46 deletions(-)
> > >
> > > diff --git a/server/char-device.c b/server/char-device.c
> > > index f3e16da..6b9596e 100644
> > > --- a/server/char-device.c
> > > +++ b/server/char-device.c
> > > @@ -41,8 +41,7 @@ struct RedCharDeviceClient {
> > > uint64_t num_send_tokens; /* send to client */
> > > SpiceTimer *wait_for_tokens_timer;
> > > int wait_for_tokens_started;
> > > -Ring send_queue;
> > > -uint32_t send_queue_size;
> > > +GQueue *send_queue;
> > > uint32_t max_send_queue_size;
> > > };
> > >
> > > @@ -105,11 +104,6 @@ static guint signals[RED_CHAR_DEVICE_LAST_SIGNAL];
> > > static void red_char_device_write_buffer_unref(RedCharDeviceWriteBuffer
> > > *write_buf);
> > > static void red_char_device_write_retry(void *opaque);
> > >
> > > -typedef struct RedCharDeviceMsgToClientItem {
> > > -RingItem link;
> > > -PipeItem *msg;
> > > -} RedCharDeviceMsgToClientItem;
> > > -
> > > static PipeItem *
> > > red_char_device_read_one_msg_from_device(RedCharDevice *dev)
> > > {
> > > @@ -187,19 +181,10 @@ static void
> > > red_char_device_write_buffer_pool_add(RedCharDevice *dev,
> > > static void red_char_device_client_send_queue_free(RedCharDevice *dev,
> > > RedCharDeviceClient
> > > *dev_client)
> > > {
> > > -spice_debug("send_queue_empty %d", ring_is_empty(&dev_client
> > > ->send_queue));
> > > -while (!ring_is_empty(&dev_client->send_queue)) {
> > > -RingItem *item = ring_get_tail(&dev_client->send_queue);
> > > -RedCharDeviceMsgToClientItem *msg_item = SPICE_CONTAINEROF(item,
> > > -
> > > RedCharDeviceMsgToClientItem,
> > > - link);
> > > -
> > > -ring_remove(item);
> > > -pipe_item_unref(msg_item->msg);
> > > -free(msg_item);
> > > -}
> > > -dev_client->num_send_tokens += dev_client->send_queue_size;
> > > -dev_client->send_queue_size = 0;
> > > +spice_debug("send_queue_empty %d", g_queue_is_empty(dev_client
> > > ->send_queue));
> > > +g_queue_free_full(dev_client->send_queue, pipe_item_unref);
> > > +g_queue_clear(dev_client->send_queue);
> > > +dev_client->num_send_tokens += g_queue_get_length(dev_client
> > > ->send_queue);
> >
> > This looks wrong. We're calling g_queue_get_length() immediately after
> > clearing
> > the queue, so it's guaranteed to be 0. This last line should be moved before
> > the
> > g_queue_free_full() call.
> >
> > > }
> > >
> > > static void red_char_device_client_free(RedCharDevice *dev,
> > > @@ -303,17 +288,14 @@ static void
> > > red_char_device_add_msg_to_client_queue(RedCharDeviceClient *dev_cli
> > > PipeItem *msg)
> > > {
> > > RedCharDevice *dev = dev_client->dev;
> > > -RedCharDeviceMsgToClientItem *msg_item;
> > >
> > > -if (dev_client->send_queue_size >= dev_client->max_send_queue_size) {
> > > +if (g_queue_get_length(dev_client->send_queue) >= dev_client
> > > ->max_send_queue_size) {
> > > red_char_device_handle_client_overflow(dev_client);
> > > return;
> > > }
> > >
> > > -msg_item = spice_new0(RedCharDeviceMsgToClientItem, 1);
> > > -msg_item->msg = pipe_item_ref(msg);
> > > -ring_add(&dev_client->send_queue, &msg_item->link);
> > > -dev_client->send_queue_size++;
> > > +pipe_item_ref(msg);
> > > +g_queue_push_head(dev_client->send_queue, msg);
> > > if (!dev_client->wait_for_tokens_started) {
> > > reds_core_timer_start(dev->priv->reds, dev_client
> > > ->wait_for_tokens_timer,
> > >RED_CHAR_DEVICE_WAIT_TOKENS_TIMEOUT);
> > > @@ -332,7 +314,7 @@ static void
> > > red_char_device_send_msg_to_clients(RedCharDevice *dev,
> > > dev_client = SPICE_CONTAINEROF(item, RedCharDeviceClient, link);
> > > if (red_char_device_can_send_to_client(dev_client)) {
> > > dev_client->num_send_tokens--;
> > > -spice_assert(ring_is_empty(&dev_client->send_queue));
> > > +spice_assert(g_queue_is_empty(dev_client->send_queue));
> > > red_char_device_send_msg_to_client(dev, msg,
> > > dev_client->client);
> > >
> > >
Re: [Spice-devel] [virt-tools-list] [PATCH] Disable IME to allow receiving all keys
> > On Wed, Apr 20, 2016 at 05:37:52PM +0900, Takao Fujiwara wrote: > > My understanding is, Virt-Viewer with ImmDisableIME(-1) can send key > > events even if the title bar is focused when IME is enabled. > > I attached the screenshot of the problem when ImmDisableIME(-1) is not > > applied. > > https://bugzilla.redhat.com/show_bug.cgi?id=1311858#c42 > > > > But spice-gtk can receives all the key events and if ImmDisableIME(-1) is > > applied, users cannot switch IMEs during running Virt-Viewer. > > I tested native client with Internet Explorer 10 in Windows 7 and I > > couldn't see any other differences when ImmDisableIME(-1) is applied. > > This patch was needed for > https://bugzilla.redhat.com/show_bug.cgi?id=1297640 > Setup was Windows client connecting to a Windows VM. > Without this patch, when remote-viewer is focused and the user is > interacting with the VM, it was not possible to press the > Zenkaku_Hankaku in order to switch input method within the VM. > > Are you saying this is not needed, and that the situation I describe is > working > even without calling ImmDisableIME? > > Christophe > From the subject looks like multiple keys (not specified) are not working, one is Zenkaku_Hankaku. Can we have a list of not working keys? In this case the problem is different as the key is managed by Windows and not sent to application. I also noted that an issue happen when you press the title of the remote-viewer window and continue using the keyboard. In this case when you are in Japanese mode you don't receive keys events but a small window is opened where characters are typed. Do we want to have this behavior? Frediano ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 02/16] Replace RedCharDevice::on_free_self_token with a signal
Hey, On Wed, Apr 20, 2016 at 04:29:56AM -0400, Frediano Ziglio wrote: > > > > From: Christophe Fergeau > > > > It's more natural to do things this way with glib, and this allows to > > remove some coupling between Reds and RedCharDeviceVDIPort. Before this > > commit, the RedCharDeviceVDIPort has to provide a on_free_self_token() > > because Reds needs to know about it. With a signal, RedCharDeviceVDIPort > > does not have to do anything special, and Reds can just connect to the > > signal it's interested in. > > --- > > Frediano didn't like the signal approach. I think it's OK. Still needs > > resolution. > > Let me clarify this, I think there are some pending issues/comments > > 1) signal and callback > This patch change the callback and use a glib signal instead. I don't > like the dynamic binding and I prefer previous static one but Christophe > and Jonathon prefer the signal (which is dynamic) and we are moving to > the unsafe world of glib so it's not a problem > > 2) reasoning > Moving from callback to signal does not remove the dependency, the > dependency is just changed to a signal, specifically > " this allows to remove some coupling between Reds and RedCharDeviceVDIPort" > no, you just changed char_dev_class->on_free_self_token assignment > to a g_signal_connect. The reasoning behind this coupling comment is that reds.c wants to do some housekeeping when a token is released in a char device. It seems odd to have the notification needs of something external to the RedCharDevice mandate the addition of a vfunc in the base RedCharDevice class. And moving the on_free_self_token assignment to a g_signal_connect gives us more flexibility, it can be done by external code, it no longer has to be done in the implementation of the class we are interested in. With that said, I looked a bit more closely at that code, and "on_free_self_token" is not about getting resource released, but more about feeding more data to the char device now that it's ok to do so. So yeah, maybe it would make sense to do things a bit differently. Maybe a more explicit API for token handling (grouping all the token stuff in a struct, having a few methods to consume/release a token, ...) would make more sense, in which case we could register a handler to feed more data into the device when needed. But really, a vfunc called on_xxx sounds to me like some code external to the class needed to be notified of something, and in the absence of a non-invasive mechanism to do these notifications, we fell back on hardcoding this requirement on the class itself. I prefer to have the class notifies in a generic way about this, and interested consumer to be able to catch it if needed. > 3) why the single callback was changed to multiple signals? > (this was raised by Jonathon) Probably for symmetry even if it's unused. The additional signal can be dropped. Christophe signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 04/16] Use GQueue for RedCharDevice::send_queue
On Wed, 2016-04-20 at 09:48 -0500, Jonathon Jongsma wrote:
> On Wed, 2016-04-20 at 04:38 -0400, Frediano Ziglio wrote:
> > >
> > > From: Christophe Fergeau
> > >
> > > There was an extra RedCharDeviceMsgToClientItem type whose only
> > > purpose was to manage a linked list of items to send. GQueue has the
> > > same purpose as this type in addition to being generic. As the length of
> > > the send queue is tracked, a GQueue is more appropriate than a GList and
> > > allow to remove RedCharDevice::send_queue_size.
> > > ---
> > > Changes since last version:
> > > - red_char_device_client_send_queue_free(): update tokens before clearing
> > > queue
> > > - red_char_device_client_send_queue_push(): change GList* variable to
> > > PipeItem*
> > >
> > > server/char-device.c | 69
> > > ++--
> > > 1 file changed, 23 insertions(+), 46 deletions(-)
> > >
> > > diff --git a/server/char-device.c b/server/char-device.c
> > > index f3e16da..2206c21 100644
> > > --- a/server/char-device.c
> > > +++ b/server/char-device.c
> > > @@ -41,8 +41,7 @@ struct RedCharDeviceClient {
> > > uint64_t num_send_tokens; /* send to client */
> > > SpiceTimer *wait_for_tokens_timer;
> > > int wait_for_tokens_started;
> > > -Ring send_queue;
> > > -uint32_t send_queue_size;
> > > +GQueue *send_queue;
> > > uint32_t max_send_queue_size;
> > > };
> > >
> > > @@ -105,11 +104,6 @@ static guint signals[RED_CHAR_DEVICE_LAST_SIGNAL];
> > > static void red_char_device_write_buffer_unref(RedCharDeviceWriteBuffer
> > > *write_buf);
> > > static void red_char_device_write_retry(void *opaque);
> > >
> > > -typedef struct RedCharDeviceMsgToClientItem {
> > > -RingItem link;
> > > -PipeItem *msg;
> > > -} RedCharDeviceMsgToClientItem;
> > > -
> > > static PipeItem *
> > > red_char_device_read_one_msg_from_device(RedCharDevice *dev)
> > > {
> > > @@ -187,19 +181,10 @@ static void
> > > red_char_device_write_buffer_pool_add(RedCharDevice *dev,
> > > static void red_char_device_client_send_queue_free(RedCharDevice *dev,
> > > RedCharDeviceClient
> > > *dev_client)
> > > {
> > > -spice_debug("send_queue_empty %d",
> > > ring_is_empty(&dev_client->send_queue));
> > > -while (!ring_is_empty(&dev_client->send_queue)) {
> > > -RingItem *item = ring_get_tail(&dev_client->send_queue);
> > > -RedCharDeviceMsgToClientItem *msg_item = SPICE_CONTAINEROF(item,
> > > -
> > > RedCharDeviceMsgToClientItem,
> > > - link);
> > > -
> > > -ring_remove(item);
> > > -pipe_item_unref(msg_item->msg);
> > > -free(msg_item);
> > > -}
> > > -dev_client->num_send_tokens += dev_client->send_queue_size;
> > > -dev_client->send_queue_size = 0;
> > > +spice_debug("send_queue_empty %d",
> > > g_queue_is_empty(dev_client->send_queue));
> > > +dev_client->num_send_tokens +=
> > > g_queue_get_length(dev_client->send_queue);
> > > +g_queue_free_full(dev_client->send_queue, pipe_item_unref);
> > > +g_queue_clear(dev_client->send_queue);
> > > }
> > >
> > > static void red_char_device_client_free(RedCharDevice *dev,
> > > @@ -303,17 +288,14 @@ static void
> > > red_char_device_add_msg_to_client_queue(RedCharDeviceClient *dev_cli
> > > PipeItem *msg)
> > > {
> > > RedCharDevice *dev = dev_client->dev;
> > > -RedCharDeviceMsgToClientItem *msg_item;
> > >
> > > -if (dev_client->send_queue_size >= dev_client->max_send_queue_size) {
> > > +if (g_queue_get_length(dev_client->send_queue) >=
> > > dev_client->max_send_queue_size) {
> > > red_char_device_handle_client_overflow(dev_client);
> > > return;
> > > }
> > >
> > > -msg_item = spice_new0(RedCharDeviceMsgToClientItem, 1);
> > > -msg_item->msg = pipe_item_ref(msg);
> > > -ring_add(&dev_client->send_queue, &msg_item->link);
> > > -dev_client->send_queue_size++;
> > > +pipe_item_ref(msg);
> > > +g_queue_push_head(dev_client->send_queue, msg);
> > > if (!dev_client->wait_for_tokens_started) {
> > > reds_core_timer_start(dev->priv->reds,
> > > dev_client->wait_for_tokens_timer,
> > >RED_CHAR_DEVICE_WAIT_TOKENS_TIMEOUT);
> > > @@ -332,7 +314,7 @@ static void
> > > red_char_device_send_msg_to_clients(RedCharDevice *dev,
> > > dev_client = SPICE_CONTAINEROF(item, RedCharDeviceClient, link);
> > > if (red_char_device_can_send_to_client(dev_client)) {
> > > dev_client->num_send_tokens--;
> > > -spice_assert(ring_is_empty(&dev_client->send_queue));
> > > +spice_assert(g_queue_is_empty(dev_client->send_queue));
> > > red_char_device_send_msg_to_client(dev, msg
[Spice-devel] [PATCH v2] Use GQueue for RedCharDevice::send_queue
From: Christophe Fergeau
There was an extra RedCharDeviceMsgToClientItem type whose only
purpose was to manage a linked list of items to send. GQueue has the
same purpose as this type in addition to being generic. As the length of
the send queue is tracked, a GQueue is more appropriate than a GList and
allow to remove RedCharDevice::send_queue_size.
---
server/char-device.c | 72 +++-
1 file changed, 20 insertions(+), 52 deletions(-)
diff --git a/server/char-device.c b/server/char-device.c
index f3e16da..46036d8 100644
--- a/server/char-device.c
+++ b/server/char-device.c
@@ -41,8 +41,7 @@ struct RedCharDeviceClient {
uint64_t num_send_tokens; /* send to client */
SpiceTimer *wait_for_tokens_timer;
int wait_for_tokens_started;
-Ring send_queue;
-uint32_t send_queue_size;
+GQueue *send_queue;
uint32_t max_send_queue_size;
};
@@ -105,11 +104,6 @@ static guint signals[RED_CHAR_DEVICE_LAST_SIGNAL];
static void red_char_device_write_buffer_unref(RedCharDeviceWriteBuffer
*write_buf);
static void red_char_device_write_retry(void *opaque);
-typedef struct RedCharDeviceMsgToClientItem {
-RingItem link;
-PipeItem *msg;
-} RedCharDeviceMsgToClientItem;
-
static PipeItem *
red_char_device_read_one_msg_from_device(RedCharDevice *dev)
{
@@ -184,24 +178,6 @@ static void
red_char_device_write_buffer_pool_add(RedCharDevice *dev,
red_char_device_write_buffer_unref(buf);
}
-static void red_char_device_client_send_queue_free(RedCharDevice *dev,
- RedCharDeviceClient
*dev_client)
-{
-spice_debug("send_queue_empty %d", ring_is_empty(&dev_client->send_queue));
-while (!ring_is_empty(&dev_client->send_queue)) {
-RingItem *item = ring_get_tail(&dev_client->send_queue);
-RedCharDeviceMsgToClientItem *msg_item = SPICE_CONTAINEROF(item,
-
RedCharDeviceMsgToClientItem,
- link);
-
-ring_remove(item);
-pipe_item_unref(msg_item->msg);
-free(msg_item);
-}
-dev_client->num_send_tokens += dev_client->send_queue_size;
-dev_client->send_queue_size = 0;
-}
-
static void red_char_device_client_free(RedCharDevice *dev,
RedCharDeviceClient *dev_client)
{
@@ -212,7 +188,7 @@ static void red_char_device_client_free(RedCharDevice *dev,
dev_client->wait_for_tokens_timer = NULL;
}
-red_char_device_client_send_queue_free(dev, dev_client);
+g_queue_free_full(dev_client->send_queue, pipe_item_unref);
/* remove write buffers that are associated with the client */
spice_debug("write_queue_is_empty %d",
ring_is_empty(&dev->priv->write_queue) && !dev->priv->cur_write_buf);
@@ -303,17 +279,14 @@ static void
red_char_device_add_msg_to_client_queue(RedCharDeviceClient *dev_cli
PipeItem *msg)
{
RedCharDevice *dev = dev_client->dev;
-RedCharDeviceMsgToClientItem *msg_item;
-if (dev_client->send_queue_size >= dev_client->max_send_queue_size) {
+if (g_queue_get_length(dev_client->send_queue) >=
dev_client->max_send_queue_size) {
red_char_device_handle_client_overflow(dev_client);
return;
}
-msg_item = spice_new0(RedCharDeviceMsgToClientItem, 1);
-msg_item->msg = pipe_item_ref(msg);
-ring_add(&dev_client->send_queue, &msg_item->link);
-dev_client->send_queue_size++;
+pipe_item_ref(msg);
+g_queue_push_head(dev_client->send_queue, msg);
if (!dev_client->wait_for_tokens_started) {
reds_core_timer_start(dev->priv->reds,
dev_client->wait_for_tokens_timer,
RED_CHAR_DEVICE_WAIT_TOKENS_TIMEOUT);
@@ -332,7 +305,7 @@ static void
red_char_device_send_msg_to_clients(RedCharDevice *dev,
dev_client = SPICE_CONTAINEROF(item, RedCharDeviceClient, link);
if (red_char_device_can_send_to_client(dev_client)) {
dev_client->num_send_tokens--;
-spice_assert(ring_is_empty(&dev_client->send_queue));
+spice_assert(g_queue_is_empty(dev_client->send_queue));
red_char_device_send_msg_to_client(dev, msg, dev_client->client);
/* don't refer to dev_client anymore, it may have been released */
@@ -394,21 +367,14 @@ static int red_char_device_read_from_device(RedCharDevice
*dev)
static void red_char_device_client_send_queue_push(RedCharDeviceClient
*dev_client)
{
-RingItem *item;
-while ((item = ring_get_tail(&dev_client->send_queue)) &&
+while (!g_queue_is_empty(dev_client->send_queue) &&
red_char_device_can_send_to_client(dev_client)) {
-RedCharDeviceMsgToClientItem *msg_item;
-
-msg_item = SPICE_CONTAINEROF(item, RedCharDeviceMsgToClientItem, link);
-ring_remove(item
Re: [Spice-devel] [PATCH 01/16] UpgradeItem: use base PipeItem for refcounting
On Wed, 2016-04-20 at 04:18 -0400, Frediano Ziglio wrote:
> >
> > No need to re-implement refcounting in this subclass. However, I needed
> > to add a new 'dcc' member to UpgradeItem to be able to unref properly.
> > ---
> > Change since last patch: take ref on dcc
> >
> > server/dcc.c | 18 +-
> > server/display-channel.c | 4 +---
> > server/display-channel.h | 2 +-
> > server/stream.c | 18 --
> > 4 files changed, 19 insertions(+), 23 deletions(-)
> >
> > diff --git a/server/dcc.c b/server/dcc.c
> > index 91c3f82..5193c17 100644
> > --- a/server/dcc.c
> > +++ b/server/dcc.c
> > @@ -1591,29 +1591,15 @@ int dcc_handle_migrate_data(DisplayChannelClient
> > *dcc, uint32_t size, void *mess
> > return TRUE;
> > }
> >
> > -static void upgrade_item_unref(DisplayChannel *display, UpgradeItem *item)
> > -{
> > -if (--item->refs != 0)
> > -return;
> > -
> > -display_channel_drawable_unref(display, item->drawable);
> > -free(item->rects);
> > -free(item);
> > -}
> > -
> > static void release_item_after_push(DisplayChannelClient *dcc, PipeItem
> > *item)
> > {
> > -DisplayChannel *display = DCC_TO_DC(dcc);
> > -
> > switch (item->type) {
> > case PIPE_ITEM_TYPE_DRAW:
> > case PIPE_ITEM_TYPE_IMAGE:
> > case PIPE_ITEM_TYPE_STREAM_CLIP:
> > case PIPE_ITEM_TYPE_MONITORS_CONFIG:
> > -pipe_item_unref(item);
> > -break;
> > case PIPE_ITEM_TYPE_UPGRADE:
> > -upgrade_item_unref(display, (UpgradeItem *)item);
> > +pipe_item_unref(item);
> > break;
> > case PIPE_ITEM_TYPE_GL_SCANOUT:
> > case PIPE_ITEM_TYPE_GL_DRAW:
> > @@ -1651,8 +1637,6 @@ static void
> > release_item_before_push(DisplayChannelClient *dcc, PipeItem *item)
> > }
> > case PIPE_ITEM_TYPE_STREAM_CLIP:
> > case PIPE_ITEM_TYPE_UPGRADE:
> > -upgrade_item_unref(display, (UpgradeItem *)item);
> > -break;
> > case PIPE_ITEM_TYPE_IMAGE:
> > case PIPE_ITEM_TYPE_MONITORS_CONFIG:
> > pipe_item_unref(item);
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index 88dbc74..98b4a43 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -1973,10 +1973,8 @@ static void hold_item(RedChannelClient *rcc, PipeItem
> > *item)
> > case PIPE_ITEM_TYPE_DRAW:
> > case PIPE_ITEM_TYPE_IMAGE:
> > case PIPE_ITEM_TYPE_STREAM_CLIP:
> > -pipe_item_ref(item);
> > -break;
> > case PIPE_ITEM_TYPE_UPGRADE:
> > -((UpgradeItem *)item)->refs++;
> > +pipe_item_ref(item);
> > break;
> > default:
> > spice_warn_if_reached();
> > diff --git a/server/display-channel.h b/server/display-channel.h
> > index 6b053de..8944f06 100644
> > --- a/server/display-channel.h
> > +++ b/server/display-channel.h
> > @@ -244,7 +244,7 @@ typedef struct SurfaceDestroyItem {
> >
> > typedef struct UpgradeItem {
> > PipeItem base;
> > -int refs;
> > +DisplayChannelClient *dcc;
>
> Here a display channel is enough I think.
>
> > Drawable *drawable;
> > SpiceClipRects *rects;
> > } UpgradeItem;
> > diff --git a/server/stream.c b/server/stream.c
> > index ae37a62..80cfe2e 100644
> > --- a/server/stream.c
> > +++ b/server/stream.c
> > @@ -758,6 +758,18 @@ void stream_agent_stop(StreamAgent *agent)
> > }
> > }
> >
> > +static void upgrade_item_free(UpgradeItem *item)
> > +{
> > +g_return_if_fail(item != NULL);
> > +DisplayChannel *display = DCC_TO_DC(item->dcc);
> > +g_return_if_fail(item->base.refcount != 0);
> > +
> > +display_channel_drawable_unref(display, item->drawable);
>
> I'm still pondering this.
> My initial question was mainly a question (even to me) than a
> proposal. Here the problem is that this can create a circular
> reference counting. The problem is quite hidden by the fact that
> we don't still have a way to free part of our stuff (DisplayChannel
> is one). This make hard to verify this, I should have pushed the
> inclusion of my cleanup branch when was refused.
>
> Jonathon... did you test for leaks?
No I didn't. I did just now and didn't notice any leaks. But you're right that
it basically creates a circular ref, which could be problematic. It would
probably be better to use a weak ref, although I can't do until after the
GObject conversions have been merged. What if I remove the ref and add a FIXME
to use a weak ref in the future?
>
> > +free(item->rects);
> > +red_channel_client_unref(RED_CHANNEL_CLIENT(item->dcc));
> > +free(item);
> > +}
> > +
> > /*
> > * after dcc_detach_stream_gracefully is called for all the display channel
> > clients,
> > * detach_stream should be called. See comment (1).
> > @@ -798,10 +810,12 @@ static void
> > dcc_detach_stream_gracefully(DisplayChannelClient *dcc,
> > rect_debug(&stream->current->red_drawable->bbox);
> > rcc = RED_CHAN
Re: [Spice-devel] [PATCH] Disable IME to allow receiving all keys
On 04/20/16 23:29, Frediano Ziglio-san wrote: My understanding is, Virt-Viewer with ImmDisableIME(-1) can send key events even if the title bar is focused when IME is enabled. I attached the screenshot of the problem when ImmDisableIME(-1) is not applied. https://bugzilla.redhat.com/show_bug.cgi?id=1311858#c42 But spice-gtk can receives all the key events and if ImmDisableIME(-1) is applied, users cannot switch IMEs during running Virt-Viewer. I tested native client with Internet Explorer 10 in Windows 7 and I couldn't see any other differences when ImmDisableIME(-1) is applied. If ImmDisableIME(-1) fixes only when the title bar is focused, I'd think the customer's bug is different and Virt-Viewer without ImmDisableIME(-1) might be more useful because it can switch IMEs during running Virt-Viewer. I asked the customer if all the bugs can be fixed without ImmDisableIME(-1). Thanks, Fujiwara From my point of view is an improvement and fix a bug. However we are not Japanese users and perhaps we don't get your point. Can you explain a bit the user case issues having this patch in? If the patch is applied, when the user launch remote-viewer.exe, Windows does not show the keyboard status icon on the Windows task bar and users are hard to find which keyboard is used with remote-viewer.exe. Thanks, Fujiwara Frediano On 04/19/16 23:31, Fabiano Fidêncio-san wrote: On Tue, Apr 19, 2016 at 4:11 PM, Fabiano Fidêncio wrote: On Tue, Apr 19, 2016 at 4:00 PM, Frediano Ziglio wrote: On Tue, Apr 19, 2016 at 3:28 PM, Pavel Grunt wrote: Ack, I am going to push it. Do we really need this patch upstream? It's a half-solution, at most, that doesn't work on newer Windows. Are you referring to Window 11? It works on Windows 10. Hmm. I was basing my comment on https://bugzilla.redhat.com/show_bug.cgi?id=1297640#c12 but I didn't realize your comment https://bugzilla.redhat.com/show_bug.cgi?id=1297640#c13 So, yeah, you have my ACK as well, but a better/more descriptive commit message is needed. Patch got applied already, so, nevermind. Frediano Also, a better commit message would be more than appreciated in case we really decide to have it upstream. Thanks, Pavel On Mon, 2016-04-18 at 14:55 +0100, Frediano Ziglio wrote: From: Christophe Fergeau This fixes https://bugzilla.redhat.com/show_bug.cgi?id=1297640 Acked-by: Frediano Ziglio --- src/virt-viewer-util.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/src/virt-viewer-util.c b/src/virt-viewer-util.c index 279f9a5..8cf52ec 100644 --- a/src/virt-viewer-util.c +++ b/src/virt-viewer-util.c @@ -30,6 +30,7 @@ #ifdef G_OS_WIN32 #include #include +#include #endif #include @@ -272,6 +273,12 @@ void virt_viewer_util_init(const char *appname) dup2(fileno(stdout), STDOUT_FILENO); dup2(fileno(stderr), STDERR_FILENO); } + +/* Disable input method handling so that the Zenkaku_Hankaku can be passed + * to VMs rather than being captured by Windows. + * https://bugzilla.redhat.com/show_bug.cgi?id=1297640 + */ +ImmDisableIME(-1); #endif setlocale(LC_ALL, ""); ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel -- Fabiano Fidêncio -- Fabiano Fidêncio ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [virt-tools-list] [PATCH] Disable IME to allow receiving all keys
On 04/20/16 23:45, Christophe Fergeau-san wrote: On Wed, Apr 20, 2016 at 05:37:52PM +0900, Takao Fujiwara wrote: My understanding is, Virt-Viewer with ImmDisableIME(-1) can send key events even if the title bar is focused when IME is enabled. I attached the screenshot of the problem when ImmDisableIME(-1) is not applied. https://bugzilla.redhat.com/show_bug.cgi?id=1311858#c42 But spice-gtk can receives all the key events and if ImmDisableIME(-1) is applied, users cannot switch IMEs during running Virt-Viewer. I tested native client with Internet Explorer 10 in Windows 7 and I couldn't see any other differences when ImmDisableIME(-1) is applied. This patch was needed for https://bugzilla.redhat.com/show_bug.cgi?id=1297640 Setup was Windows client connecting to a Windows VM. Without this patch, when remote-viewer is focused and the user is interacting with the VM, it was not possible to press the Zenkaku_Hankaku in order to switch input method within the VM. Are you saying this is not needed, and that the situation I describe is working even without calling ImmDisableIME? I guess the patch does not fix 1297640. I mean whether ImmDisableIME(-1) is called or not, Zenkaku_Hankaku is sent to spice-gtk with my test. I think the root cause is that MapVirtualKey() cannot get the scancode of the released Zenkaku_Hankaku and the remote desktop cannot receive Zenkaku_Hankaku. I observed when the title bar of remote-viewer is focused, Zenkaku_Hankaku event is not sent without this patch. But I guess the original submitter mistook the issue and the customer's issue would be that Zenkaku_Hankaku is not sent when the window of remote-viewer is focused but not the title bar. And I asked the customers if all the problems are fixed with the patched spice-gtk and without ImmDisableIME(-1). Thanks, Fujiwara Christophe ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [virt-tools-list] [PATCH] Disable IME to allow receiving all keys
On 04/21/16 01:17, Frediano Ziglio-san wrote: On Wed, Apr 20, 2016 at 05:37:52PM +0900, Takao Fujiwara wrote: My understanding is, Virt-Viewer with ImmDisableIME(-1) can send key events even if the title bar is focused when IME is enabled. I attached the screenshot of the problem when ImmDisableIME(-1) is not applied. https://bugzilla.redhat.com/show_bug.cgi?id=1311858#c42 But spice-gtk can receives all the key events and if ImmDisableIME(-1) is applied, users cannot switch IMEs during running Virt-Viewer. I tested native client with Internet Explorer 10 in Windows 7 and I couldn't see any other differences when ImmDisableIME(-1) is applied. This patch was needed for https://bugzilla.redhat.com/show_bug.cgi?id=1297640 Setup was Windows client connecting to a Windows VM. Without this patch, when remote-viewer is focused and the user is interacting with the VM, it was not possible to press the Zenkaku_Hankaku in order to switch input method within the VM. Are you saying this is not needed, and that the situation I describe is working even without calling ImmDisableIME? Christophe From the subject looks like multiple keys (not specified) are not working, one is Zenkaku_Hankaku. Can we have a list of not working keys? spice-gtk can receives Zenkaku_Hankaku events without the patch of ImmDisableIME(-1). I didn't find any problems without ImmDisableIME(-1) with the native client from Internet Explorer 10 in Windows 7 and either MS-IME or MS Office IME. In this case the problem is different as the key is managed by Windows and not sent to application. I also noted that an issue happen when you press the title of the remote-viewer window and continue using the keyboard. In this case when you are in Japanese mode you don't receive keys events but a small window is opened where characters are typed. Do we want to have this behavior? I think this is a specification of Windows and users don't mind the issue. E.g. users can send IME events to the title bar of explorer.exe I think the customer real issue is MapVirtualKey() in spice-gtk and I guess bug 1297640 didn't describe the accurate problem. I also think showing keyboard status on Windows task bar is useful for the users. Thanks, Fujiwara Frediano ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
