>
> > On 9 Feb 2018, at 17:59, Frediano Ziglio <fziglio@xxxxxxxxxx> 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel