| Message ID | 20170823083901.852-2-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Aug 23, 2017 at 10:39:00AM +0200, Juan Quintela wrote: > We were using -1 instead of the real size because the functions check > what is bigger, size in bytes or the size of the iov. Recent gcc's > barf at this. > > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > tests/test-iov.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/tests/test-iov.c b/tests/test-iov.c > index a22d71fd2c..819c410a51 100644 > --- a/tests/test-iov.c > +++ b/tests/test-iov.c > @@ -81,17 +81,17 @@ static void test_to_from_buf_1(void) > * skip whole vector and process exactly 0 bytes */ > > /* first set bytes [i..sz) to some "random" value */ > - n = iov_memset(iov, niov, 0, 0xff, -1); > + n = iov_memset(iov, niov, 0, 0xff, sz); This one is not needed? > g_assert(n == sz); > > /* next copy bytes [i..sz) from ibuf to iovec */ > - n = iov_from_buf(iov, niov, i, ibuf + i, -1); > + n = iov_from_buf(iov, niov, i, ibuf + i, sz - i); > g_assert(n == sz - i); > > /* clear part of obuf */ > memset(obuf + i, 0, sz - i); > /* and set this part of obuf to values from iovec */ > - n = iov_to_buf(iov, niov, i, obuf + i, -1); > + n = iov_to_buf(iov, niov, i, obuf + i, sz - i); > g_assert(n == sz - i); > > /* now compare resulting buffers */ > @@ -109,7 +109,7 @@ static void test_to_from_buf_1(void) > * with j in [i..sz]. */ > > /* clear iovec */ > - n = iov_memset(iov, niov, 0, 0xff, -1); > + n = iov_memset(iov, niov, 0, 0xff, sz); This one as well? Actually I think we can keep the two places above, but there seems to be a 3rd one below which is untouched. If we do change the two, maybe we'd better change the 3rd one as well. Besides: Reviewed-by: Peter Xu <peterx@redhat.com> > g_assert(n == sz); > > /* copy bytes [i..j) from ibuf to iovec */ > -- > 2.13.5 >
Peter Xu <peterx@redhat.com> wrote: > On Wed, Aug 23, 2017 at 10:39:00AM +0200, Juan Quintela wrote: >> We were using -1 instead of the real size because the functions check >> what is bigger, size in bytes or the size of the iov. Recent gcc's >> barf at this. >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> --- >> tests/test-iov.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/tests/test-iov.c b/tests/test-iov.c >> index a22d71fd2c..819c410a51 100644 >> --- a/tests/test-iov.c >> +++ b/tests/test-iov.c >> @@ -81,17 +81,17 @@ static void test_to_from_buf_1(void) >> * skip whole vector and process exactly 0 bytes */ >> >> /* first set bytes [i..sz) to some "random" value */ >> - n = iov_memset(iov, niov, 0, 0xff, -1); >> + n = iov_memset(iov, niov, 0, 0xff, sz); > > This one is not needed? Not, but it is for consistence. iov_memset() has that property, that it memset whatever is smaller, bytes parameter or iov size. >> * with j in [i..sz]. */ >> >> /* clear iovec */ >> - n = iov_memset(iov, niov, 0, 0xff, -1); >> + n = iov_memset(iov, niov, 0, 0xff, sz); > > This one as well? I decided to change all of them for consistence. Using -1 is a trick that just happens to work for current implementation, but it is a hack. and we have just after that an assert with the real size that we want to copy. Just use them everywhere. > > Actually I think we can keep the two places above, but there seems to > be a 3rd one below which is untouched. If we do change the two, maybe > we'd better change the 3rd one as well. Opps, didn't saw that other. With this changes it fixed all for me. > Besides: > > Reviewed-by: Peter Xu <peterx@redhat.com> Thanks, Juan. > >> g_assert(n == sz); >> >> /* copy bytes [i..j) from ibuf to iovec */ >> -- >> 2.13.5 >>
On 23.08.2017 10:39, Juan Quintela wrote: > We were using -1 instead of the real size because the functions check > what is bigger, size in bytes or the size of the iov. Recent gcc's > barf at this. > > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > tests/test-iov.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) While you're at it, could you maybe also adjust the comments in include/qemu/iov.h ? It currently says: * It is okay to use very large value for `bytes' since we're * limited by the size of the iovec anyway, provided that the * buffer pointed to by buf has enough space. One possible * such "large" value is -1 (sinice size_t is unsigned), * so specifying `-1' as `bytes' means 'up to the end of iovec'. ... and apparently -1 is not working anymore as expected. Maybe SIZE_MAX from stdint.h is a better choice? Thomas
Thomas Huth <thuth@redhat.com> wrote: > On 23.08.2017 10:39, Juan Quintela wrote: >> We were using -1 instead of the real size because the functions check >> what is bigger, size in bytes or the size of the iov. Recent gcc's >> barf at this. >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> --- >> tests/test-iov.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) > > While you're at it, could you maybe also adjust the comments in > include/qemu/iov.h ? It currently says: > > * It is okay to use very large value for `bytes' since we're > * limited by the size of the iovec anyway, provided that the > * buffer pointed to by buf has enough space. One possible > * such "large" value is -1 (sinice size_t is unsigned), > * so specifying `-1' as `bytes' means 'up to the end of iovec'. Haven't found this. Fixing. > > ... and apparently -1 is not working anymore as expected. Maybe SIZE_MAX > from stdint.h is a better choice? Compiler get confused with it, because we have _new_ values, see the warning on the cover, I *think* that in this one, the compiler is *kind* of right. (*) > Thomas Later, Juan. (*) see the warning, I think that it gets a bit confused, but generaly, we are just abusing the interface, and anyways, we are using in real code the real size, and in the test, it was easy to change everything to the real size, so I think it is better just to remove *that* feature.
Thomas Huth <thuth@redhat.com> wrote: > On 23.08.2017 10:39, Juan Quintela wrote: >> We were using -1 instead of the real size because the functions check >> what is bigger, size in bytes or the size of the iov. Recent gcc's >> barf at this. >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> --- >> tests/test-iov.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) > > While you're at it, could you maybe also adjust the comments in > include/qemu/iov.h ? It currently says: > > * It is okay to use very large value for `bytes' since we're > * limited by the size of the iovec anyway, provided that the > * buffer pointed to by buf has enough space. One possible > * such "large" value is -1 (sinice size_t is unsigned), > * so specifying `-1' as `bytes' means 'up to the end of iovec'. This is for the _full() versions, and still work. the same for iov_memset(). > ... and apparently -1 is not working anymore as expected. Maybe SIZE_MAX > from stdint.h is a better choice? static inline size_t iov_from_buf(const struct iovec *iov, unsigned int iov_cnt, size_t offset, const void *buf, size_t bytes) { if (__builtin_constant_p(bytes) && iov_cnt && offset <= iov[0].iov_len && bytes <= iov[0].iov_len - offset) { memcpy(iov[0].iov_base + offset, buf, bytes); return bytes; } else { return iov_from_buf_full(iov, iov_cnt, offset, buf, bytes); } } This optimization don't work very well if we used bytes = -1, furthermore, we return the wrong value. And no, I don't understand how the (bytes <= iov[0].iov_len - offset) test pass when bytes == -1. Later, Juan.
diff --git a/tests/test-iov.c b/tests/test-iov.c index a22d71fd2c..819c410a51 100644 --- a/tests/test-iov.c +++ b/tests/test-iov.c @@ -81,17 +81,17 @@ static void test_to_from_buf_1(void) * skip whole vector and process exactly 0 bytes */ /* first set bytes [i..sz) to some "random" value */ - n = iov_memset(iov, niov, 0, 0xff, -1); + n = iov_memset(iov, niov, 0, 0xff, sz); g_assert(n == sz); /* next copy bytes [i..sz) from ibuf to iovec */ - n = iov_from_buf(iov, niov, i, ibuf + i, -1); + n = iov_from_buf(iov, niov, i, ibuf + i, sz - i); g_assert(n == sz - i); /* clear part of obuf */ memset(obuf + i, 0, sz - i); /* and set this part of obuf to values from iovec */ - n = iov_to_buf(iov, niov, i, obuf + i, -1); + n = iov_to_buf(iov, niov, i, obuf + i, sz - i); g_assert(n == sz - i); /* now compare resulting buffers */ @@ -109,7 +109,7 @@ static void test_to_from_buf_1(void) * with j in [i..sz]. */ /* clear iovec */ - n = iov_memset(iov, niov, 0, 0xff, -1); + n = iov_memset(iov, niov, 0, 0xff, sz); g_assert(n == sz); /* copy bytes [i..j) from ibuf to iovec */
We were using -1 instead of the real size because the functions check what is bigger, size in bytes or the size of the iov. Recent gcc's barf at this. Signed-off-by: Juan Quintela <quintela@redhat.com> --- tests/test-iov.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)