| Message ID | 1309506172-17762-2-git-send-email-hare@suse.de |
|---|---|
| State | New |
| Headers | show |
On 01.07.2011, at 09:42, Hannes Reinecke wrote: > Occasionally, the buffer needs to be placed at a offset within > the iovec when copying the buffer to the iovec. So this is a buffer into the iovec, right? Wouldn't it make sense to also modify iov_to_buf respectively then, so the API stays similar? Also, it'd be nice to give the parameter a more obvious name, so potential users can easily recognize what it offsets. Alex > > Signed-off-by: Hannes Reinecke <hare@suse.de> > --- > hw/virtio-net.c | 2 +- > hw/virtio-serial-bus.c | 2 +- > iov.c | 23 ++++++++++++++--------- > iov.h | 2 +- > 4 files changed, 17 insertions(+), 12 deletions(-) > > diff --git a/hw/virtio-net.c b/hw/virtio-net.c > index 6997e02..a32cc01 100644 > --- a/hw/virtio-net.c > +++ b/hw/virtio-net.c > @@ -657,7 +657,7 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_ > > /* copy in packet. ugh */ > len = iov_from_buf(sg, elem.in_num, > - buf + offset, size - offset); > + buf + offset, 0, size - offset); > total += len; > offset += len; > /* If buffers can't be merged, at this point we > diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c > index 7f6db7b..53c58d0 100644 > --- a/hw/virtio-serial-bus.c > +++ b/hw/virtio-serial-bus.c > @@ -103,7 +103,7 @@ static size_t write_to_port(VirtIOSerialPort *port, > } > > len = iov_from_buf(elem.in_sg, elem.in_num, > - buf + offset, size - offset); > + buf + offset, 0, size - offset); > offset += len; > > virtqueue_push(vq, &elem, len); > diff --git a/iov.c b/iov.c > index 588cd04..9ead6ee 100644 > --- a/iov.c > +++ b/iov.c > @@ -15,21 +15,26 @@ > #include "iov.h" > > size_t iov_from_buf(struct iovec *iov, unsigned int iovcnt, > - const void *buf, size_t size) > + const void *buf, size_t offset, size_t size) > { > - size_t offset; > + size_t iov_off, buf_off; > unsigned int i; > > - offset = 0; > - for (i = 0; offset < size && i < iovcnt; i++) { > - size_t len; > + iov_off = 0; > + buf_off = 0; > + for (i = 0; i < iovcnt && size; i++) { > + if (offset < (iov_off + iov[i].iov_len)) { > + size_t len = MIN((iov_off + iov[i].iov_len) - offset, size); > > - len = MIN(iov[i].iov_len, size - offset); > + memcpy(iov[i].iov_base + (offset - iov_off), buf + buf_off, len); > > - memcpy(iov[i].iov_base, buf + offset, len); > - offset += len; > + buf_off += len; > + offset += len; > + size -= len; > + } > + iov_off += iov[i].iov_len; > } > - return offset; > + return buf_off; > } > > size_t iov_to_buf(const struct iovec *iov, const unsigned int iovcnt, > diff --git a/iov.h b/iov.h > index 60a8547..2677527 100644 > --- a/iov.h > +++ b/iov.h > @@ -13,7 +13,7 @@ > #include "qemu-common.h" > > size_t iov_from_buf(struct iovec *iov, unsigned int iovcnt, > - const void *buf, size_t size); > + const void *buf, size_t offset, size_t size); > size_t iov_to_buf(const struct iovec *iov, const unsigned int iovcnt, > void *buf, size_t offset, size_t size); > size_t iov_size(const struct iovec *iov, const unsigned int iovcnt); > -- > 1.6.0.2 >
On 07/01/2011 09:42 AM, Hannes Reinecke wrote: > size_t iov_from_buf(struct iovec *iov, unsigned int iovcnt, > - const void *buf, size_t size) > + const void *buf, size_t offset, size_t size) Wrong commit subject, it seems. :) Paolo
On 07/01/2011 10:03 AM, Paolo Bonzini wrote: > On 07/01/2011 09:42 AM, Hannes Reinecke wrote: >> size_t iov_from_buf(struct iovec *iov, unsigned int iovcnt, >> - const void *buf, size_t size) >> + const void *buf, size_t offset, size_t size) > > Wrong commit subject, it seems. :) > Bummer. Cheers, Hannes
On 07/01/2011 10:02 AM, Alexander Graf wrote: > > On 01.07.2011, at 09:42, Hannes Reinecke wrote: > >> Occasionally, the buffer needs to be placed at a offset within >> the iovec when copying the buffer to the iovec. > > So this is a buffer into the iovec, right? Wouldn't it make sense > to also modify iov_to_buf respectively then, so the API stays similar? Ahem. That's exactly what the patch does. Except from the mixed-up subject. iov_to_buff() has an offset parameter, iov_from_buf() has not. For no obvious reasons. > Also, it'd be nice to give the parameter a more obvious name, so potential > users can easily recognize what it offsets. > Yes, that sounds reasonable. What about 'iov_off' ? (And possibly rename 'iovcnt' to 'iov_cnt' for consistency ?) Cheers, Hannes
On 01.07.2011, at 10:07, Hannes Reinecke wrote: > On 07/01/2011 10:02 AM, Alexander Graf wrote: >> >> On 01.07.2011, at 09:42, Hannes Reinecke wrote: >> >>> Occasionally, the buffer needs to be placed at a offset within >>> the iovec when copying the buffer to the iovec. >> >> So this is a buffer into the iovec, right? Wouldn't it make sense > > to also modify iov_to_buf respectively then, so the API stays similar? > > Ahem. That's exactly what the patch does. Except from the mixed-up subject. > > iov_to_buff() has an offset parameter, iov_from_buf() has not. > For no obvious reasons. Ah, I see. Please state this in your patch description :). Makes it a lot easier to understand the rationale that you're merely moving the "from" API towards the same parameters as to "to" one. > >> Also, it'd be nice to give the parameter a more obvious name, so potential > > users can easily recognize what it offsets. >> > Yes, that sounds reasonable. > > What about 'iov_off' ? > (And possibly rename 'iovcnt' to 'iov_cnt' for consistency ?) Yup, that'd be a lot more readable :) Alex
diff --git a/hw/virtio-net.c b/hw/virtio-net.c index 6997e02..a32cc01 100644 --- a/hw/virtio-net.c +++ b/hw/virtio-net.c @@ -657,7 +657,7 @@ static ssize_t virtio_net_receive(VLANClientState *nc, const uint8_t *buf, size_ /* copy in packet. ugh */ len = iov_from_buf(sg, elem.in_num, - buf + offset, size - offset); + buf + offset, 0, size - offset); total += len; offset += len; /* If buffers can't be merged, at this point we diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c index 7f6db7b..53c58d0 100644 --- a/hw/virtio-serial-bus.c +++ b/hw/virtio-serial-bus.c @@ -103,7 +103,7 @@ static size_t write_to_port(VirtIOSerialPort *port, } len = iov_from_buf(elem.in_sg, elem.in_num, - buf + offset, size - offset); + buf + offset, 0, size - offset); offset += len; virtqueue_push(vq, &elem, len); diff --git a/iov.c b/iov.c index 588cd04..9ead6ee 100644 --- a/iov.c +++ b/iov.c @@ -15,21 +15,26 @@ #include "iov.h" size_t iov_from_buf(struct iovec *iov, unsigned int iovcnt, - const void *buf, size_t size) + const void *buf, size_t offset, size_t size) { - size_t offset; + size_t iov_off, buf_off; unsigned int i; - offset = 0; - for (i = 0; offset < size && i < iovcnt; i++) { - size_t len; + iov_off = 0; + buf_off = 0; + for (i = 0; i < iovcnt && size; i++) { + if (offset < (iov_off + iov[i].iov_len)) { + size_t len = MIN((iov_off + iov[i].iov_len) - offset, size); - len = MIN(iov[i].iov_len, size - offset); + memcpy(iov[i].iov_base + (offset - iov_off), buf + buf_off, len); - memcpy(iov[i].iov_base, buf + offset, len); - offset += len; + buf_off += len; + offset += len; + size -= len; + } + iov_off += iov[i].iov_len; } - return offset; + return buf_off; } size_t iov_to_buf(const struct iovec *iov, const unsigned int iovcnt, diff --git a/iov.h b/iov.h index 60a8547..2677527 100644 --- a/iov.h +++ b/iov.h @@ -13,7 +13,7 @@ #include "qemu-common.h" size_t iov_from_buf(struct iovec *iov, unsigned int iovcnt, - const void *buf, size_t size); + const void *buf, size_t offset, size_t size); size_t iov_to_buf(const struct iovec *iov, const unsigned int iovcnt, void *buf, size_t offset, size_t size); size_t iov_size(const struct iovec *iov, const unsigned int iovcnt);
Occasionally, the buffer needs to be placed at a offset within the iovec when copying the buffer to the iovec. Signed-off-by: Hannes Reinecke <hare@suse.de> --- hw/virtio-net.c | 2 +- hw/virtio-serial-bus.c | 2 +- iov.c | 23 ++++++++++++++--------- iov.h | 2 +- 4 files changed, 17 insertions(+), 12 deletions(-)