[Spice-devel] [PATCH spice-server v2 1/2] stream-device: handle cursor from device

[Christophe de Dinechin]

> On 9 Feb 2018, at 17:59, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
>>>> 
>>>> 
>>>>> +
>>>>> +    // read part of the message till we get all
>>>>> +    if (dev->msg_len < dev->hdr.size) {
>>>>> +        dev->msg = g_realloc(dev->msg, dev->hdr.size);
>>>>> +        dev->msg_len = dev->hdr.size;
>>>>> +    }
>>>>> +    int n = sif->read(sin, dev->msg->buf + dev->msg_pos, dev->hdr.size -
>>>>> dev->msg_pos);
>>>>> +    if (n <= 0) {
>>>>> +        return false;
>>>>> +    }
>>>>> +    dev->msg_pos += n;
>>>>> +    if (dev->msg_pos != dev->hdr.size) {
>>>> 
>>>> Is it assumed you can all read in one go? I’m surprized there is a “while”
>>>> loop for reading the header (which is small) but no while loop for reading
>>>> the payload which is larger).
>>>> 
>>> 
>>> When we return false is not an error, just there's no data left next time
>>> will add more data. So there's no while but there's a loop anyway.
>>> I suppose similarly we could avoid the while in the other function.
>> 
>> Oh, so you are saying that you think it’s OK to loop ouside, truncate the
>> data that you g_realloc’d, and then start over?
>> 
>> Does not work. Say you started with 1K in buffer, then go to this inner read
>> here, which does a g_realloc and reads 10K. Then we re-enter the above
>> function, so I believe we will truncate to 1K again, and then re-extend to
>> 10K and start reading at 10K. We lost 9K of data, unless I’m mistaken.
>> 
> 
> No, the header is not read again until all message is processed.
> There are also tests for this.

This is not what I am talking about. I’m saying you read this, where H is header and D is data.

You first read header, let’s say four items.

[HHHH]

Then you realloc, which puts some undefined garbage in the area the data:

[HHHH][????????????]

Then you read data, get 4 elements:

[HHHH][DDDD????????]

So you restart in the outer loop, where you truncate the header

[HHHH]

and reallocate

[HHHH][????????????]

and restart reading, not sure if we have reset msg_pos or not, so you get either

[HHHH][DDDDDDDD????]

or

[HHHH][????DDDDDDDD]


None of which is correct. I’ve not seen in the code how we prevent this if we don’t loop in the inner read.

Maybe I missed the part that makes us restart correctly, but where did we keep the data?


Regards
Christophe