| Message ID | 20120311135745.7866A65E0@gandalf.tls.msk.ru |
|---|---|
| State | New |
| Headers | show |
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
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
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
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!
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
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); }
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(-)