Re: [Spice-devel] [vdagent-linux v1] systemd-login: check for LockedHint property
Hi,
On Thu, Jun 02, 2016 at 07:43:25PM +0200, Pavel Grunt wrote:
> Hi,
>
> On Fri, 2016-05-27 at 11:42 +0200, Victor Toso wrote:
> > Property introduced in v230 of systemd.
> >
> > Systems that don't have up to date systemd-login will get the
> > following log message:
> >
> > "Properties.Get failed: Unknown property or interface."
> Can we give more info (which property is unknown) ? To make it easier for
> users.
> The property is new, so almost everyone will see the message in the log.
Right, I hard-coded '(locked-hint)' string in the error message as the
message comes from dbus-1. I hope we can get better error messages when
using gdbus in the future.
> >
> > Resolves: rhbz#1323623
> > ---
> > src/systemd-login.c | 102
> > ---
> > -
> > 1 file changed, 95 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/systemd-login.c b/src/systemd-login.c
> > index 730547a..c56fca4 100644
> > --- a/src/systemd-login.c
> > +++ b/src/systemd-login.c
> > @@ -36,14 +36,21 @@ struct session_info {
> > char *match_session_signals;
> > } dbus;
> > gboolean session_is_locked;
> > +gboolean session_locked_hint;
> > };
> >
> > +#define LOGIND_INTERFACE"org.freedesktop.login1"
> > +
> > #define LOGIND_SESSION_INTERFACE"org.freedesktop.login1.Session"
> > #define LOGIND_SESSION_OBJ_TEMPLATE "/org/freedesktop/login1/session/_3%s"
> >
> > +#define DBUS_PROPERTIES_INTERFACE "org.freedesktop.DBus.Properties"
> > +
> > #define SESSION_SIGNAL_LOCK "Lock"
> > #define SESSION_SIGNAL_UNLOCK "Unlock"
> >
> > +#define SESSION_PROP_LOCKED_HINT"LockedHint"
> > +
> > /* dbus related */
> > static DBusConnection *si_dbus_get_system_bus(void)
> > {
> > @@ -112,6 +119,84 @@ static void si_dbus_match_rule_update(struct
> > session_info
> > *si)
> > }
> >
> > static void
> > +si_dbus_read_properties(struct session_info *si)
> > +{
> > +dbus_bool_t locked_hint, ret;
> > +DBusMessageIter iter, iter_variant;
> > +gint type;
> > +DBusError error;
> > +DBusMessage *message = NULL;
> > +DBusMessage *reply = NULL;
> > +gchar *session_object;
> > +const gchar *interface, *property;
> > +
> > +if (si->session == NULL)
> > +return;
> > +
> > +session_object = g_strdup_printf(LOGIND_SESSION_OBJ_TEMPLATE, si-
> > >session);
> > +message = dbus_message_new_method_call(LOGIND_INTERFACE,
> > + session_object,
> > + DBUS_PROPERTIES_INTERFACE,
> > + "Get");
> > +g_free (session_object);
> > +if (message == NULL) {
> > +syslog(LOG_ERR, "Unable to create dbus message");
> > +goto exit;
> > +}
> > +
> > +interface = LOGIND_SESSION_INTERFACE;
> > +property = SESSION_PROP_LOCKED_HINT;
> > +ret = dbus_message_append_args(message,
> > + DBUS_TYPE_STRING, &interface,
> > + DBUS_TYPE_STRING, &property,
> > + DBUS_TYPE_INVALID);
> > +if (!ret) {
> > +syslog(LOG_ERR, "Unable to request locked-hint");
> > +goto exit;
> > +}
> > +
> > +dbus_error_init(&error);
> > +reply = dbus_connection_send_with_reply_and_block(si-
> > >dbus.system_connection,
> > + message,
> > + -1,
> > + &error);
> > +if (reply == NULL || dbus_error_is_set(&error)) {
> According to function definition error is set on failure, so this block can be
> simplified
I'm following the same approach Hans did with console-kit so I would
rather to keep it this way till we move to gdbus. I hope that gdbus
would improve the error handling and not let things like [0] happen for
instance.
[0]
https://cgit.freedesktop.org/spice/linux/vd_agent/tree/src/console-kit.c#n170
Also, I was taught [1] that checking the return value of the function is
better then checking the error.
[1] https://bugzilla.gnome.org/show_bug.cgi?id=763046#c20
> > +if (dbus_error_is_set(&error)) {
> > +syslog(LOG_ERR, "Properties.Get failed: %s", error.message);
> > +dbus_error_free(&error);
> > +} else {
> > +syslog(LOG_ERR, "Properties.Get failed");
> > +}
> > +goto exit;
> > +}
> > +
> > +dbus_message_iter_init(reply, &iter);
> > +type = dbus_message_iter_get_arg_type(&iter);
> > +if (type != DBUS_TYPE_VARIANT) {
> > +syslog(LOG_ERR, "expected a variant, got a '%c' instead", type);
> > +goto exit;
> > +}
> > +
> > +dbus_message_iter_recurse(&iter, &iter_variant);
> > +type = dbus_message_iter_get_arg_type(&iter_variant);
> > +if (type != DBUS_TYPE_BOOLEAN) {
> > +syslog(LOG_E
[Spice-devel] [vdagent-linux v2] systemd-login: check for LockedHint property
That was introduced in systemd v230
---
src/systemd-login.c | 108 +++-
1 file changed, 98 insertions(+), 10 deletions(-)
diff --git a/src/systemd-login.c b/src/systemd-login.c
index fa59348..a89fef7 100644
--- a/src/systemd-login.c
+++ b/src/systemd-login.c
@@ -36,14 +36,21 @@ struct session_info {
char *match_session_signals;
} dbus;
gboolean session_is_locked;
+gboolean session_locked_hint;
};
+#define LOGIND_INTERFACE"org.freedesktop.login1"
+
#define LOGIND_SESSION_INTERFACE"org.freedesktop.login1.Session"
-#define LOGIND_SESSION_OBJ_TEMPLATE "'/org/freedesktop/login1/session/_3%s'"
+#define LOGIND_SESSION_OBJ_TEMPLATE "/org/freedesktop/login1/session/_3%s"
+
+#define DBUS_PROPERTIES_INTERFACE "org.freedesktop.DBus.Properties"
#define SESSION_SIGNAL_LOCK "Lock"
#define SESSION_SIGNAL_UNLOCK "Unlock"
+#define SESSION_PROP_LOCKED_HINT"LockedHint"
+
/* dbus related */
static DBusConnection *si_dbus_get_system_bus(void)
{
@@ -91,8 +98,8 @@ static void si_dbus_match_rule_update(struct session_info *si)
si_dbus_match_remove(si);
si->dbus.match_session_signals =
-g_strdup_printf ("type='signal',interface='%s',path="
- LOGIND_SESSION_OBJ_TEMPLATE,
+g_strdup_printf ("type='signal',interface='%s',path='"
+ LOGIND_SESSION_OBJ_TEMPLATE"'",
LOGIND_SESSION_INTERFACE,
si->session);
if (si->verbose)
@@ -112,6 +119,84 @@ static void si_dbus_match_rule_update(struct session_info
*si)
}
static void
+si_dbus_read_properties(struct session_info *si)
+{
+dbus_bool_t locked_hint, ret;
+DBusMessageIter iter, iter_variant;
+gint type;
+DBusError error;
+DBusMessage *message = NULL;
+DBusMessage *reply = NULL;
+gchar *session_object;
+const gchar *interface, *property;
+
+if (si->session == NULL)
+return;
+
+session_object = g_strdup_printf(LOGIND_SESSION_OBJ_TEMPLATE, si->session);
+message = dbus_message_new_method_call(LOGIND_INTERFACE,
+ session_object,
+ DBUS_PROPERTIES_INTERFACE,
+ "Get");
+g_free (session_object);
+if (message == NULL) {
+syslog(LOG_ERR, "Unable to create dbus message");
+goto exit;
+}
+
+interface = LOGIND_SESSION_INTERFACE;
+property = SESSION_PROP_LOCKED_HINT;
+ret = dbus_message_append_args(message,
+ DBUS_TYPE_STRING, &interface,
+ DBUS_TYPE_STRING, &property,
+ DBUS_TYPE_INVALID);
+if (!ret) {
+syslog(LOG_ERR, "Unable to request locked-hint");
+goto exit;
+}
+
+dbus_error_init(&error);
+reply =
dbus_connection_send_with_reply_and_block(si->dbus.system_connection,
+ message,
+ -1,
+ &error);
+if (reply == NULL || dbus_error_is_set(&error)) {
+if (dbus_error_is_set(&error)) {
+syslog(LOG_ERR, "Properties.Get failed (locked-hint) due %s",
error.message);
+dbus_error_free(&error);
+} else {
+syslog(LOG_ERR, "Properties.Get failed (locked-hint)");
+}
+goto exit;
+}
+
+dbus_message_iter_init(reply, &iter);
+type = dbus_message_iter_get_arg_type(&iter);
+if (type != DBUS_TYPE_VARIANT) {
+syslog(LOG_ERR, "expected a variant, got a '%c' instead", type);
+goto exit;
+}
+
+dbus_message_iter_recurse(&iter, &iter_variant);
+type = dbus_message_iter_get_arg_type(&iter_variant);
+if (type != DBUS_TYPE_BOOLEAN) {
+syslog(LOG_ERR, "expected a boolean, got a '%c' instead", type);
+goto exit;
+}
+dbus_message_iter_get_basic(&iter_variant, &locked_hint);
+
+si->session_locked_hint = (locked_hint) ? TRUE : FALSE;
+exit:
+if (reply != NULL) {
+dbus_message_unref(reply);
+}
+
+if (message != NULL) {
+dbus_message_unref(message);
+}
+}
+
+static void
si_dbus_read_signals(struct session_info *si)
{
DBusMessage *message = NULL;
@@ -220,16 +305,19 @@ char *session_info_session_for_pid(struct session_info
*si, uint32_t pid)
gboolean session_info_session_is_locked(struct session_info *si)
{
-g_return_val_if_fail (si != NULL, FALSE);
+gboolean locked;
-/* We could also rely on IdleHint property from Session which seems to work
- * well in rhel7 but it wasn't working well in my own system (F23). I'm
- * convinced for now that Lock/Unlock signals should be enough but that
- * means Lock/Unlock being done by logind. That might take a while.
- * Check: ht
Re: [Spice-devel] [spice-gtk v3 02/16] file-xfer: introduce flush_callback and flush_done
Hi,
On Thu, Jun 02, 2016 at 08:20:02AM +0200, Pavel Grunt wrote:
> On Wed, 2016-06-01 at 16:23 +0200, Victor Toso wrote:
> > Hi,
> >
> > On Wed, Jun 01, 2016 at 03:24:09PM +0200, Pavel Grunt wrote:
> > > Hi Victor,
> > >
> > > I put some questions/comments inline
> > >
> > > On Mon, 2016-05-30 at 11:54 +0200, Victor Toso wrote:
> > > > By introducing a flush_callback such as SpiceFileTransferTaskFlushCb
> > > > SpiceFileTransferTask becomes agnostic on how channel-main flushes
> > > > the data.
> > > >
> > > > The spice_file_transfer_task_flush_done() function is now introduced
> > > > to tell SpiceFileTransferTask that flushing is over and we can read
> > > > more data if no error has happened.
> > > >
> > > > This change is related to split SpiceFileTransferTask from
> > > > channel-main.
> > > > ---
> > > > src/channel-main.c | 143 --
> > > >
> > > > ---
> > > > 1 file changed, 88 insertions(+), 55 deletions(-)
> > > >
> > > > diff --git a/src/channel-main.c b/src/channel-main.c
> > > > index 89675d5..702f146 100644
> > > > --- a/src/channel-main.c
> > > > +++ b/src/channel-main.c
> > > > @@ -54,9 +54,14 @@
> > > >
> > > > typedef struct spice_migrate spice_migrate;
> > > >
> > > > +typedef void (*SpiceFileTransferTaskFlushCb)(SpiceFileTransferTask
> > > > *xfer_task,
> > > to be consistent with other functions, use 'self' instead of 'xfer_task'
> >
> > This was on purpose. I would like to use _self_ for functions inside
> > spice-file-transfer-task.c and xfer_task elsewhere. As this is callback,
> > I thought xfer_task would fit in situation I just mentioned.
> >
> > The main reason to change the name is, as you point out, some places
> > that use _self_ and some places that use even _task_ (which are very
> > confusing with the GTask).
> >
> > At the end of the series most of the places have consistent names this
> > way. What do you think about it?
> >
> > >
> > > > + void *buffer,
> > > > + gssize count,
> > > > + gpointer user_data);
> > > > static guint32 spice_file_transfer_task_get_id(SpiceFileTransferTask
> > > > *self);
> > > > static SpiceMainChannel
> > > > *spice_file_transfer_task_get_channel(SpiceFileTransferTask *self);
> > > > static GCancellable
> > > > *spice_file_transfer_task_get_cancellable(SpiceFileTransferTask *self);
> > > > +static void spice_file_transfer_task_flush_done(SpiceFileTransferTask
> > > > *self,
> > > > GError *error);
> > > >
> > > > /**
> > > > * SECTION:file-transfer-task
> > > > @@ -89,6 +94,8 @@ struct _SpiceFileTransferTask
> > > > GCancellable *cancellable;
> > > > GFileProgressCallback progress_callback;
> > > > gpointer progress_callback_data;
> > > > +SpiceFileTransferTaskFlushCb flush_callback;
> > > > +gpointer flush_callback_data;
> > > > GAsyncReadyCallbackcallback;
> > > > gpointer user_data;
> > > > char *buffer;
> > > > @@ -1859,73 +1866,49 @@ static void file_xfer_data_flushed_cb(GObject
> > > > *source_object,
> > > > SpiceMainChannel *channel = (SpiceMainChannel *)source_object;
> > > > GError *error = NULL;
> > > >
> > > > -self->pending = FALSE;
> > > > file_xfer_flush_finish(channel, res, &error);
> > > > -if (error || self->error) {
> > > > -spice_file_transfer_task_completed(self, error);
> > > > -return;
> > > > -}
> > > > -
> > > > -if (spice_util_get_debug()) {
> > > > -const GTimeSpan interval = 20 * G_TIME_SPAN_SECOND;
> > > > -gint64 now = g_get_monotonic_time();
> > > > -
> > > > -if (interval < now - self->last_update) {
> > > > -gchar *basename = g_file_get_basename(self->file);
> > > > -self->last_update = now;
> > > > -SPICE_DEBUG("transferred %.2f%% of the file %s",
> > > > -100.0 * self->read_bytes / self->file_size,
> > > > basename);
> > > > -g_free(basename);
> > > > -}
> > > > -}
> > > > -
> > > > -if (self->progress_callback) {
> > > > -goffset read = 0;
> > > > -goffset total = 0;
> > > > -SpiceMainChannel *main_channel = self->channel;
> > > > -GHashTableIter iter;
> > > > -gpointer key, value;
> > > > -
> > > > -/* since the progress_callback does not have a parameter to
> > > > indicate
> > > > - * which file the progress is associated with, report progress
> > > > on
> > > > all
> > > > - * current transfers */
> > > > -g_hash_table_iter_init(&iter, main_channel->priv-
> > > > >file_xfer_tasks);
> > > > -while (g_hash_table_iter_next(&iter, &key, &value)) {
> > > > -SpiceFileTransferTask *t = (Spi
Re: [Spice-devel] [vdagent-linux v1] systemd-login: check for LockedHint property
On Fri, 2016-06-03 at 09:44 +0200, Victor Toso wrote:
> Hi,
>
> On Thu, Jun 02, 2016 at 07:43:25PM +0200, Pavel Grunt wrote:
> > Hi,
> >
> > On Fri, 2016-05-27 at 11:42 +0200, Victor Toso wrote:
> > > Property introduced in v230 of systemd.
> > >
> > > Systems that don't have up to date systemd-login will get the
> > > following log message:
> > >
> > > "Properties.Get failed: Unknown property or interface."
> > Can we give more info (which property is unknown) ? To make it easier for
> > users.
> > The property is new, so almost everyone will see the message in the log.
>
> Right, I hard-coded '(locked-hint)' string in the error message as the
> message comes from dbus-1. I hope we can get better error messages when
> using gdbus in the future.
thanks
>
> > >
> > > Resolves: rhbz#1323623
> > > ---
> > > src/systemd-login.c | 102
> > > ---
> > > -
> > > 1 file changed, 95 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/src/systemd-login.c b/src/systemd-login.c
> > > index 730547a..c56fca4 100644
> > > --- a/src/systemd-login.c
> > > +++ b/src/systemd-login.c
> > > @@ -36,14 +36,21 @@ struct session_info {
> > > char *match_session_signals;
> > > } dbus;
> > > gboolean session_is_locked;
> > > +gboolean session_locked_hint;
> > > };
> > >
> > > +#define LOGIND_INTERFACE"org.freedesktop.login1"
> > > +
> > > #define LOGIND_SESSION_INTERFACE"org.freedesktop.login1.Session"
> > > #define LOGIND_SESSION_OBJ_TEMPLATE
> > > "/org/freedesktop/login1/session/_3%s"
> > >
> > > +#define DBUS_PROPERTIES_INTERFACE "org.freedesktop.DBus.Properties"
> > > +
> > > #define SESSION_SIGNAL_LOCK "Lock"
> > > #define SESSION_SIGNAL_UNLOCK "Unlock"
> > >
> > > +#define SESSION_PROP_LOCKED_HINT"LockedHint"
> > > +
> > > /* dbus related */
> > > static DBusConnection *si_dbus_get_system_bus(void)
> > > {
> > > @@ -112,6 +119,84 @@ static void si_dbus_match_rule_update(struct
> > > session_info
> > > *si)
> > > }
> > >
> > > static void
> > > +si_dbus_read_properties(struct session_info *si)
> > > +{
> > > +dbus_bool_t locked_hint, ret;
> > > +DBusMessageIter iter, iter_variant;
> > > +gint type;
> > > +DBusError error;
> > > +DBusMessage *message = NULL;
> > > +DBusMessage *reply = NULL;
> > > +gchar *session_object;
> > > +const gchar *interface, *property;
> > > +
> > > +if (si->session == NULL)
> > > +return;
> > > +
> > > +session_object = g_strdup_printf(LOGIND_SESSION_OBJ_TEMPLATE, si-
> > > > session);
> > > +message = dbus_message_new_method_call(LOGIND_INTERFACE,
> > > + session_object,
> > > + DBUS_PROPERTIES_INTERFACE,
> > > + "Get");
> > > +g_free (session_object);
> > > +if (message == NULL) {
> > > +syslog(LOG_ERR, "Unable to create dbus message");
> > > +goto exit;
> > > +}
> > > +
> > > +interface = LOGIND_SESSION_INTERFACE;
> > > +property = SESSION_PROP_LOCKED_HINT;
> > > +ret = dbus_message_append_args(message,
> > > + DBUS_TYPE_STRING, &interface,
> > > + DBUS_TYPE_STRING, &property,
> > > + DBUS_TYPE_INVALID);
> > > +if (!ret) {
> > > +syslog(LOG_ERR, "Unable to request locked-hint");
> > > +goto exit;
> > > +}
> > > +
> > > +dbus_error_init(&error);
> > > +reply = dbus_connection_send_with_reply_and_block(si-
> > > > dbus.system_connection,
> > > + message,
> > > + -1,
> > > + &error);
> > > +if (reply == NULL || dbus_error_is_set(&error)) {
> > According to function definition error is set on failure, so this block can
> > be
> > simplified
>
> I'm following the same approach Hans did with console-kit so I would
> rather to keep it this way till we move to gdbus. I hope that gdbus
> would improve the error handling and not let things like [0] happen for
> instance.
>
> [0] https://cgit.freedesktop.org/spice/linux/vd_agent/tree/src/console-kit.c#n
> 170
>
> Also, I was taught [1] that checking the return value of the function is
> better then checking the error.
Definitely, the function returns NULL w/o setting error when validating its
parameters. But the reply cannot be nonnull and error set. I just wanted to say
that the second part of the condition is redundant: 'if (reply == NULL)' is
enough
Pavel
>
> [1] https://bugzilla.gnome.org/show_bug.cgi?id=763046#c20
>
> > > +if (dbus_error_is_set(&error)) {
> > > +syslog(LOG_ERR, "Properties.Get failed: %s", error.message);
> > > +dbus_error_free(&error);
> > > +} else
Re: [Spice-devel] 1cec1c5118b65124de6bc6f984f376ff4e297bfb ("reds: Make VDIPortState a GObject") changes
Hi, On Thu, Jun 02, 2016 at 10:11:33AM -0400, Frediano Ziglio wrote: > > > > Hi, > > I think this patch is one of the most puzzling commit and caused > > some regression (2 patches applied and one going). > > The problem was not actually the move to GObject but that the > > implementation change completely the lifetime of the underlying > > RedCharDevice. > > Before the patch the RedCharDevice was created when Qemu created the > > device and possibly destroyed when device is destroyed but also on > > some errors from the main channel. > > I tried to understand what the other RedCharDevice do. SpiceVmc and > > SmartCard basically create the device when Qemu create it and destroy > > when Qemu destroy them (currently never). When the device is created > > a channel is created. Here there are some differences. The VDI > > CharDevice is already present, no matter if a Qemu device is present > > or not. Also the MainChannel (the channel corresponding to the VDI > > CharDevice) is always present and not created when the VDI CharDevice > > is created. These 2 differences were however already present before > > this patch. > > One of the difference is that before an error could lead to > > destroying the CharDevice basically causing all future MainChannels > > to have a not working agent (as the agent requires a working CharDevice). > > > > At the end I consider this as a good patch and in line with what other > > CharDevices are working (the CharDevice is destroyed only by Qemu) however > > there are cases were before CharDevice was not present and now is it. > > Particularly what happens when the agent is stopped in the guest? > > The guest agent is informed so won't send data? > > Is it stopped when sending large data (as instance large file)? Or > > will send lot of data that will be ignored by the server? > > In case of errors agent_attached is set to FALSE but who will set again > > this field to TRUE? Apparently this is set to TRUE only in > > attach_to_red_agent > > called during spice_server_add_interface so this will be set back to TRUE > > only during migration. Looks like this is a bug, guest should be able > > to launch again the agent and variable should be set back to TRUE. > > This morning we had some issues with agents and we did some tests. > Turns out that Qemu create the CharDevice when guest attach to the device, > not when Qemu starts. > This change a lot the life and state of the device. > This causes some sync problem between server and client. > I'm actually considering a rollback. If I'm following the discussion correctly (from IRC) the main regression is the recently problems with server <-> client when agent is restarted/killed under ongoing operation like file-transfer. I agree that this is a good moment double check the code as it's been a few bugs already but I'm not 100% sure that all of them were related to this patch or if it was even possible before (i.e not a regression). I'll try to take a look on this today and provide a better feedback. toso ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [vdagent-linux v3] systemd-login: check for LockedHint property
That was introduced in systemd v230
---
src/systemd-login.c | 108 +++-
1 file changed, 98 insertions(+), 10 deletions(-)
diff --git a/src/systemd-login.c b/src/systemd-login.c
index fa59348..9719c0b 100644
--- a/src/systemd-login.c
+++ b/src/systemd-login.c
@@ -36,14 +36,21 @@ struct session_info {
char *match_session_signals;
} dbus;
gboolean session_is_locked;
+gboolean session_locked_hint;
};
+#define LOGIND_INTERFACE"org.freedesktop.login1"
+
#define LOGIND_SESSION_INTERFACE"org.freedesktop.login1.Session"
-#define LOGIND_SESSION_OBJ_TEMPLATE "'/org/freedesktop/login1/session/_3%s'"
+#define LOGIND_SESSION_OBJ_TEMPLATE "/org/freedesktop/login1/session/_3%s"
+
+#define DBUS_PROPERTIES_INTERFACE "org.freedesktop.DBus.Properties"
#define SESSION_SIGNAL_LOCK "Lock"
#define SESSION_SIGNAL_UNLOCK "Unlock"
+#define SESSION_PROP_LOCKED_HINT"LockedHint"
+
/* dbus related */
static DBusConnection *si_dbus_get_system_bus(void)
{
@@ -91,8 +98,8 @@ static void si_dbus_match_rule_update(struct session_info *si)
si_dbus_match_remove(si);
si->dbus.match_session_signals =
-g_strdup_printf ("type='signal',interface='%s',path="
- LOGIND_SESSION_OBJ_TEMPLATE,
+g_strdup_printf ("type='signal',interface='%s',path='"
+ LOGIND_SESSION_OBJ_TEMPLATE"'",
LOGIND_SESSION_INTERFACE,
si->session);
if (si->verbose)
@@ -112,6 +119,84 @@ static void si_dbus_match_rule_update(struct session_info
*si)
}
static void
+si_dbus_read_properties(struct session_info *si)
+{
+dbus_bool_t locked_hint, ret;
+DBusMessageIter iter, iter_variant;
+gint type;
+DBusError error;
+DBusMessage *message = NULL;
+DBusMessage *reply = NULL;
+gchar *session_object;
+const gchar *interface, *property;
+
+if (si->session == NULL)
+return;
+
+session_object = g_strdup_printf(LOGIND_SESSION_OBJ_TEMPLATE, si->session);
+message = dbus_message_new_method_call(LOGIND_INTERFACE,
+ session_object,
+ DBUS_PROPERTIES_INTERFACE,
+ "Get");
+g_free (session_object);
+if (message == NULL) {
+syslog(LOG_ERR, "Unable to create dbus message");
+goto exit;
+}
+
+interface = LOGIND_SESSION_INTERFACE;
+property = SESSION_PROP_LOCKED_HINT;
+ret = dbus_message_append_args(message,
+ DBUS_TYPE_STRING, &interface,
+ DBUS_TYPE_STRING, &property,
+ DBUS_TYPE_INVALID);
+if (!ret) {
+syslog(LOG_ERR, "Unable to request locked-hint");
+goto exit;
+}
+
+dbus_error_init(&error);
+reply =
dbus_connection_send_with_reply_and_block(si->dbus.system_connection,
+ message,
+ -1,
+ &error);
+if (reply == NULL) {
+if (dbus_error_is_set(&error)) {
+syslog(LOG_ERR, "Properties.Get failed (locked-hint) due %s",
error.message);
+dbus_error_free(&error);
+} else {
+syslog(LOG_ERR, "Properties.Get failed (locked-hint)");
+}
+goto exit;
+}
+
+dbus_message_iter_init(reply, &iter);
+type = dbus_message_iter_get_arg_type(&iter);
+if (type != DBUS_TYPE_VARIANT) {
+syslog(LOG_ERR, "expected a variant, got a '%c' instead", type);
+goto exit;
+}
+
+dbus_message_iter_recurse(&iter, &iter_variant);
+type = dbus_message_iter_get_arg_type(&iter_variant);
+if (type != DBUS_TYPE_BOOLEAN) {
+syslog(LOG_ERR, "expected a boolean, got a '%c' instead", type);
+goto exit;
+}
+dbus_message_iter_get_basic(&iter_variant, &locked_hint);
+
+si->session_locked_hint = (locked_hint) ? TRUE : FALSE;
+exit:
+if (reply != NULL) {
+dbus_message_unref(reply);
+}
+
+if (message != NULL) {
+dbus_message_unref(message);
+}
+}
+
+static void
si_dbus_read_signals(struct session_info *si)
{
DBusMessage *message = NULL;
@@ -220,16 +305,19 @@ char *session_info_session_for_pid(struct session_info
*si, uint32_t pid)
gboolean session_info_session_is_locked(struct session_info *si)
{
-g_return_val_if_fail (si != NULL, FALSE);
+gboolean locked;
-/* We could also rely on IdleHint property from Session which seems to work
- * well in rhel7 but it wasn't working well in my own system (F23). I'm
- * convinced for now that Lock/Unlock signals should be enough but that
- * means Lock/Unlock being done by logind. That might take a while.
- * Check: https://bugzilla.gnome.org/show
Re: [Spice-devel] [vdagent-linux v3] systemd-login: check for LockedHint property
On Fri, 2016-06-03 at 10:55 +0200, Victor Toso wrote:
> That was introduced in systemd v230
It seems that some useful info from the v1 got lost, please put it back (you
don't have to resend it)
Acked-by: Pavel Grunt
> ---
> src/systemd-login.c | 108 +++--
> ---
> 1 file changed, 98 insertions(+), 10 deletions(-)
>
> diff --git a/src/systemd-login.c b/src/systemd-login.c
> index fa59348..9719c0b 100644
> --- a/src/systemd-login.c
> +++ b/src/systemd-login.c
> @@ -36,14 +36,21 @@ struct session_info {
> char *match_session_signals;
> } dbus;
> gboolean session_is_locked;
> +gboolean session_locked_hint;
> };
>
> +#define LOGIND_INTERFACE"org.freedesktop.login1"
> +
> #define LOGIND_SESSION_INTERFACE"org.freedesktop.login1.Session"
> -#define LOGIND_SESSION_OBJ_TEMPLATE "'/org/freedesktop/login1/session/_3%s'"
> +#define LOGIND_SESSION_OBJ_TEMPLATE "/org/freedesktop/login1/session/_3%s"
> +
> +#define DBUS_PROPERTIES_INTERFACE "org.freedesktop.DBus.Properties"
>
> #define SESSION_SIGNAL_LOCK "Lock"
> #define SESSION_SIGNAL_UNLOCK "Unlock"
>
> +#define SESSION_PROP_LOCKED_HINT"LockedHint"
> +
> /* dbus related */
> static DBusConnection *si_dbus_get_system_bus(void)
> {
> @@ -91,8 +98,8 @@ static void si_dbus_match_rule_update(struct session_info
> *si)
> si_dbus_match_remove(si);
>
> si->dbus.match_session_signals =
> -g_strdup_printf ("type='signal',interface='%s',path="
> - LOGIND_SESSION_OBJ_TEMPLATE,
> +g_strdup_printf ("type='signal',interface='%s',path='"
> + LOGIND_SESSION_OBJ_TEMPLATE"'",
> LOGIND_SESSION_INTERFACE,
> si->session);
> if (si->verbose)
> @@ -112,6 +119,84 @@ static void si_dbus_match_rule_update(struct session_info
> *si)
> }
>
> static void
> +si_dbus_read_properties(struct session_info *si)
> +{
> +dbus_bool_t locked_hint, ret;
> +DBusMessageIter iter, iter_variant;
> +gint type;
> +DBusError error;
> +DBusMessage *message = NULL;
> +DBusMessage *reply = NULL;
> +gchar *session_object;
> +const gchar *interface, *property;
> +
> +if (si->session == NULL)
> +return;
> +
> +session_object = g_strdup_printf(LOGIND_SESSION_OBJ_TEMPLATE, si-
> >session);
> +message = dbus_message_new_method_call(LOGIND_INTERFACE,
> + session_object,
> + DBUS_PROPERTIES_INTERFACE,
> + "Get");
> +g_free (session_object);
> +if (message == NULL) {
> +syslog(LOG_ERR, "Unable to create dbus message");
> +goto exit;
> +}
> +
> +interface = LOGIND_SESSION_INTERFACE;
> +property = SESSION_PROP_LOCKED_HINT;
> +ret = dbus_message_append_args(message,
> + DBUS_TYPE_STRING, &interface,
> + DBUS_TYPE_STRING, &property,
> + DBUS_TYPE_INVALID);
> +if (!ret) {
> +syslog(LOG_ERR, "Unable to request locked-hint");
> +goto exit;
> +}
> +
> +dbus_error_init(&error);
> +reply = dbus_connection_send_with_reply_and_block(si-
> >dbus.system_connection,
> + message,
> + -1,
> + &error);
> +if (reply == NULL) {
> +if (dbus_error_is_set(&error)) {
> +syslog(LOG_ERR, "Properties.Get failed (locked-hint) due %s",
> error.message);
> +dbus_error_free(&error);
> +} else {
> +syslog(LOG_ERR, "Properties.Get failed (locked-hint)");
> +}
> +goto exit;
> +}
> +
> +dbus_message_iter_init(reply, &iter);
> +type = dbus_message_iter_get_arg_type(&iter);
> +if (type != DBUS_TYPE_VARIANT) {
> +syslog(LOG_ERR, "expected a variant, got a '%c' instead", type);
> +goto exit;
> +}
> +
> +dbus_message_iter_recurse(&iter, &iter_variant);
> +type = dbus_message_iter_get_arg_type(&iter_variant);
> +if (type != DBUS_TYPE_BOOLEAN) {
> +syslog(LOG_ERR, "expected a boolean, got a '%c' instead", type);
> +goto exit;
> +}
> +dbus_message_iter_get_basic(&iter_variant, &locked_hint);
> +
> +si->session_locked_hint = (locked_hint) ? TRUE : FALSE;
> +exit:
> +if (reply != NULL) {
> +dbus_message_unref(reply);
> +}
> +
> +if (message != NULL) {
> +dbus_message_unref(message);
> +}
> +}
> +
> +static void
> si_dbus_read_signals(struct session_info *si)
> {
> DBusMessage *message = NULL;
> @@ -220,16 +305,19 @@ char *session_info_session_for_pid(struct session_info
> *si, uint32_t pid)
>
> gboolean session_info_session_is_lock
Re: [Spice-devel] [vdagent-linux v3] systemd-login: check for LockedHint property
Hi,
On Fri, Jun 03, 2016 at 11:04:11AM +0200, Pavel Grunt wrote:
> On Fri, 2016-06-03 at 10:55 +0200, Victor Toso wrote:
> > That was introduced in systemd v230
>
> It seems that some useful info from the v1 got lost, please put it back (you
> don't have to resend it)
True! Thanks! My fault on squashing both patches.
>
> Acked-by: Pavel Grunt
Thanks, pushed:
https://cgit.freedesktop.org/spice/linux/vd_agent/commit/?id=ec843a21b29d7fa21ba3393b84368bc2e39d3ce7
> > ---
> > src/systemd-login.c | 108 +++--
> > ---
> > 1 file changed, 98 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/systemd-login.c b/src/systemd-login.c
> > index fa59348..9719c0b 100644
> > --- a/src/systemd-login.c
> > +++ b/src/systemd-login.c
> > @@ -36,14 +36,21 @@ struct session_info {
> > char *match_session_signals;
> > } dbus;
> > gboolean session_is_locked;
> > +gboolean session_locked_hint;
> > };
> >
> > +#define LOGIND_INTERFACE"org.freedesktop.login1"
> > +
> > #define LOGIND_SESSION_INTERFACE"org.freedesktop.login1.Session"
> > -#define LOGIND_SESSION_OBJ_TEMPLATE
> > "'/org/freedesktop/login1/session/_3%s'"
> > +#define LOGIND_SESSION_OBJ_TEMPLATE "/org/freedesktop/login1/session/_3%s"
> > +
> > +#define DBUS_PROPERTIES_INTERFACE "org.freedesktop.DBus.Properties"
> >
> > #define SESSION_SIGNAL_LOCK "Lock"
> > #define SESSION_SIGNAL_UNLOCK "Unlock"
> >
> > +#define SESSION_PROP_LOCKED_HINT"LockedHint"
> > +
> > /* dbus related */
> > static DBusConnection *si_dbus_get_system_bus(void)
> > {
> > @@ -91,8 +98,8 @@ static void si_dbus_match_rule_update(struct session_info
> > *si)
> > si_dbus_match_remove(si);
> >
> > si->dbus.match_session_signals =
> > -g_strdup_printf ("type='signal',interface='%s',path="
> > - LOGIND_SESSION_OBJ_TEMPLATE,
> > +g_strdup_printf ("type='signal',interface='%s',path='"
> > + LOGIND_SESSION_OBJ_TEMPLATE"'",
> > LOGIND_SESSION_INTERFACE,
> > si->session);
> > if (si->verbose)
> > @@ -112,6 +119,84 @@ static void si_dbus_match_rule_update(struct
> > session_info
> > *si)
> > }
> >
> > static void
> > +si_dbus_read_properties(struct session_info *si)
> > +{
> > +dbus_bool_t locked_hint, ret;
> > +DBusMessageIter iter, iter_variant;
> > +gint type;
> > +DBusError error;
> > +DBusMessage *message = NULL;
> > +DBusMessage *reply = NULL;
> > +gchar *session_object;
> > +const gchar *interface, *property;
> > +
> > +if (si->session == NULL)
> > +return;
> > +
> > +session_object = g_strdup_printf(LOGIND_SESSION_OBJ_TEMPLATE, si-
> > >session);
> > +message = dbus_message_new_method_call(LOGIND_INTERFACE,
> > + session_object,
> > + DBUS_PROPERTIES_INTERFACE,
> > + "Get");
> > +g_free (session_object);
> > +if (message == NULL) {
> > +syslog(LOG_ERR, "Unable to create dbus message");
> > +goto exit;
> > +}
> > +
> > +interface = LOGIND_SESSION_INTERFACE;
> > +property = SESSION_PROP_LOCKED_HINT;
> > +ret = dbus_message_append_args(message,
> > + DBUS_TYPE_STRING, &interface,
> > + DBUS_TYPE_STRING, &property,
> > + DBUS_TYPE_INVALID);
> > +if (!ret) {
> > +syslog(LOG_ERR, "Unable to request locked-hint");
> > +goto exit;
> > +}
> > +
> > +dbus_error_init(&error);
> > +reply = dbus_connection_send_with_reply_and_block(si-
> > >dbus.system_connection,
> > + message,
> > + -1,
> > + &error);
> > +if (reply == NULL) {
> > +if (dbus_error_is_set(&error)) {
> > +syslog(LOG_ERR, "Properties.Get failed (locked-hint) due %s",
> > error.message);
> > +dbus_error_free(&error);
> > +} else {
> > +syslog(LOG_ERR, "Properties.Get failed (locked-hint)");
> > +}
> > +goto exit;
> > +}
> > +
> > +dbus_message_iter_init(reply, &iter);
> > +type = dbus_message_iter_get_arg_type(&iter);
> > +if (type != DBUS_TYPE_VARIANT) {
> > +syslog(LOG_ERR, "expected a variant, got a '%c' instead", type);
> > +goto exit;
> > +}
> > +
> > +dbus_message_iter_recurse(&iter, &iter_variant);
> > +type = dbus_message_iter_get_arg_type(&iter_variant);
> > +if (type != DBUS_TYPE_BOOLEAN) {
> > +syslog(LOG_ERR, "expected a boolean, got a '%c' instead", type);
> > +goto exit;
> > +}
> > +dbus_message_iter_get_basic(&iter_variant, &locked_hint)
[Spice-devel] [PATCH 0/4] Add support for cursor commands to replay utility
Allow to record and replay even cursor commands. This allows to extend tests to these commands. Frediano Ziglio (4): record: Support cursor commands worker: Record cursor commands replay: Load cursor commands replay: Handle cursor commands server/red-record-qxl.c | 7 +++-- server/red-replay-qxl.c | 70 + server/red-worker.c | 5 server/tests/replay.c | 50 ++- 4 files changed, 112 insertions(+), 20 deletions(-) -- 2.7.4 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH 1/4] record: Support cursor commands
Use red_record_cursor_cmd to be able to record cursor commands.
Signed-off-by: Frediano Ziglio
---
server/red-record-qxl.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/server/red-record-qxl.c b/server/red-record-qxl.c
index 39d6639..9b7d2af 100644
--- a/server/red-record-qxl.c
+++ b/server/red-record-qxl.c
@@ -763,8 +763,8 @@ static void red_record_cursor(FILE *fd, RedMemSlotInfo
*slots, int group_id,
&qxl->chunk);
}
-void red_record_cursor_cmd(FILE *fd, RedMemSlotInfo *slots, int group_id,
- QXLPHYSICAL addr)
+static void red_record_cursor_cmd(FILE *fd, RedMemSlotInfo *slots, int
group_id,
+ QXLPHYSICAL addr)
{
QXLCursorCmd *qxl;
int error;
@@ -834,6 +834,9 @@ void red_record_qxl_command(RedRecord *record,
RedMemSlotInfo *slots,
case QXL_CMD_SURFACE:
red_record_surface_cmd(fd, slots, ext_cmd.group_id, ext_cmd.cmd.data);
break;
+case QXL_CMD_CURSOR:
+red_record_cursor_cmd(fd, slots, ext_cmd.group_id, ext_cmd.cmd.data);
+break;
}
}
--
2.7.4
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH 2/4] worker: Record cursor commands
Signed-off-by: Frediano Ziglio
---
server/red-worker.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/server/red-worker.c b/server/red-worker.c
index a14f55d..e754bd2 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -156,6 +156,11 @@ static int red_process_cursor(RedWorker *worker, int
*ring_is_empty)
worker->cursor_poll_tries++;
return n;
}
+
+if (worker->record)
+red_record_qxl_command(worker->record, &worker->mem_slots, ext_cmd,
+ stat_now(CLOCK_MONOTONIC));
+
worker->cursor_poll_tries = 0;
switch (ext_cmd.cmd.type) {
case QXL_CMD_CURSOR: {
--
2.7.4
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH 3/4] replay: Load cursor commands
Signed-off-by: Frediano Ziglio
---
server/red-replay-qxl.c | 70 +
1 file changed, 70 insertions(+)
diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
index 17019f8..c7d570c 100644
--- a/server/red-replay-qxl.c
+++ b/server/red-replay-qxl.c
@@ -306,6 +306,14 @@ static void red_replay_point_ptr(SpiceReplay *replay,
QXLPoint *qxl)
replay_fscanf(replay, "point %d %d\n", &qxl->x, &qxl->y);
}
+static void red_replay_point16_ptr(SpiceReplay *replay, QXLPoint16 *qxl)
+{
+int x, y;
+replay_fscanf(replay, "point16 %d %d\n", &x, &y);
+qxl->x = x;
+qxl->y = y;
+}
+
static void red_replay_rect_ptr(SpiceReplay *replay, const char *prefix,
QXLRect *qxl)
{
char template[1024];
@@ -1052,6 +1060,59 @@ static void red_replay_surface_cmd_free(SpiceReplay
*replay, QXLSurfaceCmd *qxl)
free(qxl);
}
+static QXLCursor *red_replay_cursor(SpiceReplay *replay)
+{
+int temp;
+QXLCursor cursor, *qxl = NULL;
+
+replay_fscanf(replay, "header.unique %"SCNu64"\n", &cursor.header.unique);
+replay_fscanf(replay, "header.type %d\n", &temp); cursor.header.type =
temp;
+replay_fscanf(replay, "header.width %d\n", &temp); cursor.header.width =
temp;
+replay_fscanf(replay, "header.height %d\n", &temp); cursor.header.height =
temp;
+replay_fscanf(replay, "header.hot_spot_x %d\n", &temp);
cursor.header.hot_spot_x = temp;
+replay_fscanf(replay, "header.hot_spot_y %d\n", &temp);
cursor.header.hot_spot_y = temp;
+
+replay_fscanf(replay, "data_size %d\n", &temp); cursor.data_size = temp;
+cursor.data_size = red_replay_data_chunks(replay, "cursor",
(uint8_t**)&qxl, sizeof(QXLCursor));
+qxl->header = cursor.header;
+qxl->data_size = cursor.data_size;
+return qxl;
+}
+
+static QXLCursorCmd *red_replay_cursor_cmd(SpiceReplay *replay)
+{
+int temp;
+QXLCursorCmd *qxl = spice_new0(QXLCursorCmd, 1);
+
+replay_fscanf(replay, "cursor_cmd\n");
+replay_fscanf(replay, "type %d\n", &temp); qxl->type = temp;
+switch (qxl->type) {
+case QXL_CURSOR_SET:
+red_replay_point16_ptr(replay, &qxl->u.set.position);
+replay_fscanf(replay, "u.set.visible %d\n", &temp); qxl->u.set.visible
= temp;
+qxl->u.set.shape = QXLPHYSICAL_FROM_PTR(red_replay_cursor(replay));
+break;
+case QXL_CURSOR_MOVE:
+red_replay_point16_ptr(replay, &qxl->u.position);
+break;
+case QXL_CURSOR_TRAIL:
+replay_fscanf(replay, "u.trail.length %d\n", &temp);
qxl->u.trail.length = temp;
+replay_fscanf(replay, "u.trail.frequency %d\n", &temp);
qxl->u.trail.frequency = temp;
+break;
+}
+return qxl;
+}
+
+static void red_replay_cursor_cmd_free(SpiceReplay *replay, QXLCursorCmd *qxl)
+{
+if (qxl->type == QXL_CURSOR_SET) {
+QXLCursor *cursor = QXLPHYSICAL_TO_PTR(qxl->u.set.shape);
+red_replay_data_chunks_free(replay, cursor, sizeof(*cursor));
+}
+
+free(qxl);
+}
+
static void replay_handle_create_primary(QXLWorker *worker, SpiceReplay
*replay)
{
QXLDevSurfaceCreate surface = { 0, };
@@ -1148,6 +1209,9 @@ SPICE_GNUC_VISIBLE QXLCommandExt*
spice_replay_next_cmd(SpiceReplay *replay,
case QXL_CMD_SURFACE:
cmd->cmd.data = QXLPHYSICAL_FROM_PTR(red_replay_surface_cmd(replay));
break;
+case QXL_CMD_CURSOR:
+cmd->cmd.data = QXLPHYSICAL_FROM_PTR(red_replay_cursor_cmd(replay));
+break;
}
QXLReleaseInfo *info;
@@ -1155,6 +1219,7 @@ SPICE_GNUC_VISIBLE QXLCommandExt*
spice_replay_next_cmd(SpiceReplay *replay,
case QXL_CMD_DRAW:
case QXL_CMD_UPDATE:
case QXL_CMD_SURFACE:
+case QXL_CMD_CURSOR:
info = QXLPHYSICAL_TO_PTR(cmd->cmd.data);
info->id = (uintptr_t)cmd;
}
@@ -1187,6 +1252,11 @@ SPICE_GNUC_VISIBLE void
spice_replay_free_cmd(SpiceReplay *replay, QXLCommandExt
red_replay_surface_cmd_free(replay, qxl);
break;
}
+case QXL_CMD_CURSOR: {
+QXLCursorCmd *qxl = QXLPHYSICAL_TO_PTR(cmd->cmd.data);
+red_replay_cursor_cmd_free(replay, qxl);
+break;
+}
default:
break;
}
--
2.7.4
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH 4/4] replay: Handle cursor commands
Signed-off-by: Frediano Ziglio
---
server/tests/replay.c | 50 --
1 file changed, 32 insertions(+), 18 deletions(-)
diff --git a/server/tests/replay.c b/server/tests/replay.c
index d552327..528609b 100644
--- a/server/tests/replay.c
+++ b/server/tests/replay.c
@@ -52,7 +52,8 @@ static gboolean print_count = FALSE;
static guint ncommands = 0;
static pid_t client_pid;
static GMainLoop *loop = NULL;
-static GAsyncQueue *aqueue = NULL;
+static GAsyncQueue *cmd_queue = NULL;
+static GAsyncQueue *cursor_queue = NULL;
static long total_size;
static pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
@@ -113,10 +114,12 @@ static gboolean fill_queue_idle(gpointer user_data)
gboolean keep = FALSE;
gboolean wakeup = FALSE;
-while (g_async_queue_length(aqueue) < 50) {
+while ((g_async_queue_length(cmd_queue) +
+g_async_queue_length(cursor_queue)) < 50) {
QXLCommandExt *cmd = spice_replay_next_cmd(replay, qxl_worker);
if (!cmd) {
-g_async_queue_push(aqueue, GINT_TO_POINTER(-1));
+g_async_queue_push(cmd_queue, GINT_TO_POINTER(-1));
+g_async_queue_push(cursor_queue, GINT_TO_POINTER(-1));
goto end;
}
@@ -127,7 +130,11 @@ static gboolean fill_queue_idle(gpointer user_data)
}
wakeup = TRUE;
-g_async_queue_push(aqueue, cmd);
+if (cmd->cmd.type == QXL_CMD_CURSOR) {
+g_async_queue_push(cursor_queue, cmd);
+} else {
+g_async_queue_push(cmd_queue, cmd);
+}
}
end:
@@ -166,17 +173,21 @@ end:
// called from spice_server thread (i.e. red_worker thread)
-static int get_command(QXLInstance *qin, QXLCommandExt *ext)
+static int get_command_from(QXLInstance *qin, QXLCommandExt *ext, GAsyncQueue
*queue)
{
QXLCommandExt *cmd;
-if (g_async_queue_length(aqueue) == 0) {
+if (g_async_queue_length(cmd_queue) == 0 &&
+g_async_queue_length(cursor_queue) == 0) {
/* could use a gcondition ? */
fill_queue();
return FALSE;
}
-cmd = g_async_queue_try_pop(aqueue);
+cmd = g_async_queue_try_pop(queue);
+if (cmd == NULL) {
+return FALSE;
+}
if (GPOINTER_TO_INT(cmd) == -1) {
g_main_loop_quit(loop);
return FALSE;
@@ -187,8 +198,14 @@ static int get_command(QXLInstance *qin, QXLCommandExt
*ext)
return TRUE;
}
-static int req_cmd_notification(QXLInstance *qin)
+static int get_command(QXLInstance *qin, QXLCommandExt *ext)
+{
+return get_command_from(qin, ext, cmd_queue);
+}
+
+static int req_notification(QXLInstance *qin)
{
+/* we don't have currently message pending */
return TRUE;
}
@@ -214,12 +231,7 @@ static void release_resource(QXLInstance *qin, struct
QXLReleaseInfoExt release_
static int get_cursor_command(QXLInstance *qin, struct QXLCommandExt *ext)
{
-return FALSE;
-}
-
-static int req_cursor_notification(QXLInstance *qin)
-{
-return TRUE;
+return get_command_from(qin, ext, cursor_queue);
}
static void notify_update(QXLInstance *qin, uint32_t update_id)
@@ -243,10 +255,10 @@ static QXLInterface display_sif = {
.set_mm_time = set_mm_time,
.get_init_info = get_init_info,
.get_command = get_command,
-.req_cmd_notification = req_cmd_notification,
+.req_cmd_notification = req_notification,
.release_resource = release_resource,
.get_cursor_command = get_cursor_command,
-.req_cursor_notification = req_cursor_notification,
+.req_cursor_notification = req_notification,
.notify_update = notify_update,
.flush_resources = flush_resources,
};
@@ -379,7 +391,8 @@ int main(int argc, char **argv)
exit(1);
}
-aqueue = g_async_queue_new();
+cmd_queue = g_async_queue_new();
+cursor_queue = g_async_queue_new();
core = basic_event_loop_init();
core->channel_event = replay_channel_event;
@@ -414,7 +427,8 @@ int main(int argc, char **argv)
g_print("Counted %d commands\n", ncommands);
end_replay();
-g_async_queue_unref(aqueue);
+g_async_queue_unref(cmd_queue);
+g_async_queue_unref(cursor_queue);
/* FIXME: there should be a way to join server threads before:
* g_main_loop_unref(loop);
--
2.7.4
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] streaming: Use the frame dimensions reported by the video decoder
Hi,
On Thu, 2016-05-26 at 15:35 +0200, Francois Gouget wrote:
> The dimensions sent by the remote end are redundant and should not be
> trusted.
>
> Signed-off-by: Francois Gouget
> ---
>
> Now with smooth early variable declarations and chunky gotos inside.
>
thanks
>
> src/channel-display-gst.c | 39 ---
> src/channel-display-mjpeg.c | 25 +
> src/channel-display-priv.h | 3 +--
> src/channel-display.c | 24 +---
> 4 files changed, 47 insertions(+), 44 deletions(-)
>
> diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> index 46a85ea..07121eb 100644
> --- a/src/channel-display-gst.c
> +++ b/src/channel-display-gst.c
> @@ -87,26 +87,51 @@ static void schedule_frame(SpiceGstDecoder *decoder);
> static gboolean display_frame(gpointer video_decoder)
> {
> SpiceGstDecoder *decoder = (SpiceGstDecoder*)video_decoder;
> +SpiceFrame *frame;
> +GstCaps *caps;
> +gint width, height;
> +GstStructure *s;
> +GstBuffer *buffer;
> +GstMapInfo mapinfo;
>
> decoder->timer_id = 0;
>
> g_mutex_lock(&decoder->queues_mutex);
> -SpiceFrame *frame = g_queue_pop_head(decoder->display_queue);
> +frame = g_queue_pop_head(decoder->display_queue);
> g_mutex_unlock(&decoder->queues_mutex);
> +/* If the queue is empty we don't even need to reschedule */
> g_return_val_if_fail(frame, G_SOURCE_REMOVE);
>
> -GstBuffer *buffer = frame->sample ? gst_sample_get_buffer(frame->sample)
> : NULL;
> -GstMapInfo mapinfo;
> if (!frame->sample) {
> spice_warning("got a frame without a sample!");
> -} else if (gst_buffer_map(buffer, &mapinfo, GST_MAP_READ)) {
> -stream_display_frame(decoder->base.stream, frame->msg, mapinfo.data);
> -gst_buffer_unmap(buffer, &mapinfo);
> -} else {
> +goto error;
> +}
> +
> +caps = gst_sample_get_caps(frame->sample);
> +if (!caps) {
> +spice_warning("GStreamer error: could not get the caps of the
> sample");
> +goto error;
> +}
> +
> +s = gst_caps_get_structure(caps, 0);
> +if (!gst_structure_get_int(s, "width", &width) ||
> +!gst_structure_get_int(s, "height", &height)) {
> +spice_warning("GStreamer error: could not get the size of the
> frame");
> +goto error;
> +}
> +
> +buffer = gst_sample_get_buffer(frame->sample);
> +if (!gst_buffer_map(buffer, &mapinfo, GST_MAP_READ)) {
> spice_warning("GStreamer error: could not map the buffer");
> +goto error;
> }
> +
> +stream_display_frame(decoder->base.stream, frame->msg,
> + width, height, mapinfo.data);
> +gst_buffer_unmap(buffer, &mapinfo);
> free_frame(frame);
now, it can skip free_frame.
>
> + error:
> schedule_frame(decoder);
> return G_SOURCE_REMOVE;
> }
> diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
> index 1238b41..3327a6b 100644
> --- a/src/channel-display-mjpeg.c
> +++ b/src/channel-display-mjpeg.c
> @@ -86,20 +86,18 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
> video_decoder)
> {
> MJpegDecoder *decoder = (MJpegDecoder*)video_decoder;
> gboolean back_compat = decoder->base.stream->channel->priv-
> >peer_hdr.major_version == 1;
> -int width;
> -int height;
> uint8_t *dest;
> uint8_t *lines[4];
>
> -stream_get_dimensions(decoder->base.stream, decoder->cur_frame_msg,
> &width, &height);
> -if (decoder->out_size < width * height * 4) {
> +jpeg_read_header(&decoder->mjpeg_cinfo, 1);
> +uint32_t img_size = decoder->mjpeg_cinfo.image_width * decoder-
declaration ;)
> >mjpeg_cinfo.image_height * 4;
> +if (decoder->out_size < img_size) {
> g_free(decoder->out_frame);
> -decoder->out_size = width * height * 4;
> +decoder->out_size = img_size;
> decoder->out_frame = g_malloc(decoder->out_size);
> }
> dest = decoder->out_frame;
>
> -jpeg_read_header(&decoder->mjpeg_cinfo, 1);
> #ifdef JCS_EXTENSIONS
> // requires jpeg-turbo
> if (back_compat)
> @@ -134,9 +132,9 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
> video_decoder)
> for (unsigned int j = 0; j < decoder->mjpeg_cinfo.rec_outbuf_height;
> j++) {
> lines[j] = dest;
> #ifdef JCS_EXTENSIONS
> -dest += 4 * width;
what about having the width variable (const unsigned int)? imo it would be more
readable
> +dest += 4 * decoder->mjpeg_cinfo.image_width;
> #else
> -dest += 3 * width;
> +dest += 3 * decoder->mjpeg_cinfo.image_width;
> #endif
> }
> lines_read = jpeg_read_scanlines(&decoder->mjpeg_cinfo, lines,
> @@ -147,14 +145,14 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
> video_decoder)
> uint32_t *d = (uint32_t *)s;
>
> if (back_compat) {
> -for
[Spice-devel] [PATCH] Constify event_loop_core
Was used as write variable only for testing.
Avoid usage of not constant globals.
Signed-off-by: Frediano Ziglio
---
server/event-loop.c | 6 +++---
server/red-common.h | 2 +-
server/tests/basic_event_loop.c | 20 +++-
3 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/server/event-loop.c b/server/event-loop.c
index 4738ed9..d16aca1 100644
--- a/server/event-loop.c
+++ b/server/event-loop.c
@@ -17,9 +17,9 @@
*/
/*
- *This file export a global variable:
+ * This file export a global variable:
*
- * SpiceCoreInterfaceInternal event_loop_core;
+ * const SpiceCoreInterfaceInternal event_loop_core;
*/
#include "red-common.h"
@@ -168,7 +168,7 @@ static void watch_remove(SpiceWatch *watch)
free(watch);
}
-SpiceCoreInterfaceInternal event_loop_core = {
+const SpiceCoreInterfaceInternal event_loop_core = {
.timer_add = timer_add,
.timer_start = timer_start,
.timer_cancel = timer_cancel,
diff --git a/server/red-common.h b/server/red-common.h
index 7add3d0..7ab7e15 100644
--- a/server/red-common.h
+++ b/server/red-common.h
@@ -58,7 +58,7 @@ struct SpiceCoreInterfaceInternal {
GMainContext *main_context;
};
-extern SpiceCoreInterfaceInternal event_loop_core;
+extern const SpiceCoreInterfaceInternal event_loop_core;
typedef struct RedsState RedsState;
diff --git a/server/tests/basic_event_loop.c b/server/tests/basic_event_loop.c
index b9e1b9c..4820387 100644
--- a/server/tests/basic_event_loop.c
+++ b/server/tests/basic_event_loop.c
@@ -36,6 +36,7 @@ int debug = 0;
} \
}
+static SpiceCoreInterfaceInternal base_core_interface;
static GMainContext *main_context = NULL;
GMainContext *basic_event_loop_get_context(void)
@@ -69,12 +70,12 @@ static void ignore_sigpipe(void)
static SpiceTimer* base_timer_add(SpiceTimerFunc func, void *opaque)
{
-return event_loop_core.timer_add(&event_loop_core, func, opaque);
+return base_core_interface.timer_add(&base_core_interface, func, opaque);
}
static SpiceWatch *base_watch_add(int fd, int event_mask, SpiceWatchFunc func,
void *opaque)
{
-return event_loop_core.watch_add(&event_loop_core, fd, event_mask, func,
opaque);
+return base_core_interface.watch_add(&base_core_interface, fd, event_mask,
func, opaque);
}
static SpiceCoreInterface core = {
@@ -91,13 +92,14 @@ SpiceCoreInterface *basic_event_loop_init(void)
ignore_sigpipe();
spice_assert(main_context == NULL);
main_context = g_main_context_new();
-core.timer_start = event_loop_core.timer_start;
-core.timer_cancel = event_loop_core.timer_cancel;
-core.timer_remove = event_loop_core.timer_remove;
-core.watch_update_mask = event_loop_core.watch_update_mask;
-core.watch_remove = event_loop_core.watch_remove;
-event_loop_core.channel_event = core.channel_event =
event_loop_channel_event;
-event_loop_core.main_context = main_context;
+base_core_interface = event_loop_core;
+core.timer_start = base_core_interface.timer_start;
+core.timer_cancel = base_core_interface.timer_cancel;
+core.timer_remove = base_core_interface.timer_remove;
+core.watch_update_mask = base_core_interface.watch_update_mask;
+core.watch_remove = base_core_interface.watch_remove;
+base_core_interface.channel_event = core.channel_event =
event_loop_channel_event;
+base_core_interface.main_context = main_context;
return &core;
}
--
2.7.4
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH] Make sure g_object_new receive the correct data
g_object_new is a variadic function which take property values.
As compiler cannot check if these property values are correct
make sure they are using casts.
This actully fix a crash in reds.c for 32 bit architectures.
Signed-off-by: Frediano Ziglio
---
server/main-dispatcher.c | 2 +-
server/reds.c| 4 ++--
server/smartcard.c | 4 ++--
server/spicevmc.c| 4 ++--
4 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/server/main-dispatcher.c b/server/main-dispatcher.c
index bc0de24..93de440 100644
--- a/server/main-dispatcher.c
+++ b/server/main-dispatcher.c
@@ -292,7 +292,7 @@ MainDispatcher* main_dispatcher_new(RedsState *reds,
SpiceCoreInterfaceInternal
MainDispatcher *self = g_object_new(TYPE_MAIN_DISPATCHER,
"spice-server", reds,
"core-interface", core,
-"max-message-type",
MAIN_DISPATCHER_NUM_MESSAGES,
+"max-message-type", (guint)
MAIN_DISPATCHER_NUM_MESSAGES,
NULL);
return self;
}
diff --git a/server/reds.c b/server/reds.c
index 4fd1d35..5ec73b7 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -4379,8 +4379,8 @@ static RedCharDeviceVDIPort
*red_char_device_vdi_port_new(RedsState *reds)
{
return g_object_new(RED_TYPE_CHAR_DEVICE_VDIPORT,
"spice-server", reds,
-"client-tokens-interval", REDS_TOKENS_TO_SEND,
-"self-tokens", REDS_NUM_INTERNAL_AGENT_MESSAGES,
+"client-tokens-interval", (guint64)
REDS_TOKENS_TO_SEND,
+"self-tokens", (guint64)
REDS_NUM_INTERNAL_AGENT_MESSAGES,
"opaque", reds,
NULL);
}
diff --git a/server/smartcard.c b/server/smartcard.c
index 872aa1d..9b1b3d6 100644
--- a/server/smartcard.c
+++ b/server/smartcard.c
@@ -261,8 +261,8 @@ static RedCharDeviceSmartcard
*smartcard_device_new(RedsState *reds, SpiceCharDe
char_dev = g_object_new(RED_TYPE_CHAR_DEVICE_SMARTCARD,
"sin", sin,
"spice-server", reds,
-"client-tokens-interval", 0ULL,
-"self-tokens", ~0ULL,
+"client-tokens-interval", (guint64) 0ULL,
+"self-tokens", (guint64) ~0ULL,
NULL);
g_object_set(char_dev, "opaque", char_dev, NULL);
diff --git a/server/spicevmc.c b/server/spicevmc.c
index b662d94..a863e39 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -580,8 +580,8 @@ red_char_device_spicevmc_new(SpiceCharDeviceInstance *sin,
return g_object_new(RED_TYPE_CHAR_DEVICE_SPICEVMC,
"sin", sin,
"spice-server", reds,
-"client-tokens-interval", 0ULL,
-"self-tokens", ~0ULL,
+"client-tokens-interval", (guint64) 0ULL,
+"self-tokens", (guint64) ~0ULL,
"opaque", opaque,
NULL);
}
--
2.7.4
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] Make sure g_object_new receive the correct data
On Fri, Jun 03, 2016 at 12:00:24PM +0100, Frediano Ziglio wrote:
> g_object_new is a variadic function which take property values.
> As compiler cannot check if these property values are correct
> make sure they are using casts.
> This actully fix a crash in reds.c for 32 bit architectures.
>
> Signed-off-by: Frediano Ziglio
> ---
> server/main-dispatcher.c | 2 +-
> server/reds.c| 4 ++--
> server/smartcard.c | 4 ++--
> server/spicevmc.c| 4 ++--
> 4 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/server/main-dispatcher.c b/server/main-dispatcher.c
> index bc0de24..93de440 100644
> --- a/server/main-dispatcher.c
> +++ b/server/main-dispatcher.c
> @@ -292,7 +292,7 @@ MainDispatcher* main_dispatcher_new(RedsState *reds,
> SpiceCoreInterfaceInternal
> MainDispatcher *self = g_object_new(TYPE_MAIN_DISPATCHER,
> "spice-server", reds,
> "core-interface", core,
> -"max-message-type",
> MAIN_DISPATCHER_NUM_MESSAGES,
> +"max-message-type", (guint)
> MAIN_DISPATCHER_NUM_MESSAGES,
> NULL);
The MAIN_DISPATCHER_NUM_MESSAGES message comes from an enum, so technically
it could be as small as a 'char', but in practice it'll always be an int.
In any case var-args will always promote types small that int, to be the
size of an int.
IOW, this cast shouldn't be needed, since it'll already be past as an
int by the compiler.
> diff --git a/server/reds.c b/server/reds.c
> index 4fd1d35..5ec73b7 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -4379,8 +4379,8 @@ static RedCharDeviceVDIPort
> *red_char_device_vdi_port_new(RedsState *reds)
> {
> return g_object_new(RED_TYPE_CHAR_DEVICE_VDIPORT,
> "spice-server", reds,
> -"client-tokens-interval", REDS_TOKENS_TO_SEND,
> -"self-tokens", REDS_NUM_INTERNAL_AGENT_MESSAGES,
> +"client-tokens-interval", (guint64)
> REDS_TOKENS_TO_SEND,
> +"self-tokens", (guint64)
> REDS_NUM_INTERNAL_AGENT_MESSAGES,
> "opaque", reds,
> NULL);
> }
This all make total sense though, since you're forcing use of a larger type
than the compiler would otherwise use - it would use int by default,but you
want a 64-bit type.
> diff --git a/server/smartcard.c b/server/smartcard.c
> index 872aa1d..9b1b3d6 100644
> --- a/server/smartcard.c
> +++ b/server/smartcard.c
> @@ -261,8 +261,8 @@ static RedCharDeviceSmartcard
> *smartcard_device_new(RedsState *reds, SpiceCharDe
> char_dev = g_object_new(RED_TYPE_CHAR_DEVICE_SMARTCARD,
> "sin", sin,
> "spice-server", reds,
> -"client-tokens-interval", 0ULL,
> -"self-tokens", ~0ULL,
> +"client-tokens-interval", (guint64) 0ULL,
> +"self-tokens", (guint64) ~0ULL,
> NULL);
>
> g_object_set(char_dev, "opaque", char_dev, NULL);
> diff --git a/server/spicevmc.c b/server/spicevmc.c
> index b662d94..a863e39 100644
> --- a/server/spicevmc.c
> +++ b/server/spicevmc.c
> @@ -580,8 +580,8 @@ red_char_device_spicevmc_new(SpiceCharDeviceInstance *sin,
> return g_object_new(RED_TYPE_CHAR_DEVICE_SPICEVMC,
> "sin", sin,
> "spice-server", reds,
> -"client-tokens-interval", 0ULL,
> -"self-tokens", ~0ULL,
> +"client-tokens-interval", (guint64) 0ULL,
> +"self-tokens", (guint64) ~0ULL,
> "opaque", opaque,
> NULL);
AFAICT, these are all redundant since the ULL suffice on the constant
should already ensure it is passed as a 64-bit type.
Regards,
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] Make sure g_object_new receive the correct data
>
> On Fri, Jun 03, 2016 at 12:00:24PM +0100, Frediano Ziglio wrote:
> > g_object_new is a variadic function which take property values.
> > As compiler cannot check if these property values are correct
> > make sure they are using casts.
> > This actully fix a crash in reds.c for 32 bit architectures.
> >
> > Signed-off-by: Frediano Ziglio
> > ---
> > server/main-dispatcher.c | 2 +-
> > server/reds.c| 4 ++--
> > server/smartcard.c | 4 ++--
> > server/spicevmc.c| 4 ++--
> > 4 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/server/main-dispatcher.c b/server/main-dispatcher.c
> > index bc0de24..93de440 100644
> > --- a/server/main-dispatcher.c
> > +++ b/server/main-dispatcher.c
> > @@ -292,7 +292,7 @@ MainDispatcher* main_dispatcher_new(RedsState *reds,
> > SpiceCoreInterfaceInternal
> > MainDispatcher *self = g_object_new(TYPE_MAIN_DISPATCHER,
> > "spice-server", reds,
> > "core-interface", core,
> > -"max-message-type",
> > MAIN_DISPATCHER_NUM_MESSAGES,
> > +"max-message-type", (guint)
> > MAIN_DISPATCHER_NUM_MESSAGES,
> > NULL);
>
> The MAIN_DISPATCHER_NUM_MESSAGES message comes from an enum, so technically
> it could be as small as a 'char', but in practice it'll always be an int.
> In any case var-args will always promote types small that int, to be the
> size of an int.
>
> IOW, this cast shouldn't be needed, since it'll already be past as an
> int by the compiler.
>
Agreed, not needed. Just does not hurt as hint to check the type
if changes.
>
> > diff --git a/server/reds.c b/server/reds.c
> > index 4fd1d35..5ec73b7 100644
> > --- a/server/reds.c
> > +++ b/server/reds.c
> > @@ -4379,8 +4379,8 @@ static RedCharDeviceVDIPort
> > *red_char_device_vdi_port_new(RedsState *reds)
> > {
> > return g_object_new(RED_TYPE_CHAR_DEVICE_VDIPORT,
> > "spice-server", reds,
> > -"client-tokens-interval", REDS_TOKENS_TO_SEND,
> > -"self-tokens", REDS_NUM_INTERNAL_AGENT_MESSAGES,
> > +"client-tokens-interval", (guint64)
> > REDS_TOKENS_TO_SEND,
> > +"self-tokens", (guint64)
> > REDS_NUM_INTERNAL_AGENT_MESSAGES,
> > "opaque", reds,
> > NULL);
> > }
>
> This all make total sense though, since you're forcing use of a larger type
> than the compiler would otherwise use - it would use int by default,but you
> want a 64-bit type.
>
Yes, this is the bug. Variadic are promoted to int/unsigned int. In case of
64 bit platform usually is promoted to 64 bit as stated in the ABI so this
is not a problem there but for 32 bit (at least x86) the 64 bit will
be read from the 2 32 bit passes, the value and the next string pointer so
the REDS_NUM_INTERNAL_AGENT_MESSAGES will be interpreted as the next
property name!
>
> > diff --git a/server/smartcard.c b/server/smartcard.c
> > index 872aa1d..9b1b3d6 100644
> > --- a/server/smartcard.c
> > +++ b/server/smartcard.c
> > @@ -261,8 +261,8 @@ static RedCharDeviceSmartcard
> > *smartcard_device_new(RedsState *reds, SpiceCharDe
> > char_dev = g_object_new(RED_TYPE_CHAR_DEVICE_SMARTCARD,
> > "sin", sin,
> > "spice-server", reds,
> > -"client-tokens-interval", 0ULL,
> > -"self-tokens", ~0ULL,
> > +"client-tokens-interval", (guint64) 0ULL,
> > +"self-tokens", (guint64) ~0ULL,
> > NULL);
> >
> > g_object_set(char_dev, "opaque", char_dev, NULL);
> > diff --git a/server/spicevmc.c b/server/spicevmc.c
> > index b662d94..a863e39 100644
> > --- a/server/spicevmc.c
> > +++ b/server/spicevmc.c
> > @@ -580,8 +580,8 @@ red_char_device_spicevmc_new(SpiceCharDeviceInstance
> > *sin,
> > return g_object_new(RED_TYPE_CHAR_DEVICE_SPICEVMC,
> > "sin", sin,
> > "spice-server", reds,
> > -"client-tokens-interval", 0ULL,
> > -"self-tokens", ~0ULL,
> > +"client-tokens-interval", (guint64) 0ULL,
> > +"self-tokens", (guint64) ~0ULL,
> > "opaque", opaque,
> > NULL);
>
> AFAICT, these are all redundant since the ULL suffice on the constant
> should already ensure it is passed as a 64-bit type.
>
Not really portable assumption, in theory could be 128 bit or 256 in
a remote future. I think Gcc already support 128 bit integers.
> Regards,
> Daniel
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lis
Re: [Spice-devel] [PATCH] Make sure g_object_new receive the correct data
On Fri, Jun 03, 2016 at 07:16:57AM -0400, Frediano Ziglio wrote: > > > diff --git a/server/spicevmc.c b/server/spicevmc.c > > > index b662d94..a863e39 100644 > > > --- a/server/spicevmc.c > > > +++ b/server/spicevmc.c > > > @@ -580,8 +580,8 @@ red_char_device_spicevmc_new(SpiceCharDeviceInstance > > > *sin, > > > return g_object_new(RED_TYPE_CHAR_DEVICE_SPICEVMC, > > > "sin", sin, > > > "spice-server", reds, > > > -"client-tokens-interval", 0ULL, > > > -"self-tokens", ~0ULL, > > > +"client-tokens-interval", (guint64) 0ULL, > > > +"self-tokens", (guint64) ~0ULL, > > > "opaque", opaque, > > > NULL); > > > > AFAICT, these are all redundant since the ULL suffice on the constant > > should already ensure it is passed as a 64-bit type. > > > > Not really portable assumption, in theory could be 128 bit or 256 in > a remote future. I think Gcc already support 128 bit integers. Haha, this will be the least of our worries if platforms actually get created using 128 bit or larger integers. It'll make the switch from 32-bit to 64-bit look like child's play. Pretty much every piece of software I've seen will break horribly in multiple ways. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH 1/2] fix integer overflows in red_get_path
Use 64 bit arithmetic to avoid overflows.
The multiplication between count and a constant can overflow.
Signed-off-by: Frediano Ziglio
---
server/red-parse-qxl.c | 13 -
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
index 0fdf912..7678c7e 100644
--- a/server/red-parse-qxl.c
+++ b/server/red-parse-qxl.c
@@ -246,7 +246,8 @@ static SpicePath *red_get_path(RedMemSlotInfo *slots, int
group_id,
bool free_data;
QXLPath *qxl;
SpicePath *red;
-size_t size, mem_size, mem_size2, dsize, segment_size;
+size_t size;
+uint64_t mem_size, mem_size2, segment_size;
int n_segments;
int i;
uint32_t count;
@@ -273,7 +274,7 @@ static SpicePath *red_get_path(RedMemSlotInfo *slots, int
group_id,
while (start+1 < end) {
n_segments++;
count = start->count;
-segment_size = sizeof(SpicePathSeg) + count * sizeof(SpicePointFix);
+segment_size = sizeof(SpicePathSeg) + (uint64_t) count *
sizeof(SpicePointFix);
mem_size += sizeof(SpicePathSeg *) + SPICE_ALIGN(segment_size, 4);
start = (QXLPathSeg*)(&start->points[count]);
}
@@ -292,14 +293,8 @@ static SpicePath *red_get_path(RedMemSlotInfo *slots, int
group_id,
/* Protect against overflow in size calculations before
writing to memory */
-spice_assert(mem_size2 + sizeof(SpicePathSeg) > mem_size2);
-mem_size2 += sizeof(SpicePathSeg);
-spice_assert(count < UINT32_MAX / sizeof(SpicePointFix));
-dsize = count * sizeof(SpicePointFix);
-spice_assert(mem_size2 + dsize > mem_size2);
-mem_size2 += dsize;
-
/* Verify that we didn't overflow due to guest changing data */
+mem_size2 += sizeof(SpicePathSeg) + (uint64_t) count *
sizeof(SpicePointFix);
spice_assert(mem_size2 <= mem_size);
seg->flags = start->flags;
--
2.7.4
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH 0/2] Minor security improvements for 32 bit architectures
Mostly improvements. Frediano Ziglio (2): fix integer overflows in red_get_path avoid integer underflow under 32 bit architectures server/red-parse-qxl.c | 16 +++- 1 file changed, 7 insertions(+), 9 deletions(-) -- 2.7.4 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH 2/2] avoid integer underflow under 32 bit architectures
The segment_size computation on 32 bit can lead to big numbers which can lead to negative offset. As we test we don't overrun the buffer avoid to underrun it as we don't have a check for this. Signed-off-by: Frediano Ziglio --- server/red-parse-qxl.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c index 7678c7e..721c861 100644 --- a/server/red-parse-qxl.c +++ b/server/red-parse-qxl.c @@ -276,6 +276,9 @@ static SpicePath *red_get_path(RedMemSlotInfo *slots, int group_id, count = start->count; segment_size = sizeof(SpicePathSeg) + (uint64_t) count * sizeof(SpicePointFix); mem_size += sizeof(SpicePathSeg *) + SPICE_ALIGN(segment_size, 4); +/* avoid going backward with 32 bit architectures */ +spice_assert((uint64_t) count * sizeof(QXLPointFix) + <= (char*) end - (char*) &start->points[0]); start = (QXLPathSeg*)(&start->points[count]); } -- 2.7.4 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-gtk v2] widget: Avoid using GDK_GRAB_FAILED defined in Gtk 3.16.
The returned value from do_pointer_grab() is treated as a boolean - grab
was successful or not. Change the function to return a boolean value.
Reported-by: Eduardo Lima (Etrunko)
---
src/spice-widget.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/src/spice-widget.c b/src/spice-widget.c
index d0fa912..0c70ce4 100644
--- a/src/spice-widget.c
+++ b/src/spice-widget.c
@@ -930,12 +930,13 @@ error:
}
#endif
-static GdkGrabStatus do_pointer_grab(SpiceDisplay *display)
+static gboolean do_pointer_grab(SpiceDisplay *display)
{
SpiceDisplayPrivate *d = display->priv;
GdkWindow *window = GDK_WINDOW(gtk_widget_get_window(GTK_WIDGET(display)));
-GdkGrabStatus status = GDK_GRAB_FAILED;
+GdkGrabStatus status;
GdkCursor *blank = get_blank_cursor();
+gboolean grab_successful = FALSE;
if (!gtk_widget_get_realized(GTK_WIDGET(display)))
goto end;
@@ -964,7 +965,8 @@ static GdkGrabStatus do_pointer_grab(SpiceDisplay *display)
NULL,
blank,
GDK_CURRENT_TIME);
-if (status != GDK_GRAB_SUCCESS) {
+grab_successful = (status == GDK_GRAB_SUCCESS);
+if (!grab_successful) {
d->mouse_grab_active = false;
g_warning("pointer grab failed %u", status);
} else {
@@ -976,7 +978,7 @@ static GdkGrabStatus do_pointer_grab(SpiceDisplay *display)
end:
g_object_unref(blank);
-return status;
+return grab_successful;
}
static void update_mouse_pointer(SpiceDisplay *display)
@@ -1023,7 +1025,7 @@ static void try_mouse_grab(SpiceDisplay *display)
if (d->mouse_grab_active)
return;
-if (do_pointer_grab(display) != GDK_GRAB_SUCCESS)
+if (!do_pointer_grab(display))
return;
d->mouse_last_x = -1;
--
2.8.3
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v3] streaming: Use the frame dimensions reported by the video decoder
The dimensions sent by the remote end are redundant and should not be
trusted.
Signed-off-by: Francois Gouget
---
src/channel-display-gst.c | 41 +
src/channel-display-mjpeg.c | 11 ++-
src/channel-display-priv.h | 3 +--
src/channel-display.c | 24 +---
4 files changed, 41 insertions(+), 38 deletions(-)
diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
index ca6b6e7..c752639 100644
--- a/src/channel-display-gst.c
+++ b/src/channel-display-gst.c
@@ -87,26 +87,51 @@ static void schedule_frame(SpiceGstDecoder *decoder);
static gboolean display_frame(gpointer video_decoder)
{
SpiceGstDecoder *decoder = (SpiceGstDecoder*)video_decoder;
+SpiceFrame *frame;
+GstCaps *caps;
+gint width, height;
+GstStructure *s;
+GstBuffer *buffer;
+GstMapInfo mapinfo;
decoder->timer_id = 0;
g_mutex_lock(&decoder->queues_mutex);
-SpiceFrame *frame = g_queue_pop_head(decoder->display_queue);
+frame = g_queue_pop_head(decoder->display_queue);
g_mutex_unlock(&decoder->queues_mutex);
+/* If the queue is empty we don't even need to reschedule */
g_return_val_if_fail(frame, G_SOURCE_REMOVE);
-GstBuffer *buffer = frame->sample ? gst_sample_get_buffer(frame->sample) :
NULL;
-GstMapInfo mapinfo;
if (!frame->sample) {
spice_warning("got a frame without a sample!");
-} else if (gst_buffer_map(buffer, &mapinfo, GST_MAP_READ)) {
-stream_display_frame(decoder->base.stream, frame->msg, mapinfo.data);
-gst_buffer_unmap(buffer, &mapinfo);
-} else {
+goto error;
+}
+
+caps = gst_sample_get_caps(frame->sample);
+if (!caps) {
+spice_warning("GStreamer error: could not get the caps of the sample");
+goto error;
+}
+
+s = gst_caps_get_structure(caps, 0);
+if (!gst_structure_get_int(s, "width", &width) ||
+!gst_structure_get_int(s, "height", &height)) {
+spice_warning("GStreamer error: could not get the size of the frame");
+goto error;
+}
+
+buffer = gst_sample_get_buffer(frame->sample);
+if (!gst_buffer_map(buffer, &mapinfo, GST_MAP_READ)) {
spice_warning("GStreamer error: could not map the buffer");
+goto error;
}
-free_frame(frame);
+stream_display_frame(decoder->base.stream, frame->msg,
+ width, height, mapinfo.data);
+gst_buffer_unmap(buffer, &mapinfo);
+
+ error:
+free_frame(frame);
schedule_frame(decoder);
return G_SOURCE_REMOVE;
}
diff --git a/src/channel-display-mjpeg.c b/src/channel-display-mjpeg.c
index a2dae82..4976d53 100644
--- a/src/channel-display-mjpeg.c
+++ b/src/channel-display-mjpeg.c
@@ -86,12 +86,13 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
video_decoder)
{
MJpegDecoder *decoder = (MJpegDecoder*)video_decoder;
gboolean back_compat =
decoder->base.stream->channel->priv->peer_hdr.major_version == 1;
-int width;
-int height;
+JDIMENSION width, height;
uint8_t *dest;
uint8_t *lines[4];
-stream_get_dimensions(decoder->base.stream, decoder->cur_frame_msg,
&width, &height);
+jpeg_read_header(&decoder->mjpeg_cinfo, 1);
+width = decoder->mjpeg_cinfo.image_width;
+height = decoder->mjpeg_cinfo.image_height;
if (decoder->out_size < width * height * 4) {
g_free(decoder->out_frame);
decoder->out_size = width * height * 4;
@@ -99,7 +100,6 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
video_decoder)
}
dest = decoder->out_frame;
-jpeg_read_header(&decoder->mjpeg_cinfo, 1);
#ifdef JCS_EXTENSIONS
// requires jpeg-turbo
if (back_compat)
@@ -168,7 +168,8 @@ static gboolean mjpeg_decoder_decode_frame(gpointer
video_decoder)
jpeg_finish_decompress(&decoder->mjpeg_cinfo);
/* Display the frame and dispose of it */
-stream_display_frame(decoder->base.stream, decoder->cur_frame_msg,
decoder->out_frame);
+stream_display_frame(decoder->base.stream, decoder->cur_frame_msg,
+ width, height, decoder->out_frame);
spice_msg_in_unref(decoder->cur_frame_msg);
decoder->cur_frame_msg = NULL;
decoder->timer_id = 0;
diff --git a/src/channel-display-priv.h b/src/channel-display-priv.h
index 3155015..3fcf2e2 100644
--- a/src/channel-display-priv.h
+++ b/src/channel-display-priv.h
@@ -135,10 +135,9 @@ struct display_stream {
uint32_t report_drops_seq_len;
};
-void stream_get_dimensions(display_stream *st, SpiceMsgIn *frame_msg, int
*width, int *height);
guint32 stream_get_time(display_stream *st);
void stream_dropped_frame_on_playback(display_stream *st);
-void stream_display_frame(display_stream *st, SpiceMsgIn *frame_msg, uint8_t*
data);
+void stream_display_frame(display_stream *st, SpiceMsgIn *frame_msg, uint32_t
width, uint32_t height, uint8_t *data);
uint32_t spice_msg_in_frame_data(SpiceMsgIn *frame
[Spice-devel] [spice-server] wip: to better sync server and client tokens
---
server/char-device.c | 6 ++
server/reds.c| 8 ++--
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/server/char-device.c b/server/char-device.c
index cb35aa2..84efdfb 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);
+
+/* FIXME: WIP patch: If the device is reset and we clear all the
+ * WriteBuffers, we must garantee that the number of tokens is exactly
+ * the same that we will send to the client upon agent re-connection.
*/
+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..1cd697d 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -553,12 +553,8 @@ static void reds_reset_vdp(RedsState *reds)
* 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.
*/
-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
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH] Remove only written field
EncoderData::dcc field is never read back.
Signed-off-by: Frediano Ziglio
---
server/dcc-encoders.c | 1 -
server/dcc-encoders.h | 1 -
server/dcc.c | 2 --
3 files changed, 4 deletions(-)
diff --git a/server/dcc-encoders.c b/server/dcc-encoders.c
index d4a0ec5..218522e 100644
--- a/server/dcc-encoders.c
+++ b/server/dcc-encoders.c
@@ -143,7 +143,6 @@ void encoder_data_init(EncoderData *data,
DisplayChannelClient *dcc)
{
data->bufs_tail = g_new(RedCompressBuf, 1);
data->bufs_head = data->bufs_tail;
-data->dcc = dcc;
data->bufs_head->send_next = NULL;
}
diff --git a/server/dcc-encoders.h b/server/dcc-encoders.h
index b66fb1e..82750be 100644
--- a/server/dcc-encoders.h
+++ b/server/dcc-encoders.h
@@ -80,7 +80,6 @@ GlzSharedDictionary* dcc_restore_glz_dictionary
(DisplayChannelClie
GlzEncDictRestoreData *restore_data);
typedef struct {
-DisplayChannelClient *dcc;
RedCompressBuf *bufs_head;
RedCompressBuf *bufs_tail;
jmp_buf jmp_env;
diff --git a/server/dcc.c b/server/dcc.c
index ef43a3c..9cc651b 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -938,8 +938,6 @@ static int dcc_compress_image_jpeg(DisplayChannelClient
*dcc, SpiceImage *dest,
comp_head_left = sizeof(lz_data->data.bufs_head->buf) - comp_head_filled;
lz_out_start_byte = lz_data->data.bufs_head->buf.bytes + comp_head_filled;
-lz_data->data.dcc = dcc;
-
lz_data->data.u.lines_data.chunks = src->data;
lz_data->data.u.lines_data.stride = src->stride;
lz_data->data.u.lines_data.next = 0;
--
2.7.4
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice v13 10/29] server: Handle and recover from GStreamer encoding errors
On Wed, May 04, 2016 at 11:21:13AM +0200, Francois Gouget wrote:
> On Tue, 3 May 2016, Christophe Fergeau wrote:
> [...]
> > And GStreamer GstBus/GstAppSink API tends to be async, while we need
> > encode_frame() to be sync, so there is indeed several useful APIs that
> > we cannot use without additional complexity. I'm wondering if the
> > simpler patch attached to this message could work too.
>
> I had tried that but while it works you get a big warning that changing
> the state of the pipeline from the streaming thread is not allowed:
>
> (Xorg:29234): Spice-WARNING **:
> gstreamer-encoder.c:809:handle_pipeline_message: GStreamer error from element
> encoder: Can not initialize x264 encoder.
> gstreamer-encoder.c:809:handle_pipeline_message: GStreamer error from element
> encoder: Can not initialize x264 encoder.
> gstreamer-encoder.c:811:handle_pipeline_message: debug details:
> gstx264enc.c(1269): gst_x264_enc_init_encoder ():
> /GstPipeline:pipeline3/GstX264Enc:encoder
> 0:00:09.382605905 29234 0x7f3de0855e80 DEBUG GST_STATES
> gstelement.c:2638:gst_element_set_state_func: set_state to NULL
> [...]
> 0:00:09.384261060 29234 0x7f3de0855e80 WARNtask
> gsttask.c:862:gst_task_join: trying to join task from its thread
>
> (Xorg:29234): GStreamer-WARNING **:
> Trying to join task 0x7f3de00f7560 from its thread would deadlock.
> You cannot change the state of an element from its streaming
> thread. Use g_idle_add() or post a GstMessage on the bus to
> schedule the state change from the main thread.
>
> 0:00:09.384286706 29234 0x7f3de0855e80 DEBUG GST_PADS
> gstpad.c:5731:gst_pad_stop_task: join failed
> 0:00:09.384291170 29234 0x7f3de0855e80 DEBUGbasesrc
> gstbasesrc.c:2854:gst_base_src_stop: stopping source
For what it's worth, I tried asking GStreamer people if they had
suggestions how we could handle what is being done in this patch, but
they did not have a silver bullet :)
Suggestions included running our own mainloop/maincontext in
encode_frame(), but this would probably be not much different than
having the GCond.
Another option would be a slight variation on this patch which led to
the warnings above ("cannot set pipeline state from streaming thread"):
rather than calling gst_element_set_state() directly, call
g_thread_new(function_calling_gst_element_set_state). I think this could
work, but haven't been lucky in reproducing the initial bug/this warning
:(
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] [spice-gtk v3 05/16] file-xfer: inform agent of errors only when task finished
Related to my comments to the last patch, this change makes the
SpiceFileTransferTask class less "self-contained". In other words, if the user
does not connect to the 'finished' signal and send the appropriate agent
XFER_STATUS message in that handler, the file transfer won't work properly. I
think it's probably more maintainable and less error-prone if the
SpiceFileTransferTask has the responsibility for all of the logic for conducting
the file transfer.
I see Pavel already acked this patch, so I'm curious to hear both of your
thoughts about the above comments.
Jonathon
On Mon, 2016-05-30 at 11:55 +0200, Victor Toso wrote:
> No need to inform of a problem under
> spice_file_transfer_task_completed() as the task will be finalized and
> we can send the error to the agent there.
>
> This change is related to split SpiceFileTransferTask from
> channel-main.
>
> Acked-by: Pavel Grunt
> ---
> src/channel-main.c | 19 +--
> 1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/src/channel-main.c b/src/channel-main.c
> index 0ed322e..72dcf1f 100644
> --- a/src/channel-main.c
> +++ b/src/channel-main.c
> @@ -2968,16 +2968,6 @@ static void
> spice_file_transfer_task_completed(SpiceFileTransferTask *self,
> self->error = error;
> }
>
> -if (self->error) {
> -VDAgentFileXferStatusMessage msg = {
> -.id = self->id,
> -.result = self->error->code == G_IO_ERROR_CANCELLED ?
> -VD_AGENT_FILE_XFER_STATUS_CANCELLED :
> VD_AGENT_FILE_XFER_STATUS_ERROR,
> -};
> -agent_msg_queue_many(self->channel, VD_AGENT_FILE_XFER_STATUS,
> - &msg, sizeof(msg), NULL);
> -}
> -
> if (self->pending)
> return;
>
> @@ -3100,6 +3090,15 @@ static void task_finished(SpiceFileTransferTask *task,
> SpiceMainChannel *channel = SPICE_MAIN_CHANNEL(data);
> guint32 task_id = spice_file_transfer_task_get_id(task);
>
> +if (error) {
> +VDAgentFileXferStatusMessage msg;
> +msg.id = task_id;
> +msg.result = error->code == G_IO_ERROR_CANCELLED ?
> +VD_AGENT_FILE_XFER_STATUS_CANCELLED :
> VD_AGENT_FILE_XFER_STATUS_ERROR;
> +agent_msg_queue_many(channel, VD_AGENT_FILE_XFER_STATUS,
> + &msg, sizeof(msg), NULL);
> +}
> +
> g_hash_table_remove(channel->priv->file_xfer_tasks,
> GUINT_TO_POINTER(task_id));
> }
>
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] Remove only written field
Acked-by: Jonathon Jongsma
On Fri, 2016-06-03 at 14:43 +0100, Frediano Ziglio wrote:
> EncoderData::dcc field is never read back.
>
> Signed-off-by: Frediano Ziglio
> ---
> server/dcc-encoders.c | 1 -
> server/dcc-encoders.h | 1 -
> server/dcc.c | 2 --
> 3 files changed, 4 deletions(-)
>
> diff --git a/server/dcc-encoders.c b/server/dcc-encoders.c
> index d4a0ec5..218522e 100644
> --- a/server/dcc-encoders.c
> +++ b/server/dcc-encoders.c
> @@ -143,7 +143,6 @@ void encoder_data_init(EncoderData *data,
> DisplayChannelClient *dcc)
> {
> data->bufs_tail = g_new(RedCompressBuf, 1);
> data->bufs_head = data->bufs_tail;
> -data->dcc = dcc;
> data->bufs_head->send_next = NULL;
> }
>
> diff --git a/server/dcc-encoders.h b/server/dcc-encoders.h
> index b66fb1e..82750be 100644
> --- a/server/dcc-encoders.h
> +++ b/server/dcc-encoders.h
> @@ -80,7 +80,6 @@ GlzSharedDictionary*
> dcc_restore_glz_dictionary (DisplayChannelClie
> GlzEncDictResto
> reData *restore_data);
>
> typedef struct {
> -DisplayChannelClient *dcc;
> RedCompressBuf *bufs_head;
> RedCompressBuf *bufs_tail;
> jmp_buf jmp_env;
> diff --git a/server/dcc.c b/server/dcc.c
> index ef43a3c..9cc651b 100644
> --- a/server/dcc.c
> +++ b/server/dcc.c
> @@ -938,8 +938,6 @@ static int dcc_compress_image_jpeg(DisplayChannelClient
> *dcc, SpiceImage *dest,
> comp_head_left = sizeof(lz_data->data.bufs_head->buf) - comp_head_filled;
> lz_out_start_byte = lz_data->data.bufs_head->buf.bytes +
> comp_head_filled;
>
> -lz_data->data.dcc = dcc;
> -
> lz_data->data.u.lines_data.chunks = src->data;
> lz_data->data.u.lines_data.stride = src->stride;
> lz_data->data.u.lines_data.next = 0;
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH] Remove unused parameter
Signed-off-by: Frediano Ziglio
---
server/dcc-encoders.c | 2 +-
server/dcc-encoders.h | 2 +-
server/dcc.c | 12 ++--
3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/server/dcc-encoders.c b/server/dcc-encoders.c
index 218522e..5570798 100644
--- a/server/dcc-encoders.c
+++ b/server/dcc-encoders.c
@@ -139,7 +139,7 @@ static void glz_usr_free(GlzEncoderUsrContext *usr, void
*ptr)
free(ptr);
}
-void encoder_data_init(EncoderData *data, DisplayChannelClient *dcc)
+void encoder_data_init(EncoderData *data)
{
data->bufs_tail = g_new(RedCompressBuf, 1);
data->bufs_head = data->bufs_tail;
diff --git a/server/dcc-encoders.h b/server/dcc-encoders.h
index 82750be..0d3e96a9 100644
--- a/server/dcc-encoders.h
+++ b/server/dcc-encoders.h
@@ -98,7 +98,7 @@ typedef struct {
char message_buf[512];
} EncoderData;
-void encoder_data_init(EncoderData *data, DisplayChannelClient *dcc);
+void encoder_data_init(EncoderData *data);
void encoder_data_reset(EncoderData *data);
typedef struct {
diff --git a/server/dcc.c b/server/dcc.c
index 9cc651b..336bae0 100644
--- a/server/dcc.c
+++ b/server/dcc.c
@@ -719,7 +719,7 @@ static int dcc_compress_image_glz(DisplayChannelClient *dcc,
spice_info("LZ global compress fmt=%d", src->format);
#endif
-encoder_data_init(&glz_data->data, dcc);
+encoder_data_init(&glz_data->data);
glz_drawable = get_glz_drawable(dcc, drawable);
glz_drawable_instance = add_glz_drawable_instance(glz_drawable);
@@ -744,7 +744,7 @@ static int dcc_compress_image_glz(DisplayChannelClient *dcc,
stat_start_time_init(&start_time, &display_channel->zlib_glz_stat);
zlib_data = &dcc->zlib_data;
-encoder_data_init(&zlib_data->data, dcc);
+encoder_data_init(&zlib_data->data);
zlib_data->data.u.compressed_data.next = glz_data->data.bufs_head;
zlib_data->data.u.compressed_data.size_left = glz_size;
@@ -796,7 +796,7 @@ static int dcc_compress_image_lz(DisplayChannelClient *dcc,
spice_info("LZ LOCAL compress");
#endif
-encoder_data_init(&lz_data->data, dcc);
+encoder_data_init(&lz_data->data);
if (setjmp(lz_data->data.jmp_env)) {
encoder_data_reset(&lz_data->data);
@@ -886,7 +886,7 @@ static int dcc_compress_image_jpeg(DisplayChannelClient
*dcc, SpiceImage *dest,
return FALSE;
}
-encoder_data_init(&jpeg_data->data, dcc);
+encoder_data_init(&jpeg_data->data);
if (setjmp(jpeg_data->data.jmp_env)) {
encoder_data_reset(&jpeg_data->data);
@@ -985,7 +985,7 @@ static int dcc_compress_image_lz4(DisplayChannelClient
*dcc, SpiceImage *dest,
spice_info("LZ4 compress");
#endif
-encoder_data_init(&lz4_data->data, dcc);
+encoder_data_init(&lz4_data->data);
if (setjmp(lz4_data->data.jmp_env)) {
encoder_data_reset(&lz4_data->data);
@@ -1053,7 +1053,7 @@ static int dcc_compress_image_quic(DisplayChannelClient
*dcc, SpiceImage *dest,
return FALSE;
}
-encoder_data_init(&quic_data->data, dcc);
+encoder_data_init(&quic_data->data);
if (setjmp(quic_data->data.jmp_env)) {
encoder_data_reset(&quic_data->data);
--
2.7.4
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH 1/2] Add compress_buf_free function
dcc-encoders should be in change of allocate and free the structure
don't put internal assumptions (which functions are used for memory
management) in different files.
Signed-off-by: Frediano Ziglio
---
server/dcc-encoders.h | 5 +
1 file changed, 5 insertions(+)
diff --git a/server/dcc-encoders.h b/server/dcc-encoders.h
index 0d3e96a9..88351dc 100644
--- a/server/dcc-encoders.h
+++ b/server/dcc-encoders.h
@@ -63,6 +63,11 @@ struct RedCompressBuf {
RedCompressBuf *send_next;
};
+static inline void compress_buf_free(RedCompressBuf *buf)
+{
+g_free(buf);
+}
+
typedef struct GlzSharedDictionary {
RingItem base;
GlzEncDictContext *dict;
--
2.7.4
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH 2/2] Move marshalling code from dcc-encoders to dcc-send
No reasons why dcc-encoders should know about marshalling.
Signed-off-by: Frediano Ziglio
---
server/dcc-encoders.c | 20
server/dcc-encoders.h | 5 -
server/dcc-send.c | 20
3 files changed, 20 insertions(+), 25 deletions(-)
diff --git a/server/dcc-encoders.c b/server/dcc-encoders.c
index 5570798..a657887 100644
--- a/server/dcc-encoders.c
+++ b/server/dcc-encoders.c
@@ -429,26 +429,6 @@ void dcc_encoders_free(DisplayChannelClient *dcc)
dcc->zlib = NULL;
}
-static void marshaller_compress_buf_free(uint8_t *data, void *opaque)
-{
-g_free(opaque);
-}
-
-void marshaller_add_compressed(SpiceMarshaller *m,
- RedCompressBuf *comp_buf, size_t size)
-{
-size_t max = size;
-size_t now;
-do {
-spice_return_if_fail(comp_buf);
-now = MIN(sizeof(comp_buf->buf), max);
-max -= now;
-spice_marshaller_add_ref_full(m, comp_buf->buf.bytes, now,
- marshaller_compress_buf_free, comp_buf);
-comp_buf = comp_buf->send_next;
-} while (max);
-}
-
/* Remove from the to_free list and the instances_list.
When no instance is left - the RedGlzDrawable is released too. (and the qxl
drawable too, if
it is not used by Drawable).
diff --git a/server/dcc-encoders.h b/server/dcc-encoders.h
index 88351dc..8c3f22f 100644
--- a/server/dcc-encoders.h
+++ b/server/dcc-encoders.h
@@ -19,7 +19,6 @@
#define DCC_ENCODERS_H_
#include
-#include
#include
#include "red-channel.h"
@@ -46,10 +45,6 @@ void dcc_free_glz_drawables_to_free
(DisplayChannelClie
void dcc_freeze_glz
(DisplayChannelClient *dcc);
void dcc_release_glz
(DisplayChannelClient *dcc);
-void marshaller_add_compressed (SpiceMarshaller
*m,
- RedCompressBuf
*comp_buf,
- size_t size);
-
#define RED_COMPRESS_BUF_SIZE (1024 * 64)
struct RedCompressBuf {
/* This buffer provide space for compression algorithms.
diff --git a/server/dcc-send.c b/server/dcc-send.c
index 5171f9a..6c10565 100644
--- a/server/dcc-send.c
+++ b/server/dcc-send.c
@@ -335,6 +335,26 @@ static void fill_base(SpiceMarshaller *base_marshaller,
Drawable *drawable)
spice_marshall_DisplayBase(base_marshaller, &base);
}
+static void marshaller_compress_buf_free(uint8_t *data, void *opaque)
+{
+compress_buf_free(opaque);
+}
+
+static void marshaller_add_compressed(SpiceMarshaller *m,
+ RedCompressBuf *comp_buf, size_t size)
+{
+size_t max = size;
+size_t now;
+do {
+spice_return_if_fail(comp_buf);
+now = MIN(sizeof(comp_buf->buf), max);
+max -= now;
+spice_marshaller_add_ref_full(m, comp_buf->buf.bytes, now,
+ marshaller_compress_buf_free, comp_buf);
+comp_buf = comp_buf->send_next;
+} while (max);
+}
+
/* if the number of times fill_bits can be called per one qxl_drawable
increases -
MAX_LZ_DRAWABLE_INSTANCES must be increased as well */
static FillBitsType fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m,
--
2.7.4
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-server] wip: to better sync server and client tokens
>
> ---
> server/char-device.c | 6 ++
> server/reds.c| 8 ++--
> 2 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/server/char-device.c b/server/char-device.c
> index cb35aa2..84efdfb 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);
> +
> +/* FIXME: WIP patch: If the device is reset and we clear all the
> + * WriteBuffers, we must garantee that the number of tokens is
> exactly
> + * the same that we will send to the client upon agent
> re-connection. */
> +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);
> }
Yes, make sense. Perhaps a check that there are no connected client
but should be done in a reset function, or perhaps a client should
be passed to this function (well, multiple clients are really never
going to work reliably probably).
> diff --git a/server/reds.c b/server/reds.c
> index 4fd1d35..1cd697d 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -553,12 +553,8 @@ static void reds_reset_vdp(RedsState *reds)
> * 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.
> */
> -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) {
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] Remove unused parameter
Acked-by: Jonathon Jongsma
On Fri, 2016-06-03 at 16:35 +0100, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio
> ---
> server/dcc-encoders.c | 2 +-
> server/dcc-encoders.h | 2 +-
> server/dcc.c | 12 ++--
> 3 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/server/dcc-encoders.c b/server/dcc-encoders.c
> index 218522e..5570798 100644
> --- a/server/dcc-encoders.c
> +++ b/server/dcc-encoders.c
> @@ -139,7 +139,7 @@ static void glz_usr_free(GlzEncoderUsrContext *usr, void
> *ptr)
> free(ptr);
> }
>
> -void encoder_data_init(EncoderData *data, DisplayChannelClient *dcc)
> +void encoder_data_init(EncoderData *data)
> {
> data->bufs_tail = g_new(RedCompressBuf, 1);
> data->bufs_head = data->bufs_tail;
> diff --git a/server/dcc-encoders.h b/server/dcc-encoders.h
> index 82750be..0d3e96a9 100644
> --- a/server/dcc-encoders.h
> +++ b/server/dcc-encoders.h
> @@ -98,7 +98,7 @@ typedef struct {
> char message_buf[512];
> } EncoderData;
>
> -void encoder_data_init(EncoderData *data, DisplayChannelClient *dcc);
> +void encoder_data_init(EncoderData *data);
> void encoder_data_reset(EncoderData *data);
>
> typedef struct {
> diff --git a/server/dcc.c b/server/dcc.c
> index 9cc651b..336bae0 100644
> --- a/server/dcc.c
> +++ b/server/dcc.c
> @@ -719,7 +719,7 @@ static int dcc_compress_image_glz(DisplayChannelClient
> *dcc,
> spice_info("LZ global compress fmt=%d", src->format);
> #endif
>
> -encoder_data_init(&glz_data->data, dcc);
> +encoder_data_init(&glz_data->data);
>
> glz_drawable = get_glz_drawable(dcc, drawable);
> glz_drawable_instance = add_glz_drawable_instance(glz_drawable);
> @@ -744,7 +744,7 @@ static int dcc_compress_image_glz(DisplayChannelClient
> *dcc,
> stat_start_time_init(&start_time, &display_channel->zlib_glz_stat);
> zlib_data = &dcc->zlib_data;
>
> -encoder_data_init(&zlib_data->data, dcc);
> +encoder_data_init(&zlib_data->data);
>
> zlib_data->data.u.compressed_data.next = glz_data->data.bufs_head;
> zlib_data->data.u.compressed_data.size_left = glz_size;
> @@ -796,7 +796,7 @@ static int dcc_compress_image_lz(DisplayChannelClient
> *dcc,
> spice_info("LZ LOCAL compress");
> #endif
>
> -encoder_data_init(&lz_data->data, dcc);
> +encoder_data_init(&lz_data->data);
>
> if (setjmp(lz_data->data.jmp_env)) {
> encoder_data_reset(&lz_data->data);
> @@ -886,7 +886,7 @@ static int dcc_compress_image_jpeg(DisplayChannelClient
> *dcc, SpiceImage *dest,
> return FALSE;
> }
>
> -encoder_data_init(&jpeg_data->data, dcc);
> +encoder_data_init(&jpeg_data->data);
>
> if (setjmp(jpeg_data->data.jmp_env)) {
> encoder_data_reset(&jpeg_data->data);
> @@ -985,7 +985,7 @@ static int dcc_compress_image_lz4(DisplayChannelClient
> *dcc, SpiceImage *dest,
> spice_info("LZ4 compress");
> #endif
>
> -encoder_data_init(&lz4_data->data, dcc);
> +encoder_data_init(&lz4_data->data);
>
> if (setjmp(lz4_data->data.jmp_env)) {
> encoder_data_reset(&lz4_data->data);
> @@ -1053,7 +1053,7 @@ static int dcc_compress_image_quic(DisplayChannelClient
> *dcc, SpiceImage *dest,
> return FALSE;
> }
>
> -encoder_data_init(&quic_data->data, dcc);
> +encoder_data_init(&quic_data->data);
>
> if (setjmp(quic_data->data.jmp_env)) {
> encoder_data_reset(&quic_data->data);
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [spice-gtk v3 06/16] file-xfer: a FileTransferOperation per transfer call
On Mon, 2016-05-30 at 11:55 +0200, Victor Toso wrote:
> Each call to spice_main_file_copy_async will now create a
> FileTransferOperation which groups all SpiceFileTransferTasks of the
> copy operation and also the progress_callback passed from Application.
>
> As pointed in the fix 113093dd00a1cf10f6d3c3589b7589a184cec081, the
> progress_callback should provide information of the whole transfer
> operation; For that reason, there is no need to keep progress_callback
> and progress_callback_data per SpiceFileTransferTask but per
> FileTransferOperation.
>
> The file_xfer_tasks hash table now has FileTransferOperation instead
> of SpiceFileTransferTask. To improve handling this new operation, I've
> created the helpers:
>
> * file_transfer_operation_send_progress
> * file_transfer_operation_end
> * file_transfer_operation_reset_all
> * file_transfer_operation_find_task_by_id
> * file_transfer_operation_task_finished
>
> This change is related to split SpiceFileTransferTask from
> channel-main.
> ---
> src/channel-main.c | 206 --
> ---
> 1 file changed, 139 insertions(+), 67 deletions(-)
>
> diff --git a/src/channel-main.c b/src/channel-main.c
> index 72dcf1f..f36326d 100644
> --- a/src/channel-main.c
> +++ b/src/channel-main.c
> @@ -66,8 +66,6 @@ static GList
> *spice_file_transfer_task_create_tasks(SpiceMainChannel *channel,
> GFile **files,
> GFileCopyFlags flags,
> GCancellable
> *cancellable,
> -GFileProgressCallback
> progress_callback,
> -gpointer
> progress_callback_data,
> SpiceFileTransferTaskFlus
> hCb flush_callback,
> gpointer
> flush_callback_data,
> GAsyncReadyCallback
> callback,
> @@ -103,8 +101,6 @@ struct _SpiceFileTransferTask
> GFileInputStream *file_stream;
> GFileCopyFlags flags;
> GCancellable *cancellable;
> -GFileProgressCallback progress_callback;
> -gpointer progress_callback_data;
> SpiceFileTransferTaskFlushCb flush_callback;
> gpointer flush_callback_data;
> GAsyncReadyCallbackcallback;
> @@ -156,6 +152,15 @@ typedef struct {
> SpiceDisplayState display_state;
> } SpiceDisplayConfig;
>
> +typedef struct {
> +GList *tasks;
> +SpiceMainChannel *channel;
> +GFileProgressCallback progress_callback;
> +gpointerprogress_callback_data;
> +goffset total_sent;
> +goffset transfer_size;
> +} FileTransferOperation;
> +
> struct _SpiceMainChannelPrivate {
> enum SpiceMouseMode mouse_mode;
> enum SpiceMouseMode requested_mouse_mode;
> @@ -257,6 +262,13 @@ static void file_xfer_flushed(SpiceMainChannel *channel,
> gboolean success);
> static void spice_main_set_max_clipboard(SpiceMainChannel *self, gint max);
> static void set_agent_connected(SpiceMainChannel *channel, gboolean
> connected);
>
> +static void file_transfer_operation_send_progress(SpiceMainChannel *channel,
> SpiceFileTransferTask *xfer_task);
> +static void file_transfer_operation_end(FileTransferOperation *xfer_op);
> +static void file_transfer_operation_reset_all(SpiceMainChannel *channel);
> +static SpiceFileTransferTask
> *file_transfer_operation_find_task_by_id(SpiceMainChannel *channel,
> + guint32
> task_id);
> +static void file_transfer_operation_task_finished(SpiceMainChannel *channel,
> SpiceFileTransferTask *task);
> +
> /* -- */
>
> static const char *agent_msg_types[] = {
> @@ -315,8 +327,7 @@ static void spice_main_channel_init(SpiceMainChannel
> *channel)
>
> c = channel->priv = SPICE_MAIN_CHANNEL_GET_PRIVATE(channel);
> c->agent_msg_queue = g_queue_new();
> -c->file_xfer_tasks = g_hash_table_new_full(g_direct_hash, g_direct_equal,
> - NULL, g_object_unref);
> +c->file_xfer_tasks = g_hash_table_new(g_direct_hash, g_direct_equal);
> c->flushing = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL,
> g_object_unref);
> c->cancellable_volume_info = g_cancellable_new();
> @@ -470,9 +481,6 @@ static void spice_channel_iterate_write(SpiceChannel
> *channel)
> static void spice_main_channel_reset_agent(SpiceMainChannel *channel)
> {
> SpiceMainChannelPrivate *c = channel->p
Re: [Spice-devel] [spice-gtk v3 07/16] file-xfer: call user callback once per operation
On Mon, 2016-05-30 at 11:55 +0200, Victor Toso wrote:
> SpiceFileTransferTask has a callback to be called when operation
> ended. Til this patch, we were setting the user callback which means
> that in multiple file-transfers, we were calling the user callback
> several times.
>
> Following the same logic pointed from 113093dd00a1cf10f6d3c3589b7 this
> is a SpiceMainChannel operation and it should only call the user
> callback when this operation is over (FileTransferOperation now).
> ---
> src/channel-main.c | 72 ++-
> src/tmp-introspect325cwcm0/SpiceClientGtk-3.0.c | 628
>
> 2 files changed, 694 insertions(+), 6 deletions(-)
> create mode 100644 src/tmp-introspect325cwcm0/SpiceClientGtk-3.0.c
>
> diff --git a/src/channel-main.c b/src/channel-main.c
> index f36326d..e204a1e 100644
> --- a/src/channel-main.c
> +++ b/src/channel-main.c
> @@ -157,6 +157,10 @@ typedef struct {
> SpiceMainChannel *channel;
> GFileProgressCallback progress_callback;
> gpointerprogress_callback_data;
> +GAsyncReadyCallback end_callback;
> +gpointerend_callback_data;
> +GError *error;
> +GCancellable *cancellable;
> goffset total_sent;
> goffset transfer_size;
> } FileTransferOperation;
> @@ -1838,9 +1842,7 @@ static void file_xfer_close_cb(GObject *object,
> }
> }
>
> -/* Notify to user that files have been transferred or something error
> - happened. */
> -task = g_task_new(self->channel,
> +task = g_task_new(self,
> self->cancellable,
> self->callback,
> self->user_data);
> @@ -1919,6 +1921,42 @@ static void
> file_xfer_flush_callback(SpiceFileTransferTask *xfer_task,
> file_xfer_flush_async(main_channel, cancellable,
> file_xfer_data_flushed_cb, xfer_task);
> }
>
> +static void file_xfer_end_callback(GObject *source_object,
> + GAsyncResult *res,
> + gpointer user_data)
> +{
> +GTask *task;
> +FileTransferOperation *xfer_op;
> +
> +task = G_TASK(res);
> +if (!g_task_had_error(task))
> +/* SpiceFileTransferTask and FileTransferOperation are freed on
> + * file_transfer_operation_task_finished */
> +return;
In general, a GAsyncReadyCallback is expected to call a _finish() function to
get the error status. Since this is all internal, we can poke around at the
implementation of the asynchronous task to find the error, but I think I'd
prefer to provide a _finish() function to follow the conventions.
> +
> +xfer_op = user_data;
> +
> +if (xfer_op->error != NULL)
> +return;
> +
> +/* Get the GError from SpiceFileTransferTask so we can properly return to
> + * the application when the FileTransferOperation ends */
> +g_task_propagate_boolean(task, &xfer_op->error);
since file_xfer_end_callback() is called once for each file in the operation,
the above line will overwrite xfer_op->error each time it is called. So we'll
only report the error status of the last file that finished. We probably don't
want to overwrite an earlier error with a later success.
> +
> +/* User can cancel a FileTransfer without cancelling the whole
> + * operation. For that, spice_main_file_copy_async must be called
> + * without GCancellabe */
> +if (g_error_matches(xfer_op->error, G_IO_ERROR, G_IO_ERROR_CANCELLED) &&
> +xfer_op->cancellable == NULL) {
> +SpiceFileTransferTask *xfer_task;
> +
> +xfer_task = SPICE_FILE_TRANSFER_TASK(source_object);
> +spice_debug("file-transfer %u was cancelled",
> +spice_file_transfer_task_get_id(xfer_task));
> +g_clear_error(&xfer_op->error);
> +}
> +}
> +
> /* main context */
> static void file_xfer_read_cb(GObject *source_object,
> GAsyncResult *res,
> @@ -3108,10 +3146,24 @@ static void
> file_transfer_operation_end(FileTransferOperation *xfer_op)
> g_return_if_fail(xfer_op != NULL);
> spice_debug("Freeing file-transfer-operation %p", xfer_op);
>
> +if (xfer_op->end_callback) {
> +GTask *task = g_task_new(xfer_op->channel,
> + xfer_op->cancellable,
> + xfer_op->end_callback,
> + xfer_op->end_callback_data);
> +
> +if (xfer_op->error != NULL) {
> +g_task_return_error(task, xfer_op->error);
> +} else {
> +g_task_return_boolean(task, TRUE);
> +}
> +}
> +
If FileTransferOperation owned a GTask* member, we wouldn't have to store these
'cancellable', 'end_callback', and 'end_callback_data' fields separately. I know
that it's done elsewhere in the code, but i
Re: [Spice-devel] [spice-gtk v3 08/16] file-xfer: fix progress info on cancelled transfers
On Mon, 2016-05-30 at 11:55 +0200, Victor Toso wrote:
> Application can start multiple file-transfers in one operation and
> cancel a few of them while the operation is ongoing. In that case, we
> should remove the file-size of the transfer operation otherwise we
> will send incorrect progress data.
>
> Taking in consideration the split of SpiceFileTransferTask, this patch
> includes spice_file_transfer_task_get_file_size() internal helper
> function.
> ---
> src/channel-main.c | 13 ++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/src/channel-main.c b/src/channel-main.c
> index e204a1e..890d939 100644
> --- a/src/channel-main.c
> +++ b/src/channel-main.c
> @@ -61,6 +61,7 @@ typedef void
> (*SpiceFileTransferTaskFlushCb)(SpiceFileTransferTask *xfer_task,
> static guint32 spice_file_transfer_task_get_id(SpiceFileTransferTask *self);
> static SpiceMainChannel
> *spice_file_transfer_task_get_channel(SpiceFileTransferTask *self);
> static GCancellable
> *spice_file_transfer_task_get_cancellable(SpiceFileTransferTask *self);
> +static guint64 spice_file_transfer_task_get_file_size(SpiceFileTransferTask
> *self);
> static void spice_file_transfer_task_flush_done(SpiceFileTransferTask *self,
> GError *error);
> static GList *spice_file_transfer_task_create_tasks(SpiceMainChannel
> *channel,
> GFile **files,
> @@ -1927,6 +1928,7 @@ static void file_xfer_end_callback(GObject
> *source_object,
> {
> GTask *task;
> FileTransferOperation *xfer_op;
> +SpiceFileTransferTask *xfer_task;
>
> task = G_TASK(res);
> if (!g_task_had_error(task))
> @@ -1934,7 +1936,9 @@ static void file_xfer_end_callback(GObject
> *source_object,
> * file_transfer_operation_task_finished */
> return;
>
> +xfer_task = SPICE_FILE_TRANSFER_TASK(source_object);
> xfer_op = user_data;
> +xfer_op->transfer_size -=
> spice_file_transfer_task_get_file_size(xfer_task);
I don't think this is sufficient. The file might get cancelled after it has
transferred some of its data already. In this case, that already-transferred
data will have been added to total_sent, but we will subtract the file size from
transfer_size. Consider the situation where you're transferring two 1MB files.
If you cancel one of these files after sending 75% of both files, the next
progress update will indicate that you've sent 1.5MB of 1MB (or 150%) even
though you still have 25% of the other file to send yet.
Reviewed-by: Jonathon Jongsma
>
> if (xfer_op->error != NULL)
> return;
> @@ -1948,9 +1952,6 @@ static void file_xfer_end_callback(GObject
> *source_object,
> * without GCancellabe */
> if (g_error_matches(xfer_op->error, G_IO_ERROR, G_IO_ERROR_CANCELLED) &&
> xfer_op->cancellable == NULL) {
> -SpiceFileTransferTask *xfer_task;
> -
> -xfer_task = SPICE_FILE_TRANSFER_TASK(source_object);
> spice_debug("file-transfer %u was cancelled",
> spice_file_transfer_task_get_id(xfer_task));
> g_clear_error(&xfer_op->error);
> @@ -3382,6 +3383,12 @@ static GCancellable
> *spice_file_transfer_task_get_cancellable(SpiceFileTransferT
> return self->cancellable;
> }
>
> +static guint64 spice_file_transfer_task_get_file_size(SpiceFileTransferTask
> *self)
> +{
> +g_return_val_if_fail(self != NULL, 0);
> +return self->file_size;
> +}
> +
> static void spice_file_transfer_task_flush_done(SpiceFileTransferTask *self,
> GError *error)
> {
> g_return_if_fail(self != NULL);
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v2] widget: Avoid using GDK_GRAB_FAILED defined in Gtk 3.16.
On 06/03/2016 09:16 AM, Pavel Grunt wrote:
> The returned value from do_pointer_grab() is treated as a boolean - grab
> was successful or not. Change the function to return a boolean value.
>
> Reported-by: Eduardo Lima (Etrunko)
> ---
> src/spice-widget.c | 12 +++-
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/src/spice-widget.c b/src/spice-widget.c
> index d0fa912..0c70ce4 100644
> --- a/src/spice-widget.c
> +++ b/src/spice-widget.c
> @@ -930,12 +930,13 @@ error:
> }
> #endif
>
> -static GdkGrabStatus do_pointer_grab(SpiceDisplay *display)
> +static gboolean do_pointer_grab(SpiceDisplay *display)
> {
> SpiceDisplayPrivate *d = display->priv;
> GdkWindow *window =
> GDK_WINDOW(gtk_widget_get_window(GTK_WIDGET(display)));
> -GdkGrabStatus status = GDK_GRAB_FAILED;
> +GdkGrabStatus status;
> GdkCursor *blank = get_blank_cursor();
> +gboolean grab_successful = FALSE;
>
> if (!gtk_widget_get_realized(GTK_WIDGET(display)))
> goto end;
> @@ -964,7 +965,8 @@ static GdkGrabStatus do_pointer_grab(SpiceDisplay
> *display)
> NULL,
> blank,
> GDK_CURRENT_TIME);
> -if (status != GDK_GRAB_SUCCESS) {
> +grab_successful = (status == GDK_GRAB_SUCCESS);
> +if (!grab_successful) {
> d->mouse_grab_active = false;
> g_warning("pointer grab failed %u", status);
> } else {
> @@ -976,7 +978,7 @@ static GdkGrabStatus do_pointer_grab(SpiceDisplay
> *display)
>
> end:
> g_object_unref(blank);
> -return status;
> +return grab_successful;
> }
>
> static void update_mouse_pointer(SpiceDisplay *display)
> @@ -1023,7 +1025,7 @@ static void try_mouse_grab(SpiceDisplay *display)
> if (d->mouse_grab_active)
> return;
>
> -if (do_pointer_grab(display) != GDK_GRAB_SUCCESS)
> +if (!do_pointer_grab(display))
> return;
>
> d->mouse_last_x = -1;
>
Acked-by: Eduardo Lima (Etrunko)
--
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etru...@redhat.com
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 1/4] record: Support cursor commands
On Fri, 2016-06-03 at 10:59 +0100, Frediano Ziglio wrote:
> Use red_record_cursor_cmd to be able to record cursor commands.
>
> Signed-off-by: Frediano Ziglio
> ---
> server/red-record-qxl.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/server/red-record-qxl.c b/server/red-record-qxl.c
> index 39d6639..9b7d2af 100644
> --- a/server/red-record-qxl.c
> +++ b/server/red-record-qxl.c
> @@ -763,8 +763,8 @@ static void red_record_cursor(FILE *fd, RedMemSlotInfo
> *slots, int group_id,
> &qxl->chunk);
> }
>
> -void red_record_cursor_cmd(FILE *fd, RedMemSlotInfo *slots, int group_id,
> - QXLPHYSICAL addr)
> +static void red_record_cursor_cmd(FILE *fd, RedMemSlotInfo *slots, int
> group_id,
> + QXLPHYSICAL addr)
This change isn't strictly related. On the other hand, it's so small I don't
know if it makes sense to split it or not. Your choice.
> {
> QXLCursorCmd *qxl;
> int error;
> @@ -834,6 +834,9 @@ void red_record_qxl_command(RedRecord *record,
> RedMemSlotInfo *slots,
> case QXL_CMD_SURFACE:
> red_record_surface_cmd(fd, slots, ext_cmd.group_id,
> ext_cmd.cmd.data);
> break;
> +case QXL_CMD_CURSOR:
> +red_record_cursor_cmd(fd, slots, ext_cmd.group_id, ext_cmd.cmd.data);
> +break;
> }
> }
>
Acked-by: Jonathon Jongsma
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 2/4] worker: Record cursor commands
On Fri, 2016-06-03 at 10:59 +0100, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio
> ---
> server/red-worker.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/server/red-worker.c b/server/red-worker.c
> index a14f55d..e754bd2 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -156,6 +156,11 @@ static int red_process_cursor(RedWorker *worker, int
> *ring_is_empty)
> worker->cursor_poll_tries++;
> return n;
> }
> +
> +if (worker->record)
> +red_record_qxl_command(worker->record, &worker->mem_slots,
> ext_cmd,
> + stat_now(CLOCK_MONOTONIC));
> +
consider using spice_get_monotonic_time_ns() instead?
> worker->cursor_poll_tries = 0;
> switch (ext_cmd.cmd.type) {
> case QXL_CMD_CURSOR: {
Acked-by: Jonathon Jongsma
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 3/4] replay: Load cursor commands
On Fri, 2016-06-03 at 10:59 +0100, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio
> ---
> server/red-replay-qxl.c | 70
> +
> 1 file changed, 70 insertions(+)
>
> diff --git a/server/red-replay-qxl.c b/server/red-replay-qxl.c
> index 17019f8..c7d570c 100644
> --- a/server/red-replay-qxl.c
> +++ b/server/red-replay-qxl.c
> @@ -306,6 +306,14 @@ static void red_replay_point_ptr(SpiceReplay *replay,
> QXLPoint *qxl)
> replay_fscanf(replay, "point %d %d\n", &qxl->x, &qxl->y);
> }
>
> +static void red_replay_point16_ptr(SpiceReplay *replay, QXLPoint16 *qxl)
> +{
> +int x, y;
> +replay_fscanf(replay, "point16 %d %d\n", &x, &y);
> +qxl->x = x;
> +qxl->y = y;
> +}
> +
this is true of many existing functions as well, but we're not handling any
failures from scanf here...
> static void red_replay_rect_ptr(SpiceReplay *replay, const char *prefix,
> QXLRect *qxl)
> {
> char template[1024];
> @@ -1052,6 +1060,59 @@ static void red_replay_surface_cmd_free(SpiceReplay
> *replay, QXLSurfaceCmd *qxl)
> free(qxl);
> }
>
> +static QXLCursor *red_replay_cursor(SpiceReplay *replay)
> +{
> +int temp;
> +QXLCursor cursor, *qxl = NULL;
> +
> +replay_fscanf(replay, "header.unique %"SCNu64"\n",
> &cursor.header.unique);
> +replay_fscanf(replay, "header.type %d\n", &temp); cursor.header.type =
> temp;
> +replay_fscanf(replay, "header.width %d\n", &temp); cursor.header.width =
> temp;
> +replay_fscanf(replay, "header.height %d\n", &temp); cursor.header.height
> = temp;
> +replay_fscanf(replay, "header.hot_spot_x %d\n", &temp);
> cursor.header.hot_spot_x = temp;
> +replay_fscanf(replay, "header.hot_spot_y %d\n", &temp);
> cursor.header.hot_spot_y = temp;
> +
> +replay_fscanf(replay, "data_size %d\n", &temp); cursor.data_size = temp;
I prefer to avoid multiple statements on a single line.
> +cursor.data_size = red_replay_data_chunks(replay, "cursor",
> (uint8_t**)&qxl, sizeof(QXLCursor));
> +qxl->header = cursor.header;
> +qxl->data_size = cursor.data_size;
> +return qxl;
> +}
> +
> +static QXLCursorCmd *red_replay_cursor_cmd(SpiceReplay *replay)
> +{
> +int temp;
> +QXLCursorCmd *qxl = spice_new0(QXLCursorCmd, 1);
> +
> +replay_fscanf(replay, "cursor_cmd\n");
> +replay_fscanf(replay, "type %d\n", &temp); qxl->type = temp;
More multi-statement lines in this function.
> +switch (qxl->type) {
> +case QXL_CURSOR_SET:
> +red_replay_point16_ptr(replay, &qxl->u.set.position);
> +replay_fscanf(replay, "u.set.visible %d\n", &temp); qxl-
> >u.set.visible = temp;
> +qxl->u.set.shape = QXLPHYSICAL_FROM_PTR(red_replay_cursor(replay));
> +break;
> +case QXL_CURSOR_MOVE:
> +red_replay_point16_ptr(replay, &qxl->u.position);
> +break;
> +case QXL_CURSOR_TRAIL:
> +replay_fscanf(replay, "u.trail.length %d\n", &temp); qxl-
> >u.trail.length = temp;
> +replay_fscanf(replay, "u.trail.frequency %d\n", &temp); qxl-
> >u.trail.frequency = temp;
> +break;
> +}
> +return qxl;
> +}
> +
> +static void red_replay_cursor_cmd_free(SpiceReplay *replay, QXLCursorCmd
> *qxl)
> +{
> +if (qxl->type == QXL_CURSOR_SET) {
> +QXLCursor *cursor = QXLPHYSICAL_TO_PTR(qxl->u.set.shape);
> +red_replay_data_chunks_free(replay, cursor, sizeof(*cursor));
> +}
> +
> +free(qxl);
> +}
> +
> static void replay_handle_create_primary(QXLWorker *worker, SpiceReplay
> *replay)
> {
> QXLDevSurfaceCreate surface = { 0, };
> @@ -1148,6 +1209,9 @@ SPICE_GNUC_VISIBLE QXLCommandExt*
> spice_replay_next_cmd(SpiceReplay *replay,
> case QXL_CMD_SURFACE:
> cmd->cmd.data = QXLPHYSICAL_FROM_PTR(red_replay_surface_cmd(replay));
> break;
> +case QXL_CMD_CURSOR:
> +cmd->cmd.data = QXLPHYSICAL_FROM_PTR(red_replay_cursor_cmd(replay));
> +break;
> }
>
> QXLReleaseInfo *info;
> @@ -1155,6 +1219,7 @@ SPICE_GNUC_VISIBLE QXLCommandExt*
> spice_replay_next_cmd(SpiceReplay *replay,
> case QXL_CMD_DRAW:
> case QXL_CMD_UPDATE:
> case QXL_CMD_SURFACE:
> +case QXL_CMD_CURSOR:
> info = QXLPHYSICAL_TO_PTR(cmd->cmd.data);
> info->id = (uintptr_t)cmd;
> }
> @@ -1187,6 +1252,11 @@ SPICE_GNUC_VISIBLE void
> spice_replay_free_cmd(SpiceReplay *replay, QXLCommandExt
> red_replay_surface_cmd_free(replay, qxl);
> break;
> }
> +case QXL_CMD_CURSOR: {
> +QXLCursorCmd *qxl = QXLPHYSICAL_TO_PTR(cmd->cmd.data);
> +red_replay_cursor_cmd_free(replay, qxl);
> +break;
> +}
> default:
> break;
> }
Acked-by: Jonathon Jongsma
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH 4/4] replay: Handle cursor commands
On Fri, 2016-06-03 at 10:59 +0100, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio
> ---
> server/tests/replay.c | 50 --
> 1 file changed, 32 insertions(+), 18 deletions(-)
>
> diff --git a/server/tests/replay.c b/server/tests/replay.c
> index d552327..528609b 100644
> --- a/server/tests/replay.c
> +++ b/server/tests/replay.c
> @@ -52,7 +52,8 @@ static gboolean print_count = FALSE;
> static guint ncommands = 0;
> static pid_t client_pid;
> static GMainLoop *loop = NULL;
> -static GAsyncQueue *aqueue = NULL;
> +static GAsyncQueue *cmd_queue = NULL;
> +static GAsyncQueue *cursor_queue = NULL;
> static long total_size;
>
> static pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
> @@ -113,10 +114,12 @@ static gboolean fill_queue_idle(gpointer user_data)
> gboolean keep = FALSE;
> gboolean wakeup = FALSE;
>
> -while (g_async_queue_length(aqueue) < 50) {
> +while ((g_async_queue_length(cmd_queue) +
> +g_async_queue_length(cursor_queue)) < 50) {
> QXLCommandExt *cmd = spice_replay_next_cmd(replay, qxl_worker);
> if (!cmd) {
> -g_async_queue_push(aqueue, GINT_TO_POINTER(-1));
> +g_async_queue_push(cmd_queue, GINT_TO_POINTER(-1));
> +g_async_queue_push(cursor_queue, GINT_TO_POINTER(-1));
> goto end;
> }
>
> @@ -127,7 +130,11 @@ static gboolean fill_queue_idle(gpointer user_data)
> }
>
> wakeup = TRUE;
> -g_async_queue_push(aqueue, cmd);
> +if (cmd->cmd.type == QXL_CMD_CURSOR) {
> +g_async_queue_push(cursor_queue, cmd);
> +} else {
> +g_async_queue_push(cmd_queue, cmd);
> +}
> }
>
> end:
> @@ -166,17 +173,21 @@ end:
>
>
> // called from spice_server thread (i.e. red_worker thread)
> -static int get_command(QXLInstance *qin, QXLCommandExt *ext)
> +static int get_command_from(QXLInstance *qin, QXLCommandExt *ext, GAsyncQueue
> *queue)
> {
> QXLCommandExt *cmd;
>
> -if (g_async_queue_length(aqueue) == 0) {
> +if (g_async_queue_length(cmd_queue) == 0 &&
> +g_async_queue_length(cursor_queue) == 0) {
why not just check "g_async_queue_length(queue) == 0"? if we're trying to get a
command from the cursor_queue and it's empty, should the fact that the other
queue is not empty prevent us from trying to fill the queue? It's probably fine
this way, just curious.
> /* could use a gcondition ? */
> fill_queue();
> return FALSE;
> }
>
> -cmd = g_async_queue_try_pop(aqueue);
> +cmd = g_async_queue_try_pop(queue);
> +if (cmd == NULL) {
> +return FALSE;
> +}
> if (GPOINTER_TO_INT(cmd) == -1) {
> g_main_loop_quit(loop);
> return FALSE;
> @@ -187,8 +198,14 @@ static int get_command(QXLInstance *qin, QXLCommandExt
> *ext)
> return TRUE;
> }
>
> -static int req_cmd_notification(QXLInstance *qin)
> +static int get_command(QXLInstance *qin, QXLCommandExt *ext)
> +{
> +return get_command_from(qin, ext, cmd_queue);
> +}
> +
> +static int req_notification(QXLInstance *qin)
> {
> +/* we don't have currently message pending */
> return TRUE;
> }
>
> @@ -214,12 +231,7 @@ static void release_resource(QXLInstance *qin, struct
> QXLReleaseInfoExt release_
>
> static int get_cursor_command(QXLInstance *qin, struct QXLCommandExt *ext)
> {
> -return FALSE;
> -}
> -
> -static int req_cursor_notification(QXLInstance *qin)
> -{
> -return TRUE;
> +return get_command_from(qin, ext, cursor_queue);
> }
>
> static void notify_update(QXLInstance *qin, uint32_t update_id)
> @@ -243,10 +255,10 @@ static QXLInterface display_sif = {
> .set_mm_time = set_mm_time,
> .get_init_info = get_init_info,
> .get_command = get_command,
> -.req_cmd_notification = req_cmd_notification,
> +.req_cmd_notification = req_notification,
> .release_resource = release_resource,
> .get_cursor_command = get_cursor_command,
> -.req_cursor_notification = req_cursor_notification,
> +.req_cursor_notification = req_notification,
> .notify_update = notify_update,
> .flush_resources = flush_resources,
> };
> @@ -379,7 +391,8 @@ int main(int argc, char **argv)
> exit(1);
> }
>
> -aqueue = g_async_queue_new();
> +cmd_queue = g_async_queue_new();
> +cursor_queue = g_async_queue_new();
> core = basic_event_loop_init();
> core->channel_event = replay_channel_event;
>
> @@ -414,7 +427,8 @@ int main(int argc, char **argv)
> g_print("Counted %d commands\n", ncommands);
>
> end_replay();
> -g_async_queue_unref(aqueue);
> +g_async_queue_unref(cmd_queue);
> +g_async_queue_unref(cursor_queue);
>
> /* FIXME: there should be a way to join server threads before:
> * g_main_loop_unref(loop);
aside from the minor question above, looks fine.
Acked-by: Jonathon Jongsm
