[Spice-devel,spice-server,10/14] spicevmc: use SpiceCharDeviceState for writing to the guest device

Submitted by Yonit Halperin on June 27, 2012, 3:16 p.m.

Details

Message ID 1340810212-26230-11-git-send-email-yhalperi@redhat.com
State Superseded
Headers show

Not browsing as part of any series.

Commit Message

Yonit Halperin June 27, 2012, 3:16 p.m.
With SpiceCharDeviceState, spicevmc now supports partial writes,
and storing data that is received from the client after the device is
stopped, instead of attempting to write it to the guest.
---
 server/spicevmc.c |   49 ++++++++++++++++++++++++-------------------------
 1 files changed, 24 insertions(+), 25 deletions(-)

Patch hide | download patch | download mbox

diff --git a/server/spicevmc.c b/server/spicevmc.c
index 628a900..766b93f 100644
--- a/server/spicevmc.c
+++ b/server/spicevmc.c
@@ -32,6 +32,11 @@ 
 #include "red_channel.h"
 #include "reds.h"
 
+/* todo: add flow control. i.e.,
+ * (a) limit the amount of data on each message
+ * (b) limit the tokens available for the client
+ * (c) limit the tokens available for the server
+ */
 /* 64K should be enough for all but the largest writes + 32 bytes hdr */
 #define BUF_SIZE (64 * 1024 + 32)
 
@@ -50,9 +55,7 @@  typedef struct SpiceVmcState {
     SpiceCharDeviceState *chardev_st;
     SpiceCharDeviceInstance *chardev_sin;
     SpiceVmcPipeItem *pipe_item;
-    uint8_t *rcv_buf;
-    uint32_t rcv_buf_size;
-    int rcv_buf_in_use;
+    SpiceCharDeviceWriteBuffer *recv_from_client_buf;
 } SpiceVmcState;
 
 static SpiceVmcPipeItem *spicevmc_pipe_item_ref(SpiceVmcPipeItem *item)
@@ -204,23 +207,16 @@  static int spicevmc_red_channel_client_handle_message(RedChannelClient *rcc,
                                                       uint8_t *msg)
 {
     SpiceVmcState *state;
-    SpiceCharDeviceInstance *sin;
-    SpiceCharDeviceInterface *sif;
 
     state = SPICE_CONTAINEROF(rcc->channel, SpiceVmcState, channel);
-    sin = state->chardev_sin;
-    sif = SPICE_CONTAINEROF(sin->base.sif, SpiceCharDeviceInterface, base);
 
     if (type != SPICE_MSGC_SPICEVMC_DATA) {
         return red_channel_client_handle_message(rcc, size, type, msg);
     }
 
-    /*
-     * qemu spicevmc will consume everything we give it, no need for
-     * flow control checks (or to use a pipe).
-     */
-    sif->write(sin, msg, size);
-
+    state->recv_from_client_buf->buf_used = size;
+    spice_char_device_write_buffer_add(state->chardev_st, state->recv_from_client_buf);
+    state->recv_from_client_buf = NULL;
     return TRUE;
 }
 
