[Spice-devel] [linux-vdagent PATCH 4/4] vdagent-virtio-port: properly demultiplex vdagent messages per port

[Hans de Goede]

Hi,

On 03/24/2011 09:41 AM, Alon Levy wrote:
> On Wed, Mar 23, 2011 at 08:31:08PM +0100, Hans de Goede wrote:
>> Before this patch vdagent-virtio-port was assembling vdagent messages
>> which consist of multiple chunks without looking at the chunk header port
>> attribute. But it is possible to receive a vdagent-message which spans
>> multiple chunks for port 1, and while receiving the chunks for this message,
>> receive an unrelated chunk for port 2. Before this patch that chunk would
>> (wrongly) get added to the message for port2, messing things seriously up.
>> ---
>>   vdagent-virtio-port.c |   68 +++++++++++++++++++++++++++++++-----------------
>>   1 files changed, 44 insertions(+), 24 deletions(-)
>>
>> diff --git a/vdagent-virtio-port.c b/vdagent-virtio-port.c
>> index 06c18ec..02457d1 100644
>> --- a/vdagent-virtio-port.c
>> +++ b/vdagent-virtio-port.c
>> @@ -27,6 +27,8 @@
>>   #include<sys/select.h>
>>   #include "vdagent-virtio-port.h"
>>
>> +#define VDP_LAST_PORT VDP_SERVER_PORT
>> +
> would be nice to put this in spice-protocol (vd_agent.h)
>

Agreed I already expected this comment :) I'll do a separate
patch for spice-protocol, and then we can later drop the
define. The reason I did not do this immediately is that I
don't want to do a new spice-protocol just for this.

>>   struct vdagent_virtio_port_buf {
>>       uint8_t *buf;
>>       size_t pos;
>> @@ -35,19 +37,27 @@ struct vdagent_virtio_port_buf {
>>       struct vdagent_virtio_port_buf *next;
>>   };
>>
>> +/* Data to keep track of the assembling of vdagent messages per chunk port,
>> +   for de-multiplexing the messages */
>> +struct vdagent_virtio_port_chunk_port_data {
>> +    int message_header_read;
>> +    int message_data_pos;
>> +    VDAgentMessage message_header;
>> +    uint8_t *message_data;
>> +};
>> +
>>   struct vdagent_virtio_port {
>>       int fd;
>>       FILE *errfile;
>>
>> -    /* Read stuff, single buffer, separate header and data buffer */
>> +    /* Chunk read stuff, single buffer, separate header and data buffer */
>>       int chunk_header_read;
>>       int chunk_data_pos;
>> -    int message_header_read;
>> -    int message_data_pos;
>>       VDIChunkHeader chunk_header;
>> -    VDAgentMessage message_header;
>>       uint8_t chunk_data[VD_AGENT_MAX_DATA_SIZE];
>> -    uint8_t *message_data;
>> +
>> +    /* Per chunk port data */
>> +    struct vdagent_virtio_port_chunk_port_data port_data[VDP_LAST_PORT + 1];
>>
>>       /* Writes are stored in a linked list of buffers, with both the header
>>          + data for a single message in 1 buffer. */
>> @@ -90,6 +100,7 @@ void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
>>   {
>>       struct vdagent_virtio_port_buf *wbuf, *next_wbuf;
>>       struct vdagent_virtio_port *vport = *vportp;
>> +    int i;
>>
>>       if (!vport)
>>           return;
>> @@ -105,7 +116,9 @@ void vdagent_virtio_port_destroy(struct vdagent_virtio_port **vportp)
>>           wbuf = next_wbuf;
>>       }
>>
>> -    free(vport->message_data);
>> +    for (i = 0; i<= VDP_LAST_PORT; i++) {
> nitpick - you define a [VDP_LAST_PORT+1] array, and here
> you do<= VDP_LAST_PORT - I would use sizeof(all)/sizeof(one).
>

The Linux kernel has an ARRAY_SIZE macro which does what you want, would be good to add
that to some spice header I guess, suggestions?

Regards,

Hans