[Spice-commits] 5 commits - client/x11 server/reds.c

Hans de Goede jwrdegoede at kemper.freedesktop.org
Sat Oct 16 06:45:23 PDT 2010


 client/x11/platform.cpp |  126 ++++++++++++++++++++++++++----------------------
 server/reds.c           |   57 ++++++++-------------
 2 files changed, 92 insertions(+), 91 deletions(-)

New commits:
commit 21f586762f96f9b0c3237b2b7ea21fbc9022435c
Author: Hans de Goede <hdegoede at redhat.com>
Date:   Fri Oct 15 09:36:52 2010 +0200

    server: remove useless agent send_tokens
    
    We are keeping track of tokens for sending agent data to the client, but
    the client send an initial value of ~0, and never gives us new send tokens
    so this is all rather useless -> remove it.
    
    Note that it is kept in the migration data struct for compatibility reasons.

diff --git a/server/reds.c b/server/reds.c
index 5160b01..7ab4925 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -203,7 +203,6 @@ typedef struct VDIPortState {
     VDIChunkHeader vdi_chunk_header;
 
     int client_agent_started;
-    uint32_t send_tokens;
 } VDIPortState;
 
 typedef struct InputsState {
@@ -710,7 +709,6 @@ static void reds_reset_vdp()
         state->current_read_buf = NULL;
     }
     state->client_agent_started = FALSE;
-    state->send_tokens = 0;
 }
 
 static void reds_reset_outgoing()
@@ -1288,13 +1286,6 @@ static int read_from_vdi_port(void)
                 break;
             }
 
-            if (state->vdi_chunk_header.port == VDP_CLIENT_PORT) {
-                if (!state->send_tokens) {
-                    quit_loop = 1;
-                    break;
-                }
-                --state->send_tokens;
-            }
             ring_remove(item);
             state->current_read_buf = (VDIReadBuf *)item;
             state->recive_pos = state->current_read_buf->data;
@@ -1414,7 +1405,7 @@ static void main_channel_push_migrate_data_item()
     data->agent_connected = !!state->connected;
     data->client_agent_started = state->client_agent_started;
     data->num_client_tokens = state->num_client_tokens;
-    data->send_tokens = state->send_tokens;
+    data->send_tokens = ~0;
 
     data->read_state = state->read_state;
     data->vdi_chunk_header = state->vdi_chunk_header;