@@ -232,16 +228,16 @@  static uint8_t *spicevmc_red_channel_alloc_msg_rcv_buf(RedChannelClient *rcc,
 
     state = SPICE_CONTAINEROF(rcc->channel, SpiceVmcState, channel);
 
-    assert(!state->rcv_buf_in_use);
+    assert(!state->recv_from_client_buf);
 
-    if (size > state->rcv_buf_size) {
-        state->rcv_buf = spice_realloc(state->rcv_buf, size);
-        state->rcv_buf_size = size;
+    state->recv_from_client_buf = spice_char_device_write_buffer_get(state->chardev_st,
+                                                                     rcc->client,
+                                                                     size);
+    if (!state->recv_from_client_buf) {
+        spice_error("failed to allocate write buffer");
+        return NULL;
     }
-
-    state->rcv_buf_in_use = 1;
-
-    return state->rcv_buf;
+    return state->recv_from_client_buf->buf;
 }
 
 static void spicevmc_red_channel_release_msg_rcv_buf(RedChannelClient *rcc,
@@ -253,8 +249,10 @@  static void spicevmc_red_channel_release_msg_rcv_buf(RedChannelClient *rcc,
 
     state = SPICE_CONTAINEROF(rcc->channel, SpiceVmcState, channel);
 
-    /* NOOP, we re-use the buffer every time and only free it on destruction */
-    state->rcv_buf_in_use = 0;
+    if (state->recv_from_client_buf) { /* buffer wasn't pushed to device */
+        spice_char_device_write_buffer_release(state->chardev_st, state->recv_from_client_buf);
+        state->recv_from_client_buf = NULL;
+    }
 }
 
 static void spicevmc_red_channel_hold_pipe_item(RedChannelClient *rcc,
@@ -377,12 +375,13 @@  void spicevmc_device_disconnect(SpiceCharDeviceInstance *sin)
 
     state = (SpiceVmcState *)spice_char_device_state_opaque_get(sin->st);
 
+    if (state->recv_from_client_buf) {
+        spice_char_device_write_buffer_release(state->chardev_st, state->recv_from_client_buf);
+    }
     spice_char_device_state_destroy(sin->st);
     state->chardev_st = NULL;
 
     reds_unregister_channel(&state->channel);
-
     free(state->pipe_item);
-    free(state->rcv_buf);
     red_channel_destroy(&state->channel);
 }

Comments

Hi,

On 06/27/2012 05:16 PM, Yonit Halperin wrote:
> With SpiceCharDeviceState, spicevmc now supports partial writes,
> and storing data that is received from the client after the device is
> stopped, instead of attempting to write it to the guest.
> ---
>   server/spicevmc.c |   49 ++++++++++++++++++++++++-------------------------
>   1 files changed, 24 insertions(+), 25 deletions(-)
>
> diff --git a/server/spicevmc.c b/server/spicevmc.c
> index 628a900..766b93f 100644
> --- a/server/spicevmc.c
> +++ b/server/spicevmc.c
> @@ -32,6 +32,11 @@
>   #include "red_channel.h"
>   #include "reds.h"
>
> +/* todo: add flow control. i.e.,
> + * (a) limit the amount of data on each message

This is already done, see the BUF_SIZE define below :) And I don't
think we should go smaller then 64k with the size of the messages.

> + * (b) limit the tokens available for the client
> + * (c) limit the tokens available for the server
> + */
>   /* 64K should be enough for all but the largest writes + 32 bytes hdr */
>   #define BUF_SIZE (64 * 1024 + 32)
>
> @@ -50,9 +55,7 @@ typedef struct SpiceVmcState {
>       SpiceCharDeviceState *chardev_st;
>       SpiceCharDeviceInstance *chardev_sin;
>       SpiceVmcPipeItem *pipe_item;
> -    uint8_t *rcv_buf;
> -    uint32_t rcv_buf_size;
> -    int rcv_buf_in_use;
> +    SpiceCharDeviceWriteBuffer *recv_from_client_buf;
>   } SpiceVmcState;
>
>   static SpiceVmcPipeItem *spicevmc_pipe_item_ref(SpiceVmcPipeItem *item)
> @@ -204,23 +207,16 @@ static int spicevmc_red_channel_client_handle_message(RedChannelClient *rcc,
>                                                         uint8_t *msg)
>   {
>       SpiceVmcState *state;
> -    SpiceCharDeviceInstance *sin;
> -    SpiceCharDeviceInterface *sif;
>
>       state = SPICE_CONTAINEROF(rcc->channel, SpiceVmcState, channel);
> -    sin = state->chardev_sin;
> -    sif = SPICE_CONTAINEROF(sin->base.sif, SpiceCharDeviceInterface, base);
>
>       if (type != SPICE_MSGC_SPICEVMC_DATA) {
>           return red_channel_client_handle_message(rcc, size, type, msg);
>       }
>
> -    /*
> -     * qemu spicevmc will consume everything we give it, no need for
> -     * flow control checks (or to use a pipe).
> -     */
> -    sif->write(sin, msg, size);
> -

Add an assert here like this? :
	spice_assert(state->recv_from_client_buf->buf == msg);

> +    state->recv_from_client_buf->buf_used = size;
> +    spice_char_device_write_buffer_add(state->chardev_st, state->recv_from_client_buf);
> +    state->recv_from_client_buf = NULL;
>       return TRUE;
>   }
>
> @@ -232,16 +228,16 @@ static uint8_t *spicevmc_red_channel_alloc_msg_rcv_buf(RedChannelClient *rcc,
>
>       state = SPICE_CONTAINEROF(rcc->channel, SpiceVmcState, channel);
>
> -    assert(!state->rcv_buf_in_use);
> +    assert(!state->recv_from_client_buf);
>
> -    if (size > state->rcv_buf_size) {
> -        state->rcv_buf = spice_realloc(state->rcv_buf, size);
> -        state->rcv_buf_size = size;
> +    state->recv_from_client_buf = spice_char_device_write_buffer_get(state->chardev_st,
> +                                                                     rcc->client,
> +                                                                     size);
> +    if (!state->recv_from_client_buf) {
> +        spice_error("failed to allocate write buffer");
> +        return NULL;
>       }
> -
> -    state->rcv_buf_in_use = 1;
> -
> -    return state->rcv_buf;
> +    return state->recv_from_client_buf->buf;
>   }
>
>   static void spicevmc_red_channel_release_msg_rcv_buf(RedChannelClient *rcc,
> @@ -253,8 +249,10 @@ static void spicevmc_red_channel_release_msg_rcv_buf(RedChannelClient *rcc,
>
>       state = SPICE_CONTAINEROF(rcc->channel, SpiceVmcState, channel);
>
> -    /* NOOP, we re-use the buffer every time and only free it on destruction */
> -    state->rcv_buf_in_use = 0;
> +    if (state->recv_from_client_buf) { /* buffer wasn't pushed to device */
> +        spice_char_device_write_buffer_release(state->chardev_st, state->recv_from_client_buf);
> +        state->recv_from_client_buf = NULL;
> +    }
>   }
>
>   static void spicevmc_red_channel_hold_pipe_item(RedChannelClient *rcc,
> @@ -377,12 +375,13 @@ void spicevmc_device_disconnect(SpiceCharDeviceInstance *sin)
>
>       state = (SpiceVmcState *)spice_char_device_state_opaque_get(sin->st);
>
> +    if (state->recv_from_client_buf) {
> +        spice_char_device_write_buffer_release(state->chardev_st, state->recv_from_client_buf);
> +    }
>       spice_char_device_state_destroy(sin->st);
>       state->chardev_st = NULL;
>
>       reds_unregister_channel(&state->channel);
> -
>       free(state->pipe_item);
> -    free(state->rcv_buf);
>       red_channel_destroy(&state->channel);
>   }
>

Regards,

Hans