Patchwork virtio-serial-bus: use correct lengths in control_out() message

login
register
mail settings
Submitter Michael Tokarev
Date March 11, 2012, 1:52 p.m.
Message ID <20120311135745.7866A65E0@gandalf.tls.msk.ru>
Download mbox | patch
Permalink /patch/145980/
State New
Headers show

Comments

Michael Tokarev - March 11, 2012, 1:52 p.m.
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.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 hw/virtio-serial-bus.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
Amit Shah - March 12, 2012, 8:59 a.m.
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?

One comment below.

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

> -        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);
>  }

Thanks,

		Amit
Michael Tokarev - March 12, 2012, 9:22 a.m.
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.

> One comment below.

and the answer is below, too.

>> 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.  And
please note that even you, the author, don't understand
what it is doing :)  So I think the patch is correct
still ;)

Thanks!

/mjt
Amit Shah - March 12, 2012, 11:06 a.m.
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
Michael Tokarev - March 12, 2012, 11:44 a.m.
On 12.03.2012 15:06, Amit Shah wrote:
> 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?)

Just difficult to understand, and just this particular (very minor!)
place.

We got one thing, we requested to copy another, and we handle
3rd which is something else.  While actually we are supposed
to get, request and handle the _same_, or else we're doomed.

[]
>       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 that's the reason why the return value should be void, and
that the code should always request as many bytes as it actually
needs, and there must be some assert()s to ensure we're not
outside of something.

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

By _not_ ignoring return value in something like that is not far
away from checking if 1 is still equal to 1 after each instruction :)
I want to change this interface a bit, to be more obvious, to
stop all similar discussions and doubts.  It will handle the
requested amount and will abort() internally if it can't, so
we can stop bothering ignoring the return value, provided that
we actually request what we _want_ it to do, not what we have.
In this particular case, the 'size' argument of iov_from_buf()
should be 'bytes' or 'len', -- actual amount of bytes we need
to process, not size of the buffer we have in our disposal.

(For the iov_* family, we've another set, qemu_iovec_*, and
also qemu_sendv() &Co, each of which have different and not
obvious at all interface :)

Thanks!
Amit Shah - March 13, 2012, 2:01 p.m.
On (Mon) 12 Mar 2012 [15:44:07], Michael Tokarev wrote:
> On 12.03.2012 15:06, Amit Shah wrote:
> > 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?)
> 
> Just difficult to understand, and just this particular (very minor!)
> place.
> 
> We got one thing, we requested to copy another, and we handle
> 3rd which is something else.  While actually we are supposed
> to get, request and handle the _same_, or else we're doomed.

No doom anywhere, but thanks for the details.

> []
> >       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 that's the reason why the return value should be void, and
> that the code should always request as many bytes as it actually
> needs, and there must be some assert()s to ensure we're not
> outside of something.
> 
> >>  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.
> 
> By _not_ ignoring return value in something like that is not far
> away from checking if 1 is still equal to 1 after each instruction :)
> I want to change this interface a bit, to be more obvious, to
> stop all similar discussions and doubts.  It will handle the
> requested amount and will abort() internally if it can't, so
> we can stop bothering ignoring the return value, provided that
> we actually request what we _want_ it to do, not what we have.
> In this particular case, the 'size' argument of iov_from_buf()
> should be 'bytes' or 'len', -- actual amount of bytes we need
> to process, not size of the buffer we have in our disposal.

Can you make this patch a part of that series, then?

> (For the iov_* family, we've another set, qemu_iovec_*, and
> also qemu_sendv() &Co, each of which have different and not
> obvious at all interface :)

It's a feature to be consistent within one codebase :)

		Amit

Patch

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);
 
-        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);
 }