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

[Frediano Ziglio]

> 
> > 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
> 

why truncate the header?

The 

    while (dev->hdr_pos < sizeof(dev->hdr)) {

condition is not taken and we get back reading the partial message.

> [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
> 
> 

Frediano