[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] virtio-serial-bus: use correct lengths in contr
From: |
Amit Shah |
Subject: |
Re: [Qemu-devel] [PATCH] virtio-serial-bus: use correct lengths in control_out() message |
Date: |
Mon, 12 Mar 2012 16:36:33 +0530 |
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