Patchwork [1/3] iov: Add 'offset' parameter to iov_to_buf()

login
register
mail settings
Submitter Hannes Reinecke
Date July 1, 2011, 7:42 a.m.
Message ID <1309506172-17762-2-git-send-email-hare@suse.de>
Download mbox | patch
Permalink /patch/102879/
State New
Headers show

Comments

Hannes Reinecke - July 1, 2011, 7:42 a.m.
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(-)
Alexander Graf - July 1, 2011, 8:02 a.m.
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
>
Paolo Bonzini - July 1, 2011, 8:03 a.m.
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
Hannes Reinecke - July 1, 2011, 8:04 a.m.
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
Hannes Reinecke - July 1, 2011, 8:07 a.m.
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
Alexander Graf - July 1, 2011, 8:11 a.m.
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

Patch

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