| Message ID | 1331845217-21705-5-git-send-email-mjt@msgid.tls.msk.ru |
|---|---|
| State | New |
| Headers | show |
On 03/15/2012 04:00 PM, Michael Tokarev wrote: > This patch combines two functions into one, and replaces > the implementation with already existing iov_memset() from > iov.c. > > The new prototype of qemu_iovec_memset(): > size_t qemu_iovec_memset(qiov, size_t offset, int fillc, size_t bytes) > It is different from former qemu_iovec_memset_skip(), and > I want to make other functions to be consistent with it > too: first how much to skip, second what, and 3rd how many > of it. It also returns actual number of bytes filled in, > which may be less than the requested `bytes' if qiov is > smaller than offset+bytes, in the same way iov_memset() > does. > > While at it, use utility function iov_memset() from > iov.h in posix-aio-compat.c, where qiov was used. > > Signed-off-by: Michael Tokarev<mjt@tls.msk.ru> Please CC Kevin at least when making block changes. It looks fine to me but would appreciate Kevin/Stefan taking a look too. Regards, Anthony Liguori > --- > block/qcow2.c | 4 ++-- > block/qed.c | 4 ++-- > cutils.c | 44 ++++---------------------------------------- > linux-aio.c | 4 ++-- > posix-aio-compat.c | 8 +++----- > qemu-common.h | 5 ++--- > 6 files changed, 15 insertions(+), 54 deletions(-) > > diff --git a/block/qcow2.c b/block/qcow2.c > index 7aece65..941a6a9 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -407,7 +407,7 @@ int qcow2_backing_read1(BlockDriverState *bs, QEMUIOVector *qiov, > else > n1 = bs->total_sectors - sector_num; > > - qemu_iovec_memset_skip(qiov, 0, 512 * (nb_sectors - n1), 512 * n1); > + qemu_iovec_memset(qiov, 512 * n1, 0, 512 * (nb_sectors - n1)); > > return n1; > } > @@ -467,7 +467,7 @@ static coroutine_fn int qcow2_co_readv(BlockDriverState *bs, int64_t sector_num, > } > } else { > /* Note: in this case, no need to wait */ > - qemu_iovec_memset(&hd_qiov, 0, 512 * cur_nr_sectors); > + qemu_iovec_memset(&hd_qiov, 0, 0, 512 * cur_nr_sectors); > } > } else if (cluster_offset& QCOW_OFLAG_COMPRESSED) { > /* add AIO support for compressed blocks ? */ > diff --git a/block/qed.c b/block/qed.c > index a041d31..6f9325b 100644 > --- a/block/qed.c > +++ b/block/qed.c > @@ -738,7 +738,7 @@ static void qed_read_backing_file(BDRVQEDState *s, uint64_t pos, > /* Zero all sectors if reading beyond the end of the backing file */ > if (pos>= backing_length || > pos + qiov->size> backing_length) { > - qemu_iovec_memset(qiov, 0, qiov->size); > + qemu_iovec_memset(qiov, 0, 0, qiov->size); > } > > /* Complete now if there are no backing file sectors to read */ > @@ -1253,7 +1253,7 @@ static void qed_aio_read_data(void *opaque, int ret, > > /* Handle zero cluster and backing file reads */ > if (ret == QED_CLUSTER_ZERO) { > - qemu_iovec_memset(&acb->cur_qiov, 0, acb->cur_qiov.size); > + qemu_iovec_memset(&acb->cur_qiov, 0, 0, acb->cur_qiov.size); > qed_aio_next_io(acb, 0); > return; > } else if (ret != QED_CLUSTER_FOUND) { > diff --git a/cutils.c b/cutils.c > index af308cd..0ddf4c7 100644 > --- a/cutils.c > +++ b/cutils.c > @@ -26,6 +26,7 @@ > #include<math.h> > > #include "qemu_socket.h" > +#include "iov.h" > > void pstrcpy(char *buf, int buf_size, const char *str) > { > @@ -260,47 +261,10 @@ void qemu_iovec_from_buffer(QEMUIOVector *qiov, const void *buf, size_t count) > } > } > > -void qemu_iovec_memset(QEMUIOVector *qiov, int c, size_t count) > +size_t qemu_iovec_memset(QEMUIOVector *qiov, size_t offset, > + int fillc, size_t bytes) > { > - size_t n; > - int i; > - > - for (i = 0; i< qiov->niov&& count; ++i) { > - n = MIN(count, qiov->iov[i].iov_len); > - memset(qiov->iov[i].iov_base, c, n); > - count -= n; > - } > -} > - > -void qemu_iovec_memset_skip(QEMUIOVector *qiov, int c, size_t count, > - size_t skip) > -{ > - int i; > - size_t done; > - void *iov_base; > - uint64_t iov_len; > - > - done = 0; > - for (i = 0; (i< qiov->niov)&& (done != count); i++) { > - if (skip>= qiov->iov[i].iov_len) { > - /* Skip the whole iov */ > - skip -= qiov->iov[i].iov_len; > - continue; > - } else { > - /* Skip only part (or nothing) of the iov */ > - iov_base = (uint8_t*) qiov->iov[i].iov_base + skip; > - iov_len = qiov->iov[i].iov_len - skip; > - skip = 0; > - } > - > - if (done + iov_len> count) { > - memset(iov_base, c, count - done); > - break; > - } else { > - memset(iov_base, c, iov_len); > - } > - done += iov_len; > - } > + return iov_memset(qiov->iov, qiov->niov, offset, fillc, bytes); > } > > /* > diff --git a/linux-aio.c b/linux-aio.c > index d2fc2e7..5a46c13 100644 > --- a/linux-aio.c > +++ b/linux-aio.c > @@ -64,8 +64,8 @@ static void qemu_laio_process_completion(struct qemu_laio_state *s, > } else if (ret>= 0) { > /* Short reads mean EOF, pad with zeros. */ > if (laiocb->is_read) { > - qemu_iovec_memset_skip(laiocb->qiov, 0, > - laiocb->qiov->size - ret, ret); > + qemu_iovec_memset(laiocb->qiov, ret, 0, > + laiocb->qiov->size - ret); > } else { > ret = -EINVAL; > } > diff --git a/posix-aio-compat.c b/posix-aio-compat.c > index d311d13..1ab4a18 100644 > --- a/posix-aio-compat.c > +++ b/posix-aio-compat.c > @@ -29,6 +29,7 @@ > #include "qemu-common.h" > #include "trace.h" > #include "block_int.h" > +#include "iov.h" > > #include "block/raw-posix-aio.h" > > @@ -351,11 +352,8 @@ static void *aio_thread(void *unused) > if (ret>= 0&& ret< aiocb->aio_nbytes&& aiocb->common.bs->growable) { > /* A short read means that we have reached EOF. Pad the buffer > * with zeros for bytes after EOF. */ > - QEMUIOVector qiov; > - > - qemu_iovec_init_external(&qiov, aiocb->aio_iov, > - aiocb->aio_niov); > - qemu_iovec_memset_skip(&qiov, 0, aiocb->aio_nbytes - ret, ret); > + iov_memset(aiocb->aio_iov, aiocb->aio_niov, ret, > + 0, aiocb->aio_nbytes - ret); > > ret = aiocb->aio_nbytes; > } > diff --git a/qemu-common.h b/qemu-common.h > index b0fdf5c..3c556c8 100644 > --- a/qemu-common.h > +++ b/qemu-common.h > @@ -344,9 +344,8 @@ void qemu_iovec_destroy(QEMUIOVector *qiov); > void qemu_iovec_reset(QEMUIOVector *qiov); > void qemu_iovec_to_buffer(QEMUIOVector *qiov, void *buf); > void qemu_iovec_from_buffer(QEMUIOVector *qiov, const void *buf, size_t count); > -void qemu_iovec_memset(QEMUIOVector *qiov, int c, size_t count); > -void qemu_iovec_memset_skip(QEMUIOVector *qiov, int c, size_t count, > - size_t skip); > +size_t qemu_iovec_memset(QEMUIOVector *qiov, size_t offset, > + int fillc, size_t bytes); > > bool buffer_is_zero(const void *buf, size_t len); >
On Fri, Mar 16, 2012 at 11:19:03AM -0500, Anthony Liguori wrote: > On 03/15/2012 04:00 PM, Michael Tokarev wrote: > >This patch combines two functions into one, and replaces > >the implementation with already existing iov_memset() from > >iov.c. > > > >The new prototype of qemu_iovec_memset(): > > size_t qemu_iovec_memset(qiov, size_t offset, int fillc, size_t bytes) > >It is different from former qemu_iovec_memset_skip(), and > >I want to make other functions to be consistent with it > >too: first how much to skip, second what, and 3rd how many > >of it. It also returns actual number of bytes filled in, > >which may be less than the requested `bytes' if qiov is > >smaller than offset+bytes, in the same way iov_memset() > >does. > > > >While at it, use utility function iov_memset() from > >iov.h in posix-aio-compat.c, where qiov was used. > > > >Signed-off-by: Michael Tokarev<mjt@tls.msk.ru> > > Please CC Kevin at least when making block changes. > > It looks fine to me but would appreciate Kevin/Stefan taking a look too. I am behind and feel that refactorings like this require careful technical review but don't buy us much. The best way to get refactoring in is by making it part of a larger series that fixes a bug or adds a feature. I don't have bandwidth for non-trivial cosmetic stuff at the moment, sorry. Stefan
On 19.03.2012 18:36, Stefan Hajnoczi wrote: > On Fri, Mar 16, 2012 at 11:19:03AM -0500, Anthony Liguori wrote: >> On 03/15/2012 04:00 PM, Michael Tokarev wrote: >>> This patch combines two functions into one, and replaces >>> the implementation with already existing iov_memset() from >>> iov.c. >>> >>> The new prototype of qemu_iovec_memset(): >>> size_t qemu_iovec_memset(qiov, size_t offset, int fillc, size_t bytes) >>> It is different from former qemu_iovec_memset_skip(), and >>> I want to make other functions to be consistent with it >>> too: first how much to skip, second what, and 3rd how many >>> of it. It also returns actual number of bytes filled in, >>> which may be less than the requested `bytes' if qiov is >>> smaller than offset+bytes, in the same way iov_memset() >>> does. >>> >>> While at it, use utility function iov_memset() from >>> iov.h in posix-aio-compat.c, where qiov was used. >>> >>> Signed-off-by: Michael Tokarev<mjt@tls.msk.ru> >> >> Please CC Kevin at least when making block changes. >> >> It looks fine to me but would appreciate Kevin/Stefan taking a look too. > > I am behind and feel that refactorings like this require careful > technical review but don't buy us much. I described what it gives in the cover message. We've several interfaces around the same thing, and even several implementations of iov* functions doing the same but with different argument order. When the interface is wrong (and in this case it was wrong in argument order), it makes new code using it more error-prone. Note that whole thing I'm touching is used in some 10 places only. Note that I provided a test program for all these functions too. Note also that you were CC'ed only for the patches which touches block layer, for a review for the changes in there, which is 2 one-liner changes. > The best way to get refactoring > in is by making it part of a larger series that fixes a bug or adds a > feature. And for these, you'll tell that the changes should be in a separate series, > I don't have bandwidth for non-trivial cosmetic stuff at the > moment, sorry. What's "bandwidth" in this context? Initially I thought that just making 2 or 3 functions which were inconsistent with each other to be a very easy task. But the patchset grew to 11 patches and 5 versions, because pbonzini said it is insufficient. Now you're saying it is too much. I spent *much* more than any sane amount of time on this, rediffing and rewriting, on a *trivial* thing, and now what? If you can't plan even the most simple and low-level interface to be consistent, please at least let someone who is STILL willing to make it consistent to do that. It is not cosmetic. And if the code will grow this way further (and it does!), it will be a very big ball of mud, unmaintainable. Thank you? /mjt
On 03/19/2012 03:04 PM, Michael Tokarev wrote: > On 19.03.2012 18:36, Stefan Hajnoczi wrote: >> On Fri, Mar 16, 2012 at 11:19:03AM -0500, Anthony Liguori wrote: >>> On 03/15/2012 04:00 PM, Michael Tokarev wrote: >>>> This patch combines two functions into one, and replaces >>>> the implementation with already existing iov_memset() from >>>> iov.c. >>>> >>>> The new prototype of qemu_iovec_memset(): >>>> size_t qemu_iovec_memset(qiov, size_t offset, int fillc, size_t bytes) >>>> It is different from former qemu_iovec_memset_skip(), and >>>> I want to make other functions to be consistent with it >>>> too: first how much to skip, second what, and 3rd how many >>>> of it. It also returns actual number of bytes filled in, >>>> which may be less than the requested `bytes' if qiov is >>>> smaller than offset+bytes, in the same way iov_memset() >>>> does. >>>> >>>> While at it, use utility function iov_memset() from >>>> iov.h in posix-aio-compat.c, where qiov was used. >>>> >>>> Signed-off-by: Michael Tokarev<mjt@tls.msk.ru> >>> >>> Please CC Kevin at least when making block changes. >>> >>> It looks fine to me but would appreciate Kevin/Stefan taking a look too. >> >> I am behind and feel that refactorings like this require careful >> technical review but don't buy us much. > > I described what it gives in the cover message. We've > several interfaces around the same thing, and even several > implementations of iov* functions doing the same but with > different argument order. When the interface is wrong > (and in this case it was wrong in argument order), it > makes new code using it more error-prone. Note that > whole thing I'm touching is used in some 10 places only. > > Note that I provided a test program for all these functions > too. Yeah, and I looked at it, and I believe Paolo gave some feedback which I fully agreed with (although I can't find the mail now). I was expecting you to send another series with the test case included (and integrated into make check). > > Note also that you were CC'ed only for the patches which > touches block layer, for a review for the changes in there, > which is 2 one-liner changes. > >> The best way to get refactoring >> in is by making it part of a larger series that fixes a bug or adds a >> feature. > > And for these, you'll tell that the changes should be in > a separate series, > >> I don't have bandwidth for non-trivial cosmetic stuff at the >> moment, sorry. > > What's "bandwidth" in this context? I think Stefan is just pointing out that given his limited amount of time, he doesn't think the change is worth reviewing in detail. I don't think he means to be offensive, just honest and open which is a good thing. But since I have a vested interest in getting more test cases written, as I mentioned in a previous note, I'll review this series thoroughly and merge it when it comes with a test case, so please resubmit. Regards, Anthony Liguori
On Tue, Mar 20, 2012 at 12:04:36AM +0400, Michael Tokarev wrote: > > I don't have bandwidth for non-trivial cosmetic stuff at the > > moment, sorry. > > What's "bandwidth" in this context? Time. My review queue is 120 patches at the moment. I'm tackling those which are blocked on me and which fix bugs/add features first. > Initially I thought that just making 2 or 3 functions which > were inconsistent with each other to be a very easy task. > But the patchset grew to 11 patches and 5 versions, because > pbonzini said it is insufficient. Now you're saying it is > too much. > > I spent *much* more than any sane amount of time on this, > rediffing and rewriting, on a *trivial* thing, and now what? > > If you can't plan even the most simple and low-level interface > to be consistent, please at least let someone who is STILL > willing to make it consistent to do that. It is not cosmetic. > And if the code will grow this way further (and it does!), > it will be a very big ball of mud, unmaintainable. I'm not blocking this series. If others are happy with this series then please merge. Stefan
diff --git a/block/qcow2.c b/block/qcow2.c index 7aece65..941a6a9 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -407,7 +407,7 @@ int qcow2_backing_read1(BlockDriverState *bs, QEMUIOVector *qiov, else n1 = bs->total_sectors - sector_num; - qemu_iovec_memset_skip(qiov, 0, 512 * (nb_sectors - n1), 512 * n1); + qemu_iovec_memset(qiov, 512 * n1, 0, 512 * (nb_sectors - n1)); return n1; } @@ -467,7 +467,7 @@ static coroutine_fn int qcow2_co_readv(BlockDriverState *bs, int64_t sector_num, } } else { /* Note: in this case, no need to wait */ - qemu_iovec_memset(&hd_qiov, 0, 512 * cur_nr_sectors); + qemu_iovec_memset(&hd_qiov, 0, 0, 512 * cur_nr_sectors); } } else if (cluster_offset & QCOW_OFLAG_COMPRESSED) { /* add AIO support for compressed blocks ? */ diff --git a/block/qed.c b/block/qed.c index a041d31..6f9325b 100644 --- a/block/qed.c +++ b/block/qed.c @@ -738,7 +738,7 @@ static void qed_read_backing_file(BDRVQEDState *s, uint64_t pos, /* Zero all sectors if reading beyond the end of the backing file */ if (pos >= backing_length || pos + qiov->size > backing_length) { - qemu_iovec_memset(qiov, 0, qiov->size); + qemu_iovec_memset(qiov, 0, 0, qiov->size); } /* Complete now if there are no backing file sectors to read */ @@ -1253,7 +1253,7 @@ static void qed_aio_read_data(void *opaque, int ret, /* Handle zero cluster and backing file reads */ if (ret == QED_CLUSTER_ZERO) { - qemu_iovec_memset(&acb->cur_qiov, 0, acb->cur_qiov.size); + qemu_iovec_memset(&acb->cur_qiov, 0, 0, acb->cur_qiov.size); qed_aio_next_io(acb, 0); return; } else if (ret != QED_CLUSTER_FOUND) { diff --git a/cutils.c b/cutils.c index af308cd..0ddf4c7 100644 --- a/cutils.c +++ b/cutils.c @@ -26,6 +26,7 @@ #include <math.h> #include "qemu_socket.h" +#include "iov.h" void pstrcpy(char *buf, int buf_size, const char *str) { @@ -260,47 +261,10 @@ void qemu_iovec_from_buffer(QEMUIOVector *qiov, const void *buf, size_t count) } } -void qemu_iovec_memset(QEMUIOVector *qiov, int c, size_t count) +size_t qemu_iovec_memset(QEMUIOVector *qiov, size_t offset, + int fillc, size_t bytes) { - size_t n; - int i; - - for (i = 0; i < qiov->niov && count; ++i) { - n = MIN(count, qiov->iov[i].iov_len); - memset(qiov->iov[i].iov_base, c, n); - count -= n; - } -} - -void qemu_iovec_memset_skip(QEMUIOVector *qiov, int c, size_t count, - size_t skip) -{ - int i; - size_t done; - void *iov_base; - uint64_t iov_len; - - done = 0; - for (i = 0; (i < qiov->niov) && (done != count); i++) { - if (skip >= qiov->iov[i].iov_len) { - /* Skip the whole iov */ - skip -= qiov->iov[i].iov_len; - continue; - } else { - /* Skip only part (or nothing) of the iov */ - iov_base = (uint8_t*) qiov->iov[i].iov_base + skip; - iov_len = qiov->iov[i].iov_len - skip; - skip = 0; - } - - if (done + iov_len > count) { - memset(iov_base, c, count - done); - break; - } else { - memset(iov_base, c, iov_len); - } - done += iov_len; - } + return iov_memset(qiov->iov, qiov->niov, offset, fillc, bytes); } /* diff --git a/linux-aio.c b/linux-aio.c index d2fc2e7..5a46c13 100644 --- a/linux-aio.c +++ b/linux-aio.c @@ -64,8 +64,8 @@ static void qemu_laio_process_completion(struct qemu_laio_state *s, } else if (ret >= 0) { /* Short reads mean EOF, pad with zeros. */ if (laiocb->is_read) { - qemu_iovec_memset_skip(laiocb->qiov, 0, - laiocb->qiov->size - ret, ret); + qemu_iovec_memset(laiocb->qiov, ret, 0, + laiocb->qiov->size - ret); } else { ret = -EINVAL; } diff --git a/posix-aio-compat.c b/posix-aio-compat.c index d311d13..1ab4a18 100644 --- a/posix-aio-compat.c +++ b/posix-aio-compat.c @@ -29,6 +29,7 @@ #include "qemu-common.h" #include "trace.h" #include "block_int.h" +#include "iov.h" #include "block/raw-posix-aio.h" @@ -351,11 +352,8 @@ static void *aio_thread(void *unused) if (ret >= 0 && ret < aiocb->aio_nbytes && aiocb->common.bs->growable) { /* A short read means that we have reached EOF. Pad the buffer * with zeros for bytes after EOF. */ - QEMUIOVector qiov; - - qemu_iovec_init_external(&qiov, aiocb->aio_iov, - aiocb->aio_niov); - qemu_iovec_memset_skip(&qiov, 0, aiocb->aio_nbytes - ret, ret); + iov_memset(aiocb->aio_iov, aiocb->aio_niov, ret, + 0, aiocb->aio_nbytes - ret); ret = aiocb->aio_nbytes; } diff --git a/qemu-common.h b/qemu-common.h index b0fdf5c..3c556c8 100644 --- a/qemu-common.h +++ b/qemu-common.h @@ -344,9 +344,8 @@ void qemu_iovec_destroy(QEMUIOVector *qiov); void qemu_iovec_reset(QEMUIOVector *qiov); void qemu_iovec_to_buffer(QEMUIOVector *qiov, void *buf); void qemu_iovec_from_buffer(QEMUIOVector *qiov, const void *buf, size_t count); -void qemu_iovec_memset(QEMUIOVector *qiov, int c, size_t count); -void qemu_iovec_memset_skip(QEMUIOVector *qiov, int c, size_t count, - size_t skip); +size_t qemu_iovec_memset(QEMUIOVector *qiov, size_t offset, + int fillc, size_t bytes); bool buffer_is_zero(const void *buf, size_t len);
This patch combines two functions into one, and replaces the implementation with already existing iov_memset() from iov.c. The new prototype of qemu_iovec_memset(): size_t qemu_iovec_memset(qiov, size_t offset, int fillc, size_t bytes) It is different from former qemu_iovec_memset_skip(), and I want to make other functions to be consistent with it too: first how much to skip, second what, and 3rd how many of it. It also returns actual number of bytes filled in, which may be less than the requested `bytes' if qiov is smaller than offset+bytes, in the same way iov_memset() does. While at it, use utility function iov_memset() from iov.h in posix-aio-compat.c, where qiov was used. Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> --- block/qcow2.c | 4 ++-- block/qed.c | 4 ++-- cutils.c | 44 ++++---------------------------------------- linux-aio.c | 4 ++-- posix-aio-compat.c | 8 +++----- qemu-common.h | 5 ++--- 6 files changed, 15 insertions(+), 54 deletions(-)