Re: [Qemu-devel] [PATCH] virtio-serial-bus: use correct lengths in control_out() message

[Amit Shah <amit.shah@redhat.com>]

On (Mon) 12 Mar 2012 [13:22:22], Michael Tokarev wrote:
> On 12.03.2012 12:59, Amit Shah wrote:
> > On (Sun) 11 Mar 2012 [17:52:59], Michael Tokarev wrote:
> >> In case of more than one control message, the code will use
> >> size of the largest message so far for all subsequent messages,
> >> instead of using size of current one.  Fix it.
> > 
> > Makes sense.  How did you detect this?  Any reproducible test-case?
> 
> There's no test-case, and no detection, just reading the code.
> Actually, I think, there's no bug here, but a very, well,
> difficult to read code.

Do you mean this code is difficult to read, or in general?  Any ideas
to make it simpler (or at least details on what's difficult?)

> >> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> >> index e22940e..abe48ec 100644
> >> --- a/hw/virtio-serial-bus.c
> >> +++ b/hw/virtio-serial-bus.c
> >> @@ -451,28 +451,28 @@ static void control_out(VirtIODevice *vdev, VirtQueue *vq)
> >>  
> >>      vser = DO_UPCAST(VirtIOSerial, vdev, vdev);
> >>  
> >>      len = 0;
> >>      buf = NULL;
> >>      while (virtqueue_pop(vq, &elem)) {
> >> -        size_t cur_len, copied;
> >> +        size_t cur_len;
> >>  
> >>          cur_len = iov_size(elem.out_sg, elem.out_num);
> >>          /*
> >>           * Allocate a new buf only if we didn't have one previously or
> >>           * if the size of the buf differs
> >>           */
> >>          if (cur_len > len) {
> >>              g_free(buf);
> >>  
> >>              buf = g_malloc(cur_len);
> >>              len = cur_len;
> >>          }
> >> -        copied = iov_to_buf(elem.out_sg, elem.out_num, buf, 0, len);
> >> +        iov_to_buf(elem.out_sg, elem.out_num, buf, 0, cur_len);
> > 
> > Why drop 'copied'?  I don't think we have had a situation where copied
> > can be less than cur_len, and in any case we don't do anything special
> > as a recovery mechanism, but a warning message or an abort in case
> > copied != cur_len should work, I think.
> 
> In this case, copied was _always_ == cur_len.  That's why there's
> actually no bug.  See:
> 
>    cur_len = iov_size(elem.out_sg, elem.out_num);
>    len = max(cur_len, buflen) <= "roughly"
>    copied = iov_to_buf(elem.out_sg, elem.out_num, buf, 0, len);
> 
> iov_to_buf() will stop copying when it reaches end of buf
> (which is "len" bytes long) or end of iov, which is cur_len
> bytes long.  Obviously in all cases it will be cur_len.
> But it is obvious only when you write it one near another
> and _think_.  And the reason for this confusion is the
> introduction of this `copied' variable, which shouldn't
> be there in the first place.
> 
> It is like doing, for a memcpy-like function:
> 
>  void *memdup(const void *src, size_t size) {
>    char *dest = malloc(size+1);
>    size_t copied = copybytes(dest, src, size+1);
>    if (copied != size+1) {
>       /* What?? */
>    }
>    return dest;
> }
> 
> The only sane thing here, I think, is to drop 'copied',
> to stop any possible confusion :)
>
> >> -        handle_control_message(vser, buf, copied);
> >> +        handle_control_message(vser, buf, cur_len);
> >>          virtqueue_push(vq, &elem, 0);
> >>      }
> >>      g_free(buf);
> >>      virtio_notify(vdev, vq);
> >>  }
> 
> Please belive me, I realized that the original code is
> actually right only after re-reading your reply.

Heh.

>  And
> please note that even you, the author, don't understand
> what it is doing :)

Of course, I can't claim to remember everything, I sometimes don't
even remember stuff *while* coding it.  However, I do understand this
part of the code, what I meant above was iov_to_buf() could fail or
copy lesser than what was asked of it.  It's a memcpy() right now, it
could change to something else.  Ignoring return values, esp. in copy
functions, is not good style, even if you know it can't fail.

>  So I think the patch is correct
> still ;)

No doubt about that.  I never said otherwise.  I just feel we
shouldn't ignore return values.

		Amit