Re: [Spice-devel] allow config download with https
On Thu, Jul 25, 2013 at 06:56:20AM -0400, Marc-André Lureau wrote: > > Sure. Then I have to maintain/install a script and remote-viewer, and I want > > to avoid that. > > > > Why is the ovirt code included? I guess it is just convenient. > > I haven't used the ovirt support, but my impression was that it was > performing authentication + lookup using some rest api. Yes, libgovirt is using the REST API to go from VM name to VM guid, and then to get the connection information for that VM, and to generate a ticket to connect to it. > > I do not really understand that question? > > Why do you need to fetch the vv file directly from the HTTPS server? Why > not opening it from a browser, like "firefox url" will open up a dialog > (or not, if you set your preferences) and run virt-viewer. In my opinion, we could support running 'remote-viewer https://proxmox.example.com/myvm.vv', I guess gio/gvfs should be able to fetch files from http[s] (no libcurl please ;). Is it what you (Dietmar) are looking for? Christophe pgpd6rWwz1oh8.pgp Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] win-xp: uninstalling spice-guest-tools
Hey, On Thu, Jul 18, 2013 at 12:15:10PM +0200, Peter Münster wrote: > I've installed spice-guest-tools > (http://spice-space.org/download/windows/spice-guest-tools/spice-guest-tools-0.59.exe) > on my Win-XP guest (qemu running on GNU/Linux). But unfortunately USB is > no more working: whenever I start qemu with option "-usb", Windows > hangs. And this was working before the installation of the guest drivers? I'm surprised these interact with -usb. > How could I revert the installation of the spice-guest-tools please? I'd check in the device manager if drivers for the various virtio* devices are still there, and remove them by hand. > Any help would be very welcome, USB is important to me. > Please tell me, if you need further information. Have you tried using spice USB redirection? I think this works without using -usb. Christophe pgpIi733AFBN6.pgp Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] allow config download with https
> In my opinion, we could support running 'remote-viewer > https://proxmox.example.com/myvm.vv', Well, but we need to handle basic auth. >I guess gio/gvfs should be able to > fetch files from http[s] (no libcurl please ;). Is it what you (Dietmar) are > looking for? Why gvfs? libcurl look much simpler. ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] allow config download with https
On Fri, Jul 26, 2013 at 11:26:24AM +, Dietmar Maurer wrote: > > In my opinion, we could support running 'remote-viewer > > https://proxmox.example.com/myvm.vv', > > Well, but we need to handle basic auth. > > >I guess gio/gvfs should be able to > > fetch files from http[s] (no libcurl please ;). Is it what you (Dietmar) are > > looking for? > > Why gvfs? libcurl look much simpler. I'm not sure I agree with that. Using GVFS/GIO gives us code that is completely protocol agnostic, it can be used with plain local files, just as easily as with arbitrary URIs (whether HTPT, FTP, SSH, or any other GVFS scheme). I'd not want to see libcurl used unless there is some blocking technical limitation of GVFS that can't be resolved. 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 http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] allow config download with https
On Fri, Jul 26, 2013 at 11:26:24AM +, Dietmar Maurer wrote: > > In my opinion, we could support running 'remote-viewer > > https://proxmox.example.com/myvm.vv', > > Well, but we need to handle basic auth. Couldn't an auth dialog be popped up when needed? > > >I guess gio/gvfs should be able to > > fetch files from http[s] (no libcurl please ;). Is it what you (Dietmar) are > > looking for? > > Why gvfs? libcurl look much simpler. libcurl would look a bit alien in the rest of the code base, and if we ever want to do the download asynchronously, we won't get integration with the glib mainloop for free, ... Alternatively, I'd suggest using libsoup as Marc-André proposed (I'm not sure how good gio/gvfs https support is ;) Christophe pgpvcg93khUYD.pgp Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [Spice-commits] 9 commits - spice-protocol vdagent/desktop_layout.cpp vdagent/display_setting.cpp vdagent/file_xfer.cpp vdagent/vdagent.cpp
Hey,
These patches do not seem to have been sent to spice-devel@ before commit?
Christophe
On Wed, Jul 17, 2013 at 11:53:25AM -0700, Marc-André Lureau wrote:
> spice-protocol |2 -
> vdagent/desktop_layout.cpp |4 +--
> vdagent/display_setting.cpp |2 -
> vdagent/file_xfer.cpp | 58
> +---
> vdagent/vdagent.cpp | 42 +++
> 5 files changed, 58 insertions(+), 50 deletions(-)
>
> New commits:
> commit ad61eec9611ede0b74c577a3426670eb13bde7d0
> Author: Marc-Andr?? Lureau
> Date: Wed Jul 17 20:52:17 2013 +0200
>
> Fix cast BOOL->PVOID warning
>
> vdagent/display_setting.cpp:469:50: warning: cast to pointer from
> integer of different size [-Wint-to-pointer-cast]
> if (!SystemParametersInfo(action, 0, (PVOID)param, 0)) {
>
> diff --git a/vdagent/display_setting.cpp b/vdagent/display_setting.cpp
> index fdf5a31..1ec7397 100644
> --- a/vdagent/display_setting.cpp
> +++ b/vdagent/display_setting.cpp
> @@ -466,7 +466,7 @@ bool DisplaySetting::reload_win_animation(HKEY
> desktop_reg_key)
>
> bool DisplaySetting::set_bool_system_parameter_info(int action, BOOL param)
> {
> -if (!SystemParametersInfo(action, 0, (PVOID)param, 0)) {
> +if (!SystemParametersInfo(action, 0, (PVOID)(uintptr_t)param, 0)) {
> vd_printf("SystemParametersInfo %d: failed %lu", action,
> GetLastError());
> return false;
> }
> commit fc1de85b499759576fa52dfd20496bfb887c8295
> Author: Marc-Andr?? Lureau
> Date: Wed Jul 17 20:49:01 2013 +0200
>
> Fix wrong size_t printf format
>
> vdagent/desktop_layout.cpp:121:763: warning: format '%u' expects
> argument of type 'unsigned int', but argument 8 has type
> 'std::vector::size_type {aka long long unsigned int}'
> [-Wformat=]
>
> diff --git a/vdagent/desktop_layout.cpp b/vdagent/desktop_layout.cpp
> index ce259e1..7850e6d 100644
> --- a/vdagent/desktop_layout.cpp
> +++ b/vdagent/desktop_layout.cpp
> @@ -118,7 +118,7 @@ void DesktopLayout::set_displays()
> break;
> }
> if (display_id >= _displays.size()) {
> -vd_printf("display_id %lu out of range, #displays %u",
> display_id, _displays.size());
> +vd_printf("display_id %lu out of range, #displays %zu" ,
> display_id, _displays.size());
> break;
> }
> if (!init_dev_mode(dev_info.DeviceName, &dev_mode,
> _displays.at(display_id), true)) {
> commit 492ee05a6bd92acc94557a129cd609d66785dac2
> Author: Marc-Andr?? Lureau
> Date: Wed Jul 17 20:43:41 2013 +0200
>
> Replace sscanf_s by sscanf
>
> The _s functions need a recent msvcrt version, not shipped in XP by
> default.
>
> Furthermore, it appears that their sscanf_s usage was missing the extra
> buffer size argument.
>
> diff --git a/vdagent/file_xfer.cpp b/vdagent/file_xfer.cpp
> index 9d2f3c5..84f6043 100644
> --- a/vdagent/file_xfer.cpp
> +++ b/vdagent/file_xfer.cpp
> @@ -182,7 +182,7 @@ bool FileXfer::g_key_get_string(char* data, const char*
> group, const char* key,
> next_group_pos = strstr(group_pos + strlen(group_pfx), "[");
> if (next_group_pos && key_pos > next_group_pos) return false;
>
> -return !!sscanf_s(key_pos + strlen(key_pfx), "%s\n", value);
> +return !!sscanf(key_pos + strlen(key_pfx), "%s\n", value);
> }
>
> bool FileXfer::g_key_get_uint64(char* data, const char* group, const char*
> key, uint64_t* value)
> @@ -190,5 +190,5 @@ bool FileXfer::g_key_get_uint64(char* data, const char*
> group, const char* key,
> char str[G_KEY_MAX_LEN];
>
> if (!g_key_get_string(data, group, key, str)) return false;
> -return !!sscanf_s(str, "%llu", value);
> +return !!sscanf(str, "%" PRIu64, value);
> }
> commit 5129f33899805b55915e506d392c89421db538a2
> Author: Marc-Andr?? Lureau
> Date: Wed Jul 17 20:43:14 2013 +0200
>
> Add suggested parentheses
>
> vdagent/vdagent.cpp: In member function 'void
> VDAgent::on_clipboard_grab()':
> vdagent/vdagent.cpp:951:52: warning: suggest parentheses around
> assignment used as truth value [-Wparentheses]
>
> diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
> index 73ca39c..260ee40 100644
> --- a/vdagent/vdagent.cpp
> +++ b/vdagent/vdagent.cpp
> @@ -948,7 +948,7 @@ void VDAgent::on_clipboard_grab()
> set_clipboard_owner(owner_guest);
> } else {
> UINT format = 0;
> -while (format = EnumClipboardFormats(format)) {
> +while ((format = EnumClipboardFormats(format))) {
> vd_printf("Unsupported clipboard format %u", format);
> }
> }
> commit b9d6baec2d346e37798dd1892f6ae95ac9d6c9ef
> Author: Marc-Andr?? Lureau
> Date: Wed Jul 17 20:42:31 2013 +0200
>
> Fix task could be used uninitialized
>
> diff --git a/vdagent/file_xfer.cpp b/vdagent/file_xfer.cpp
> index 6ba15a3..9d2f3c5 100644
Re: [Spice-devel] [Spice-commits] 9 commits - spice-protocol vdagent/desktop_layout.cpp vdagent/display_setting.cpp vdagent/file_xfer.cpp vdagent/vdagent.cpp
- Mensaje original - > Hey, > > These patches do not seem to have been sent to spice-devel@ before commit? They are build fixes, or trivial runtime fixes (unlike the feature implementation that was committed before without review) Feel free to review/fix the commits now. ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [Spice-commits] 9 commits - spice-protocol vdagent/desktop_layout.cpp vdagent/display_setting.cpp vdagent/file_xfer.cpp vdagent/vdagent.cpp
- Mensaje original - > > > - Mensaje original - > > Hey, > > > > These patches do not seem to have been sent to spice-devel@ before commit? > > > They are build fixes, or trivial runtime fixes (unlike the feature > implementation that was committed before without review) My bad, it was reviewed by Hans. I wished I would have seen that before. ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [Spice-commits] 9 commits - spice-protocol vdagent/desktop_layout.cpp vdagent/display_setting.cpp vdagent/file_xfer.cpp vdagent/vdagent.cpp
On Fri, Jul 26, 2013 at 08:38:36AM -0400, Marc-André Lureau wrote: > > > - Mensaje original - > > Hey, > > > > These patches do not seem to have been sent to spice-devel@ before commit? > > > They are build fixes, or trivial runtime fixes Do we have a 'trivial rule' policy for commits to spice? Even if we do, it would be appreciated if they could be sent to spice-devel with a note that they were pushed under the trivial rule (I don't find 9 patches concatenated in a single email with no summary easy to review). Christophe pgpwaXL7LorCh.pgp Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [Spice-commits] 9 commits - spice-protocol vdagent/desktop_layout.cpp vdagent/display_setting.cpp vdagent/file_xfer.cpp vdagent/vdagent.cpp
On Wed, Jul 17, 2013 at 11:53:25AM -0700, Marc-André Lureau wrote:
> commit fc1de85b499759576fa52dfd20496bfb887c8295
> Author: Marc-Andr?? Lureau
> Date: Wed Jul 17 20:49:01 2013 +0200
>
> Fix wrong size_t printf format
>
> vdagent/desktop_layout.cpp:121:763: warning: format '%u' expects
> argument of type 'unsigned int', but argument 8 has type
> 'std::vector::size_type {aka long long unsigned int}'
> [-Wformat=]
>
> diff --git a/vdagent/desktop_layout.cpp b/vdagent/desktop_layout.cpp
> index ce259e1..7850e6d 100644
> --- a/vdagent/desktop_layout.cpp
> +++ b/vdagent/desktop_layout.cpp
> @@ -118,7 +118,7 @@ void DesktopLayout::set_displays()
> break;
> }
> if (display_id >= _displays.size()) {
> -vd_printf("display_id %lu out of range, #displays %u",
> display_id, _displays.size());
> +vd_printf("display_id %lu out of range, #displays %zu" ,
> display_id, _displays.size());
If we still support msvc builds, I don't think %zu will work there.
Christophe
pgpojrlKGtRLg.pgp
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] allow config download with https
> > >I guess gio/gvfs should be able to > > > fetch files from http[s] (no libcurl please ;). Is it what you > > >(Dietmar) are looking for? > > > > Why gvfs? libcurl look much simpler. > > libcurl would look a bit alien in the rest of the code base, and if we ever > want to do the download asynchronously, we won't get integration with the > glib mainloop for free, ... Oh, I thought the libcurl async code is quite easy to use with any event loop (but I never tried that myself). ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-server 1/8] red_channel: prevent adding and pushing pipe items after a channel_client has diconnected
Fixes: leaks of pipe items & "red_client_destroy: assertion
`rcc->send_data.size == 0'"
red_channel_disconnect clears the pipe. It is called only once. After,
it was called, not items should be added to the pipe.
An example of when this assert can occur:
on_new_cursor_channel (red_worker.c), pushes 2 pipe items.
When it pushes the first pipe item, if the client has disconnected,
it can hit a socket error, and then, red_channel_client_disconnect is called.
The second call to adding a pipe item, will add the item to
the pipe. red_channel_client_pipe_add_type also calls
red_channel_client_push, which will update the send_data.size.
Then, the push will also hit a socket error, but red_channel_client_disconnect
won't clear the pending pipe item again, since it was already called.
When red_client_destory is called, we hit assertion `rcc->send_data.size
== 0'.
Note that if a pipe item is added to the pipe after
red_channel_client_disconnect was called, but without pushing it,
we should hit "spice_assert(rcc->pipe_size == 0)".
---
server/red_channel.c | 30 --
1 file changed, 24 insertions(+), 6 deletions(-)
diff --git a/server/red_channel.c b/server/red_channel.c
index 31c991b..a35da9b 100644
--- a/server/red_channel.c
+++ b/server/red_channel.c
@@ -1524,9 +1524,23 @@ void red_channel_pipe_item_init(RedChannel *channel,
PipeItem *item, int type)
item->type = type;
}
-void red_channel_client_pipe_add(RedChannelClient *rcc, PipeItem *item)
+static inline int validate_pipe_add(RedChannelClient *rcc, PipeItem *item)
{
spice_assert(rcc && item);
+if (SPICE_UNLIKELY(!red_channel_client_is_connected(rcc))) {
+spice_debug("rcc is disconnected %p", rcc);
+red_channel_client_release_item(rcc, item, FALSE);
+return FALSE;
+}
+return TRUE;
+}
+
+void red_channel_client_pipe_add(RedChannelClient *rcc, PipeItem *item)
+{
+
+if (!validate_pipe_add(rcc, item)) {
+return;
+}
rcc->pipe_size++;
ring_add(&rcc->pipe, &item->link);
}
@@ -1540,10 +1554,10 @@ void red_channel_client_pipe_add_push(RedChannelClient
*rcc, PipeItem *item)
void red_channel_client_pipe_add_after(RedChannelClient *rcc,
PipeItem *item, PipeItem *pos)
{
-spice_assert(rcc);
spice_assert(pos);
-spice_assert(item);
-
+if (!validate_pipe_add(rcc, item)) {
+return;
+}
rcc->pipe_size++;
ring_add_after(&item->link, &pos->link);
}
@@ -1557,14 +1571,18 @@ int
red_channel_client_pipe_item_is_linked(RedChannelClient *rcc,
void red_channel_client_pipe_add_tail_no_push(RedChannelClient *rcc,
PipeItem *item)
{
-spice_assert(rcc);
+if (!validate_pipe_add(rcc, item)) {
+return;
+}
rcc->pipe_size++;
ring_add_before(&item->link, &rcc->pipe);
}
void red_channel_client_pipe_add_tail(RedChannelClient *rcc, PipeItem *item)
{
-spice_assert(rcc);
+if (!validate_pipe_add(rcc, item)) {
+return;
+}
rcc->pipe_size++;
ring_add_before(&item->link, &rcc->pipe);
red_channel_client_push(rcc);
--
1.8.1.4
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-server 2/8] red_channel: add ref count to RedClient
---
server/red_channel.c | 23 ---
server/red_channel.h | 17 +++--
2 files changed, 35 insertions(+), 5 deletions(-)
diff --git a/server/red_channel.c b/server/red_channel.c
index a35da9b..9b5e314 100644
--- a/server/red_channel.c
+++ b/server/red_channel.c
@@ -1929,10 +1929,29 @@ RedClient *red_client_new(int migrated)
pthread_mutex_init(&client->lock, NULL);
client->thread_id = pthread_self();
client->during_target_migrate = migrated;
+client->refs = 1;
return client;
}
+RedClient *red_client_ref(RedClient *client)
+{
+spice_assert(client);
+client->refs++;
+return client;
+}
+
+RedClient *red_client_unref(RedClient *client)
+{
+if (!--client->refs) {
+spice_debug("release client=%p", client);
+pthread_mutex_destroy(&client->lock);
+free(client);
+return NULL;
+}
+return client;
+}
+
/* client mutex should be locked before this call */
static void red_channel_client_set_migration_seamless(RedChannelClient *rcc)
{
@@ -2009,9 +2028,7 @@ void red_client_destroy(RedClient *client)
spice_assert(rcc->send_data.size == 0);
red_channel_client_destroy(rcc);
}
-
-pthread_mutex_destroy(&client->lock);
-free(client);
+red_client_unref(client);
}
/* client->lock should be locked */
diff --git a/server/red_channel.h b/server/red_channel.h
index ba299b6..0dd73ea 100644
--- a/server/red_channel.h
+++ b/server/red_channel.h
@@ -561,10 +561,25 @@ struct RedClient {
is called */
int seamless_migrate;
int num_migrated_channels; /* for seamless - number of channels that wait
for migrate data*/
+int refs;
};
RedClient *red_client_new(int migrated);
+/*
+ * disconnects all the client's channels (should be called from the client's
thread)
+ */
+void red_client_destroy(RedClient *client);
+
+RedClient *red_client_ref(RedClient *client);
+
+/*
+ * releases the client resources when refs == 0.
+ * We assume the red_client_derstroy was called before
+ * we reached refs==0
+ */
+RedClient *red_client_unref(RedClient *client);
+
MainChannelClient *red_client_get_main(RedClient *client);
// main should be set once before all the other channels are created
void red_client_set_main(RedClient *client, MainChannelClient *mcc);
@@ -580,7 +595,5 @@ void red_client_semi_seamless_migrate_complete(RedClient
*client); /* dst side *
int red_client_during_migrate_at_target(RedClient *client);
void red_client_migrate(RedClient *client);
-// disconnects all the client's channels (should be called from the client's
thread)
-void red_client_destroy(RedClient *client);
#endif
--
1.8.1.4
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-server 3/8] main_dispatcher: add ref count protection to RedClient instances
---
server/main_dispatcher.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/server/main_dispatcher.c b/server/main_dispatcher.c
index e7a451a..bf160dd 100644
--- a/server/main_dispatcher.c
+++ b/server/main_dispatcher.c
@@ -97,6 +97,7 @@ static void main_dispatcher_handle_migrate_complete(void
*opaque,
MainDispatcherMigrateSeamlessDstCompleteMessage *mig_complete = payload;
reds_on_client_seamless_migrate_complete(mig_complete->client);
+red_client_unref(mig_complete->client);
}
static void main_dispatcher_handle_mm_time_latency(void *opaque,
@@ -104,6 +105,7 @@ static void main_dispatcher_handle_mm_time_latency(void
*opaque,
{
MainDispatcherMmTimeLatencyMessage *msg = payload;
reds_set_client_mm_time_latency(msg->client, msg->latency);
+red_client_unref(msg->client);
}
void main_dispatcher_seamless_migrate_dst_complete(RedClient *client)
@@ -115,7 +117,7 @@ void
main_dispatcher_seamless_migrate_dst_complete(RedClient *client)
return;
}
-msg.client = client;
+msg.client = red_client_ref(client);
dispatcher_send_message(&main_dispatcher.base,
MAIN_DISPATCHER_MIGRATE_SEAMLESS_DST_COMPLETE,
&msg);
}
@@ -129,7 +131,7 @@ void main_dispatcher_set_mm_time_latency(RedClient *client,
uint32_t latency)
return;
}
-msg.client = client;
+msg.client = red_client_ref(client);
msg.latency = latency;
dispatcher_send_message(&main_dispatcher.base,
MAIN_DISPATCHER_SET_MM_TIME_LATENCY,
&msg);
--
1.8.1.4
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-server 6/8] snd_worker: fix memory leak of PlaybackChannel
When the sequence of calls bellow occurs, the PlaybackChannel
is not released (snd_channel_put is not called for the
samples that refer to the channel).
spice_server_playback_get_buffer
snd_channel_disconnect
spice_server_playback_put_samples
---
server/snd_worker.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/server/snd_worker.c b/server/snd_worker.c
index d6ec47a..849f002 100644
--- a/server/snd_worker.c
+++ b/server/snd_worker.c
@@ -1097,14 +1097,13 @@ SPICE_GNUC_VISIBLE void
spice_server_playback_put_samples(SpicePlaybackInstance
PlaybackChannel *playback_channel;
AudioFrame *frame;
-if (!sin->st->worker.connection) {
-return;
-}
-
frame = SPICE_CONTAINEROF(samples, AudioFrame, samples);
playback_channel = frame->channel;
-if (!snd_channel_put(&playback_channel->base) ||
!playback_channel->base.worker->connection) {
+spice_assert(playback_channel);
+if (!snd_channel_put(&playback_channel->base) ||
+sin->st->worker.connection != &playback_channel->base) {
/* lost last reference, channel has been destroyed previously */
+spice_info("audio samples belong to a disconnected channel");
return;
}
spice_assert(playback_channel->base.active);
--
1.8.1.4
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-server 8/8] log: improve debug information related to client disconnection
---
server/red_channel.c | 9 ++---
server/snd_worker.c | 7 ---
2 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/server/red_channel.c b/server/red_channel.c
index 9b5e314..89b4092 100644
--- a/server/red_channel.c
+++ b/server/red_channel.c
@@ -1109,6 +1109,7 @@ static void red_channel_client_ref(RedChannelClient *rcc)
static void red_channel_client_unref(RedChannelClient *rcc)
{
if (!--rcc->refs) {
+spice_debug("destroy rcc=%p", rcc);
if (rcc->send_data.main.marshaller) {
spice_marshaller_destroy(rcc->send_data.main.marshaller);
}
@@ -1705,6 +1706,8 @@ static void
red_channel_client_disconnect_dummy(RedChannelClient *rcc)
{
spice_assert(rcc->dummy);
if (ring_item_is_linked(&rcc->channel_link)) {
+spice_printerr("rcc=%p (channel=%p type=%d id=%d)", rcc, rcc->channel,
+ rcc->channel->type, rcc->channel->id);
red_channel_remove_client(rcc);
}
rcc->dummy_connected = FALSE;
@@ -1712,8 +1715,6 @@ static void
red_channel_client_disconnect_dummy(RedChannelClient *rcc)
void red_channel_client_disconnect(RedChannelClient *rcc)
{
-spice_printerr("%p (channel %p type %d id %d)", rcc, rcc->channel,
-rcc->channel->type,
rcc->channel->id);
if (rcc->dummy) {
red_channel_client_disconnect_dummy(rcc);
return;
@@ -1721,6 +1722,8 @@ void red_channel_client_disconnect(RedChannelClient *rcc)
if (!red_channel_client_is_connected(rcc)) {
return;
}
+spice_printerr("rcc=%p (channel=%p type=%d id=%d)", rcc, rcc->channel,
+ rcc->channel->type, rcc->channel->id);
red_channel_client_pipe_clear(rcc);
if (rcc->stream->watch) {
rcc->channel->core->watch_remove(rcc->stream->watch);
@@ -2004,7 +2007,7 @@ void red_client_destroy(RedClient *client)
RingItem *link, *next;
RedChannelClient *rcc;
-spice_printerr("destroy client with #channels %d", client->channels_num);
+spice_printerr("destroy client %p with #channels=%d", client,
client->channels_num);
if (!pthread_equal(pthread_self(), client->thread_id)) {
spice_warning("client->thread_id (0x%lx) != pthread_self (0x%lx)."
"If one of the threads is != io-thread && !=
vcpu-thread,"
diff --git a/server/snd_worker.c b/server/snd_worker.c
index 3827416..ebddfcd 100644
--- a/server/snd_worker.c
+++ b/server/snd_worker.c
@@ -201,8 +201,8 @@ static SndChannel *snd_channel_get(SndChannel *channel)
static SndChannel *snd_channel_put(SndChannel *channel)
{
if (!--channel->refs) {
+spice_printerr("SndChannel=%p freed", channel);
free(channel);
-spice_printerr("sound channel freed");
return NULL;
}
return channel;
@@ -216,7 +216,8 @@ static void snd_disconnect_channel(SndChannel *channel)
spice_debug("not connected");
return;
}
-spice_debug("%p", channel);
+spice_debug("SndChannel=%p rcc=%p type=%d",
+ channel, channel->channel_client,
channel->channel_client->channel->type);
worker = channel->worker;
channel->cleanup(channel);
red_channel_client_disconnect(worker->connection->channel_client);
@@ -976,11 +977,11 @@ static void
snd_disconnect_channel_client(RedChannelClient *rcc)
{
SndWorker *worker;
-spice_debug(NULL);
spice_assert(rcc->channel);
spice_assert(rcc->channel->data);
worker = (SndWorker *)rcc->channel->data;
+spice_debug("channel-type=%d", rcc->channel->type);
if (worker->connection) {
spice_assert(worker->connection->channel_client == rcc);
snd_disconnect_channel(worker->connection);
--
1.8.1.4
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-server 7/8] snd_worker/snd_disconnect_channel: don't call snd_channel_put if the channel has already been disconnected
The snd channels has one reference as long as their socket is active.
The playback channel has an additional reference for each frame that is
currently filled by the sound device.
Once the channel is disconnected (the socket has been freed and the
first reference is released) snd_disconnect_channel shouldn't release
a reference again.
---
server/snd_worker.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/server/snd_worker.c b/server/snd_worker.c
index 849f002..3827416 100644
--- a/server/snd_worker.c
+++ b/server/snd_worker.c
@@ -212,21 +212,20 @@ static void snd_disconnect_channel(SndChannel *channel)
{
SndWorker *worker;
-if (!channel) {
+if (!channel || !channel->stream) {
spice_debug("not connected");
return;
}
spice_debug("%p", channel);
worker = channel->worker;
-if (channel->stream) {
-channel->cleanup(channel);
-red_channel_client_disconnect(worker->connection->channel_client);
-core->watch_remove(channel->stream->watch);
-channel->stream->watch = NULL;
-reds_stream_free(channel->stream);
-channel->stream = NULL;
-spice_marshaller_destroy(channel->send_data.marshaller);
-}
+channel->cleanup(channel);
+red_channel_client_disconnect(worker->connection->channel_client);
+worker->connection->channel_client = NULL;
+core->watch_remove(channel->stream->watch);
+channel->stream->watch = NULL;
+reds_stream_free(channel->stream);
+channel->stream = NULL;
+spice_marshaller_destroy(channel->send_data.marshaller);
snd_channel_put(channel);
worker->connection = NULL;
}
@@ -897,6 +896,7 @@ static SndChannel *__new_channel(SndWorker *worker, int
size, uint32_t channel_i
int tos;
MainChannelClient *mcc = red_client_get_main(client);
+spice_assert(stream);
if ((flags = fcntl(stream->socket, F_GETFL)) == -1) {
spice_printerr("accept failed, %s", strerror(errno));
goto error1;
--
1.8.1.4
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH spice-server 4/8] decouple disconnection of the main channel from client destruction
Fixes rhbz#918169
Some channels make direct calls to reds/main_channel routines. If
these routines try to read/write to the socket, and they get socket
error, main_channel_client_on_disconnect is called, and triggers
red_client_destroy. In order to prevent accessing expired references
to RedClient, RedChannelClient, or other objects (inside the original call,
after
red_client_destroy has been called) I made the call to
red_client_destroy asynchronous with respect to
main_channel_client_on_disconnect.
I added MAIN_DISPATCHER_CLIENT_DISCONNECT to main_dispatcher.
main_channel_client_on_disconnect pushes this msg to the dispatcher,
instead of calling directly to reds_client_disconnect.
The patch uses RedClient ref-count in order to handle a case where
reds_client_disconnect is called directly (e.g., when a new client connects
while
another one is connected), while there is already CLIENT_DISCONNECT msg
pending in the main_dispatcher.
Examples:
(1) snd_worker.c
snd_disconnect_channel()
channel->cleanup() //snd_playback_cleanup
reds_enable_mm_timer()
.
.
main_channel_push_multi_media_time()...socket_error
.
.
red_client_destory()
.
.
snd_disconnect_channel()
channel->cleanup()
celt051_encoder_destroy()
celt051_encoder_destory() // double release
Note that this bug could have been solved by changing the order of
calls: e.g., channel->stream = NULL before calling cleanup, and
some other changes + reference counting. However, I found other
places in the code with similar problems, and I looked for a general
solution, at least till we redesign red_channel to handle reference
counting more consistently.
(2) inputs_channel.c
inputs_connect()
main_channel_client_push_notify()...socket_error
.
.
red_client_destory()
.
.
red_channel_client_create() // refers to client which is already
destroyed
(3) reds.c
reds_handle_main_link()
main_channel_push_init() ...socket error
.
.
red_client_destory()
.
.
main_channel_client_start_net_test(mcc) // refers to mcc which is
already destroyed
This can explain the assert in rhbz#964136, comment #1 (but not the hang
that occurred before).
---
server/main_channel.c| 9 +
server/main_dispatcher.c | 37 +
server/main_dispatcher.h | 7 +++
server/reds.c| 1 +
server/reds.h| 2 ++
5 files changed, 52 insertions(+), 4 deletions(-)
diff --git a/server/main_channel.c b/server/main_channel.c
index 233e633..fe032a6 100644
--- a/server/main_channel.c
+++ b/server/main_channel.c
@@ -46,6 +46,7 @@
#include "red_common.h"
#include "reds.h"
#include "migration_protocol.h"
+#include "main_dispatcher.h"
#define ZERO_BUF_SIZE 4096
@@ -175,13 +176,13 @@ int main_channel_is_connected(MainChannel *main_chan)
return red_channel_is_connected(&main_chan->base);
}
-// when disconnection occurs, let reds shutdown all channels. This will
trigger the
-// real disconnection of main channel
+/*
+ * When the main channel is disconnected, disconnect the entire client.
+ */
static void main_channel_client_on_disconnect(RedChannelClient *rcc)
{
spice_printerr("rcc=%p", rcc);
-reds_client_disconnect(rcc->client);
-//red_channel_client_disconnect(rcc);
+main_dispatcher_client_disconnect(rcc->client);
}
RedClient *main_channel_get_client_by_link_id(MainChannel *main_chan, uint32_t
connection_id)
diff --git a/server/main_dispatcher.c b/server/main_dispatcher.c
index bf160dd..dbe1037 100644
--- a/server/main_dispatcher.c
+++ b/server/main_dispatcher.c
@@ -41,6 +41,7 @@ enum {
MAIN_DISPATCHER_CHANNEL_EVENT = 0,
MAIN_DISPATCHER_MIGRATE_SEAMLESS_DST_COMPLETE,
MAIN_DISPATCHER_SET_MM_TIME_LATENCY,
+MAIN_DISPATCHER_CLIENT_DISCONNECT,
MAIN_DISPATCHER_NUM_MESSAGES
};
@@ -59,6 +60,10 @@ typedef struct MainDispatcherMmTimeLatencyMessage {
uint32_t latency;
} MainDispatcherMmTimeLatencyMessage;
+typedef struct MainDispatcherClientDisconnectMessage {
+RedClient *client;
+} MainDispatcherClientDisconnectMessage;
+
/* channel_event - calls core->channel_event, must be done in main thread */
static void main_dispatcher_self_handle_channel_event(
int event,
@@ -108,6 +113,16 @@ static void main_dispatcher_handle_mm_time_latency(void
*opaque,
red_client_unref(msg->client);
}
+static void main_dispatcher_handle_client_disconnect(void *opaque,
+ void *payload)
+{
+MainDispatcherClientDisconne
[Spice-devel] [PATCH spice-server 5/8] reds: s/red_client_disconnect/red_channel_client_shutdown inside callbacks
When we want to disconnect the main channel from a callback, it is
safer to use red_channel_client_shutdown, instead of directly
destroying the client. It is also more consistent with how other
channels treat errors.
red_channel_client_shutdown will trigger socket error in the main channel.
Then, main_channel_client_on_disconnect will be called,
and eventually, main_dispatcher_client_disconnect.
I didn't replace calls to reds_disconnect/reds_client_disconnect in
places where those calls were safe && that might need immediate client
disconnection.
---
server/reds.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/server/reds.c b/server/reds.c
index c66ddc4..ae87c90 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -879,7 +879,8 @@ static void vdi_port_on_free_self_token(void *opaque)
static void vdi_port_remove_client(RedClient *client, void *opaque)
{
-reds_client_disconnect(client);
+red_channel_client_shutdown(main_channel_client_get_base(
+red_client_get_main(client)));
}
//
@@ -1009,7 +1010,7 @@ void reds_on_main_agent_start(MainChannelClient *mcc,
uint32_t num_tokens)
if (!client_added) {
spice_warning("failed to add client to agent");
-reds_client_disconnect(rcc->client);
+red_channel_client_shutdown(rcc);
return;
}
} else {
@@ -1126,7 +1127,7 @@ void reds_on_main_agent_data(MainChannelClient *mcc, void
*message, size_t size)
reds_on_main_agent_monitors_config(mcc, message, size);
return;
case AGENT_MSG_FILTER_PROTO_ERROR:
-reds_disconnect();
+red_channel_client_shutdown(main_channel_client_get_base(mcc));
return;
}
--
1.8.1.4
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [qxl-win PATCH] display: add punting where it is missing
---
display/brush.c | 4 +++-
display/driver.c | 2 ++
display/pointer.c | 8
3 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/display/brush.c b/display/brush.c
index 0b9400d..fe94f1c 100644
--- a/display/brush.c
+++ b/display/brush.c
@@ -235,10 +235,12 @@ BOOL APIENTRY DrvRealizeBrush(BRUSHOBJ *brush, SURFOBJ
*target, SURFOBJ *pattern
int size;
if (!(pdev = (PDev *)target->dhpdev)) {
-ASSERT(NULL, 0);
+DEBUG_PRINT((NULL, 0, "%s: err no pdev\n", __FUNCTION__));
return FALSE;
}
+PUNT_IF_DISABLED(pdev);
+
DEBUG_PRINT((pdev, 3, "%s\n", __FUNCTION__));
if (mask) {
diff --git a/display/driver.c b/display/driver.c
index d7fdbf7..b8050de 100644
--- a/display/driver.c
+++ b/display/driver.c
@@ -1414,6 +1414,8 @@ HBITMAP APIENTRY DrvCreateDeviceBitmap(DHPDEV dhpdev,
SIZEL size, ULONG format)
pdev = (PDev *)dhpdev;
+PUNT_IF_DISABLED(pdev);
+
if (!pdev->create_non_primary_surfaces) {
return FALSE;
}
diff --git a/display/pointer.c b/display/pointer.c
index d38a207..17fa3c5 100644
--- a/display/pointer.c
+++ b/display/pointer.c
@@ -43,6 +43,8 @@ ULONG APIENTRY DrvSetPointerShape(SURFOBJ *surf, SURFOBJ
*mask, SURFOBJ *color_p
return SPS_ERROR;
}
+PUNT_IF_DISABLED(pdev);
+
DEBUG_PRINT((pdev, 3, "%s\n", __FUNCTION__));
if (flags & SPS_CHANGE) {
@@ -129,6 +131,12 @@ VOID APIENTRY DrvMovePointer(SURFOBJ *surf, LONG pos_x,
LONG pos_y, RECTL *area)
return;
}
+if (!pdev->enabled) {
+DEBUG_PRINT((pdev, 3, "%s: ignoring when device is disabled\n",
+__FUNCTION__));
+return;
+}
+
cursor_cmd = CursorCmd(pdev);
if (pos_x < 0) {
cursor_cmd->type = QXL_CURSOR_HIDE;
--
1.8.3.1
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] win-xp: uninstalling spice-guest-tools
On Fri, Jul 26 2013, Christophe Fergeau wrote: > And this was working before the installation of the guest drivers? I'm > surprised these interact with -usb. Hi Christophe, Yes. > I'd check in the device manager if drivers for the various virtio* devices > are still there, and remove them by hand. Thanks, your hint has helped: After removing my 2 virtio ethernet cards, everything works nicely. I've just removed "model=virtio" from my 2 "-net nic" options, and now Win-XP works with spice and usb. It seems, that the spice installation adds the virtio drivers at the same time as the spice agent. That's why I thought it was a spice problem. Sorry for the noise. > Have you tried using spice USB redirection? I think this works without > using -usb. No. Thanks for the information, but I don't need it no more, because -usb works now as expected. Thanks for your efforts, -- Peter ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