@@ -1633,8 +1624,6 @@ static void main_channel_recive_migrate_data(MainMigrateData *data, uint8_t *end
     ASSERT(state->num_client_tokens + data->write_queue_size <= REDS_AGENT_WINDOW_SIZE +
                                                                 REDS_NUM_INTERNAL_AGENT_MESSAGES);
     state->num_tokens = REDS_AGENT_WINDOW_SIZE - state->num_client_tokens;
-    state->send_tokens = data->send_tokens;
-
 
     if (!data->agent_connected) {
         if (state->connected) {
@@ -1669,19 +1658,13 @@ static void main_channel_recive_migrate_data(MainMigrateData *data, uint8_t *end
 static void reds_main_handle_message(void *opaque, size_t size, uint32_t type, void *message)
 {
     switch (type) {
-    case SPICE_MSGC_MAIN_AGENT_START: {
-        SpiceMsgcMainAgentTokens *agent_start;
-
+    case SPICE_MSGC_MAIN_AGENT_START:
         red_printf("agent start");
         if (!reds->peer) {
             return;
         }
-        agent_start = (SpiceMsgcMainAgentTokens *)message;
         reds->agent_state.client_agent_started = TRUE;
-        reds->agent_state.send_tokens = agent_start->num_tokens; // TODO: sanitize? coming from guest!
-        while (read_from_vdi_port());
         break;
-    }
     case SPICE_MSGC_MAIN_AGENT_DATA: {
         RingItem *ring_item;
         VDAgentExtBuf *buf;
@@ -1725,19 +1708,8 @@ static void reds_main_handle_message(void *opaque, size_t size, uint32_t type, v
         write_to_vdi_port();
         break;
     }
-    case SPICE_MSGC_MAIN_AGENT_TOKEN: {
-        SpiceMsgcMainAgentTokens *token;
-
-        if (!reds->agent_state.client_agent_started) {
-            red_printf("SPICE_MSGC_MAIN_AGENT_TOKEN race");
-            break;
-        }
-
-        token = (SpiceMsgcMainAgentTokens *)message;
-        reds->agent_state.send_tokens += token->num_tokens;
-        while (read_from_vdi_port());
+    case SPICE_MSGC_MAIN_AGENT_TOKEN:
         break;
-    }
     case SPICE_MSGC_MAIN_ATTACH_CHANNELS:
         reds_send_channels();
         break;
@@ -2048,7 +2020,6 @@ static void reds_handle_main_link(RedLinkInfo *link)
         reds_send_link_result(link, SPICE_LINK_ERR_OK);
         while((connection_id = rand()) == 0);
         reds->agent_state.num_tokens = 0;
-        reds->agent_state.send_tokens = 0;
         memcpy(&(reds->taTicket), &taTicket, sizeof(reds->taTicket));
         reds->mig_target = FALSE;
     } else {
@@ -2106,6 +2077,8 @@ static void reds_handle_main_link(RedLinkInfo *link)
 
         reds_push_pipe_item(item);
         reds_start_net_test();
+        /* Now that we have a client, forward any pending agent data */
+        while (read_from_vdi_port());
     }
 }
 
commit 0b2336cd9c556cec98457e16e09e6c9855d81e82
Author: Hans de Goede <hdegoede at redhat.com>
Date:   Fri Oct 15 09:16:49 2010 +0200

    Call read_from_vdi_port() from vdi_read_buf_release()
    
    read_from_vdi_port(), called from vdagent_char_device_wakeup() may
    fail to consume all data because no buffers are available in the
    read_bufs ring. When this happens we would fail to ever read more data
    from the agent on the guest as the port is throttled and stays throttled
    until we've consumed all data from the current buffer.
    
    This patch re-enables the call to read_from_vdi_port() from
    vdi_read_buf_release(), so that we will try the read again when space
    becomes available in the read_bufs ring.
    
    Together with another nasty hack in the linux guest virtio_console
    driver, where it waits for a write to be acked by the host before
    continuing with the next one, this can lead to a linux guest
    getting stuck / hang (until the write is read by the spice-server
    which never happens becaus of the above issues).
    
    Note that even with this patch, the guest will still gets stuck due to
    a bug in watch_update_mask in spice-core in qemu, which causes writing
    to the client to never resume once it blocked. A patch for this has been
    submitted to qemu.

diff --git a/server/reds.c b/server/reds.c
index 8e63062..5160b01 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -1202,7 +1202,12 @@ void vdi_read_buf_release(uint8_t *data, void *opaque)
     VDIReadBuf *buf = (VDIReadBuf *)opaque;
 
     ring_add(&reds->agent_state.read_bufs, &buf->link);
-    //read_from_vdi_port(); // XXX WTF? - ask arnon. should we be calling this here?? (causes recursion of read_from_vdi_port..) 
+    /* read_from_vdi_port() may have never completed because the read_bufs
+       ring was empty. So we call it again so it can complete its work if
+       necessary. Note since we can be called from read_from_vdi_port ourselves
+       this can cause recursion, read_from_vdi_port() contains code protecting
+       it against this. */
+    while (read_from_vdi_port());
 }
 
 static void dispatch_vdi_port_data(int port, VDIReadBuf *buf)
@@ -1235,7 +1240,12 @@ static void dispatch_vdi_port_data(int port, VDIReadBuf *buf)
    it returns 0 ensures that all data has been consumed. */
 static int read_from_vdi_port(void)
 {
-    // FIXME: UGLY HACK. Result of spice-vmc vmc_read triggering flush of throttled data, and recalling this.
+    /* There are 2 scenarios where we can get called recursively:
+       1) spice-vmc vmc_read triggering flush of throttled data, recalling us
+       2) the buf we push to the client may be send immediately without
+          blocking, in which case its free function will recall us
+       This messes up the state machine, so ignore recursive calls.
+       This is why we always must be called in a loop. */
     static int inside_call = 0;
     int quit_loop = 0;
     VDIPortState *state = &reds->agent_state;
commit a52324525d5707365c4b6758e5d5b08f21b0ac31
Author: Hans de Goede <hdegoede at redhat.com>
Date:   Fri Oct 15 09:08:10 2010 +0200

    server: always call read_from_vdi_port() in a while loop
    
    read_from_vdi_port() MUST always be called in a while loop until it returns 0.
    
    This is needed because it can cause new data available events and its
    recursion protection causes those to get lost. Calling it until it returns 0
    ensures that all data has been consumed.
    
    Example scenario where we can fail to read the available data otherwise:
    - server/reds.c: vdagent_char_device_wakeup get called
      by hw/spice-vmc.c because data has arrived from the guest,
    - hw/spice-vmc.c: vmc_read get calls
    - If the vmc_read call depletes the current buffer it calls
      virtio_serial_throttle_port(&svc->port, false)
    - This causes vmc_have_data to get called, which if in the
      mean time another buffer has arrived causes
      vdagent_char_device_wakeup to gets called again
      (so recursively)
    - vdagent_char_device_wakeup is protected against recursive
      calling and ignores the second call (a nasty hack)
    - if no other data arrives, the arrived data will not get read

diff --git a/server/reds.c b/server/reds.c
index a88ca95..8e63062 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -1229,6 +1229,10 @@ static void dispatch_vdi_port_data(int port, VDIReadBuf *buf)
     }
 }
 
+/* Note this function MUST always be called in a while loop until it
+   returns 0. This is needed because it can cause new data available events
+   and its recursion protection causes those to get lost. Calling it until
+   it returns 0 ensures that all data has been consumed. */
 static int read_from_vdi_port(void)
 {
     // FIXME: UGLY HACK. Result of spice-vmc vmc_read triggering flush of throttled data, and recalling this.
@@ -1665,7 +1669,7 @@ static void reds_main_handle_message(void *opaque, size_t size, uint32_t type, v
         agent_start = (SpiceMsgcMainAgentTokens *)message;
         reds->agent_state.client_agent_started = TRUE;
         reds->agent_state.send_tokens = agent_start->num_tokens; // TODO: sanitize? coming from guest!
-        read_from_vdi_port();
+        while (read_from_vdi_port());
         break;
     }
     case SPICE_MSGC_MAIN_AGENT_DATA: {
@@ -1721,7 +1725,7 @@ static void reds_main_handle_message(void *opaque, size_t size, uint32_t type, v
 
         token = (SpiceMsgcMainAgentTokens *)message;
         reds->agent_state.send_tokens += token->num_tokens;
-        read_from_vdi_port();
+        while (read_from_vdi_port());
         break;
     }
     case SPICE_MSGC_MAIN_ATTACH_CHANNELS:
commit d37adccfa7e4586a68e7da8a225de0a50c6eeff5
Author: Hans de Goede <hdegoede at redhat.com>
Date:   Wed Oct 13 08:07:36 2010 +0200

    Don't crash when a client disconnects while there were pending writes

diff --git a/server/reds.c b/server/reds.c
index fcdda79..a88ca95 100644
--- a/server/reds.c
+++ b/server/reds.c
@@ -1894,7 +1894,7 @@ static void reds_main_event(int fd, int event, void *data)
         RedsOutgoingData *outgoing = &reds->outgoing;
         if (reds_send_data()) {
             reds_push();
-            if (!outgoing->item) {
+            if (!outgoing->item && reds->peer) {
                 core->watch_update_mask(reds->peer->watch,
                                         SPICE_WATCH_EVENT_READ);
             }
commit f78ad47407689d1a19be0decfbff82c62bd4b190
Author: Hans de Goede <hdegoede at redhat.com>
Date:   Tue Oct 12 15:53:35 2010 +0200

    spicec-x11: add support for image copy and paste

diff --git a/client/x11/platform.cpp b/client/x11/platform.cpp
index 9751a30..0642922 100644
--- a/client/x11/platform.cpp
+++ b/client/x11/platform.cpp
@@ -107,13 +107,24 @@ static unsigned int caps_lock_mask = 0;
 static unsigned int num_lock_mask = 0;
 
 //FIXME: nicify
-static const char * const utf8_atom_names[] = {
-    "UTF8_STRING",
-    "text/plain;charset=UTF-8",
-    "text/plain;charset=utf-8",
+struct clipboard_format_info {
+    uint32_t type;
+    const char *atom_names[16];
+    Atom atoms[16];
+    int atom_count;
 };
 
-#define utf8_atom_count (sizeof(utf8_atom_names)/sizeof(utf8_atom_names[0]))
+static struct clipboard_format_info clipboard_formats[] = {
+    { VD_AGENT_CLIPBOARD_UTF8_TEXT, { "UTF8_STRING",
+      "text/plain;charset=UTF-8", "text/plain;charset=utf-8", NULL }, },
+    { VD_AGENT_CLIPBOARD_IMAGE_PNG, { "image/png", NULL }, },
+    { VD_AGENT_CLIPBOARD_IMAGE_BMP, { "image/bmp", "image/x-bmp",
+      "image/x-MS-bmp", "image/x-win-bitmap", NULL }, },
+    { VD_AGENT_CLIPBOARD_IMAGE_TIFF, { "image/tiff", NULL }, },
+    { VD_AGENT_CLIPBOARD_IMAGE_JPG, { "image/jpeg", NULL }, },
+};
+
+#define clipboard_format_count (sizeof(clipboard_formats)/sizeof(clipboard_formats[0]))
 
 struct selection_request {
     XEvent event;
@@ -128,15 +139,11 @@ static int32_t clipboard_data_space = 0;
 static Atom clipboard_request_target = None;
 static selection_request *next_selection_request = NULL;
 static uint32_t clipboard_type_count = 0;
-/* TODO Add support for more types here */
-/* Warning the size of these 2 needs to be increased each time we add
-   support for a new type!! */
-static uint32_t clipboard_agent_types[1];
-static Atom clipboard_x11_targets[1];
+static uint32_t clipboard_agent_types[256];
+static Atom clipboard_x11_targets[256];
 static Mutex clipboard_lock;
 static Atom clipboard_prop;
 static Atom incr_atom;
-static Atom utf8_atoms[utf8_atom_count];
 static Atom targets_atom;
 static Atom multiple_atom;
 static Bool handle_x_error = false;
@@ -188,16 +195,18 @@ static const char *atom_name(Atom atom)
 }
 
 static uint32_t get_clipboard_type(Atom target) {
-    int i;
+    int i, j;
 
     if (target == None)
         return VD_AGENT_CLIPBOARD_NONE;
 
-    for (i = 0; i < utf8_atom_count; i++)
-        if (utf8_atoms[i] == target)
-            return VD_AGENT_CLIPBOARD_UTF8_TEXT;
-
-    /* TODO Add support for more types here */
+    for (i = 0; i < clipboard_format_count; i++) {
+        for (j = 0; j < clipboard_formats[i].atom_count; i++) {
+            if (clipboard_formats[i].atoms[j] == target) {
+                return clipboard_formats[i].type;
+            }
+        }
+    }
 
     LOG_WARN("unexpected selection type %s", atom_name(target));
     return VD_AGENT_CLIPBOARD_NONE;
@@ -871,7 +880,7 @@ private:
 
 static void intern_clipboard_atoms()
 {
-    int i;
+    int i, j;
     static bool interned = false;
     if (interned) return;
 
@@ -880,8 +889,14 @@ static void intern_clipboard_atoms()
     incr_atom = XInternAtom(x_display, "INCR", False);
     multiple_atom = XInternAtom(x_display, "MULTIPLE", False);
     targets_atom = XInternAtom(x_display, "TARGETS", False);
-    for(i = 0; i < utf8_atom_count; i++)
-        utf8_atoms[i] = XInternAtom(x_display, utf8_atom_names[i], False);
+    for(i = 0; i < clipboard_format_count; i++) {
+        for(j = 0; clipboard_formats[i].atom_names[j]; j++) {
+            clipboard_formats[i].atoms[j] =
+                XInternAtom(x_display, clipboard_formats[i].atom_names[j],
+                            False);
+        }
+        clipboard_formats[i].atom_count = j;
+    }
 
     XUnlockDisplay(x_display);
 
@@ -2402,23 +2417,25 @@ static void print_targets(const char *action, Atom *atoms, int c)
 
 static void send_targets(XEvent& request_event)
 {
-    /* TODO Add support for more types here */
-    /* Warning the size of this needs to be increased each time we add support
-       for a new type, or the atom count of an existing type changes */
-    Atom targets[4] = { targets_atom, };
-    int i, j, target_count = 1;
+    Atom targets[256] = { targets_atom, };
+    int i, j, k, target_count = 1;
 
     for (i = 0; i < clipboard_type_count; i++) {
-        switch (clipboard_agent_types[i]) {
-            case VD_AGENT_CLIPBOARD_UTF8_TEXT:
-                for (j = 0; j < utf8_atom_count; j++) {
-                    targets[target_count] = utf8_atoms[j];
-                    target_count++;
+        for (j = 0; j < clipboard_format_count; j++) {
+            if (clipboard_formats[j].type != clipboard_agent_types[i]) {
+                continue;
+            }
+            for (k = 0; k < clipboard_formats[j].atom_count; k++) {
+                targets[target_count] = clipboard_formats[j].atoms[k];
+                target_count++;
+                if (target_count == sizeof(targets)/sizeof(Atom)) {
+                    LOG_WARN("sendtargets: too many targets");
+                    goto exit_loop;
                 }
-                break;
-            /* TODO Add support for more types here */
+            }
         }
     }
+exit_loop:
 
     Window requestor_win = request_event.xselectionrequest.requestor;
     Atom prop = request_event.xselectionrequest.property;
@@ -2576,9 +2593,9 @@ static Atom atom_lists_overlap(Atom *atoms1, Atom *atoms2, int l1, int l2)
 
 static void handle_targets_notify(XEvent& event, bool incr)
 {
-    int len;
+    int i, len;
     Lock lock(clipboard_lock);
-    Atom *atoms = NULL;
+    Atom atom, *atoms = NULL;
 
     if (!expected_targets_notifies) {
         LOG_WARN("unexpected selection notify TARGETS");
@@ -2602,16 +2619,22 @@ static void handle_targets_notify(XEvent& event, bool incr)
     print_targets("received", atoms, len);
 
     clipboard_type_count = 0;
-    Atom atom = atom_lists_overlap(utf8_atoms, atoms, utf8_atom_count, len);
-    if (atom) {
-        clipboard_agent_types[clipboard_type_count] =
-            VD_AGENT_CLIPBOARD_UTF8_TEXT;
-        clipboard_x11_targets[clipboard_type_count] = atom;
-        clipboard_type_count++;
+    for (i = 0; i < clipboard_format_count; i++) {
+        atom = atom_lists_overlap(clipboard_formats[i].atoms, atoms,
+                                  clipboard_formats[i].atom_count, len);
+        if (atom) {
+            clipboard_agent_types[clipboard_type_count] =
+                clipboard_formats[i].type;
+            clipboard_x11_targets[clipboard_type_count] = atom;
+            clipboard_type_count++;
+            if (clipboard_type_count ==
+                    sizeof(clipboard_agent_types)/sizeof(uint32_t)) {
+                LOG_WARN("handle_targets_notify: too many matching types");
+                break;
+            }
+        }
     }
 
-    /* TODO Add support for more types here */
-
     if (clipboard_type_count)
         clipboard_listener->on_clipboard_grab(clipboard_agent_types,
                                               clipboard_type_count);
@@ -3459,23 +3482,14 @@ LocalCursor* Platform::create_default_cursor()
 bool Platform::on_clipboard_grab(uint32_t *types, uint32_t type_count)
 {
     Lock lock(clipboard_lock);
-    int i;
-
-    clipboard_type_count = 0;
-    for (i = 0; i < type_count; i++) {
-        /* TODO Add support for more types here */
-        /* Check if we support the type */
-        if (types[i] != VD_AGENT_CLIPBOARD_UTF8_TEXT)
-            continue;
 
-        clipboard_agent_types[clipboard_type_count] = types[i];
-        clipboard_type_count++;
+    if (type_count > sizeof(clipboard_agent_types)/sizeof(uint32_t)) {
+        LOG_WARN("on_clipboard_grab: too many types");
+        type_count = sizeof(clipboard_agent_types)/sizeof(uint32_t);
     }
 
-    if (!clipboard_type_count) {
-        LOG_INFO("No supported clipboard types in agent grab");
-        return false;
-    }
+    memcpy(clipboard_agent_types, types, type_count * sizeof(uint32_t));
+    clipboard_type_count = type_count;
 
     XSetSelectionOwner(x_display, clipboard_prop, platform_win, CurrentTime);
     XFlush(x_display);


More information about the Spice-commits mailing list