diff mbox series

[v20210601,04/38] tools: add readv_exact to libxenctrl

Message ID 20210601161118.18986-5-olaf@aepfle.de (mailing list archive)
State Superseded
Headers show
Series leftover from 2020 | expand

Commit Message

Olaf Hering June 1, 2021, 4:10 p.m. UTC
Read a batch of iovec's.

In the common case of short reads, finish individual iov's with read_exact.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 tools/libs/ctrl/xc_private.c | 55 +++++++++++++++++++++++++++++++++++-
 tools/libs/ctrl/xc_private.h |  1 +
 2 files changed, 55 insertions(+), 1 deletion(-)

Comments

Jürgen Groß June 2, 2021, 6:30 a.m. UTC | #1
On 01.06.21 18:10, Olaf Hering wrote:
> Read a batch of iovec's.
> 
> In the common case of short reads, finish individual iov's with read_exact.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
>   tools/libs/ctrl/xc_private.c | 55 +++++++++++++++++++++++++++++++++++-
>   tools/libs/ctrl/xc_private.h |  1 +
>   2 files changed, 55 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libs/ctrl/xc_private.c b/tools/libs/ctrl/xc_private.c
> index d94f846686..ea420b9ba8 100644
> --- a/tools/libs/ctrl/xc_private.c
> +++ b/tools/libs/ctrl/xc_private.c
> @@ -659,8 +659,23 @@ int write_exact(int fd, const void *data, size_t size)
>   
>   #if defined(__MINIOS__)
>   /*
> - * MiniOS's libc doesn't know about writev(). Implement it as multiple 
write()s.
> + * MiniOS's libc doesn't know about readv/writev().
> + * Implement it as multiple read/write()s.
>    */
> +int readv_exact(int fd, const struct iovec *iov, int iovcnt)
> +{
> +    int rc, i;
> +
> +    for ( i = 0; i < iovcnt; ++i )
> +    {
> +        rc = read_exact(fd, iov[i].iov_base, iov[i].iov_len);
> +        if ( rc )
> +            return rc;
> +    }
> +
> +    return 0;
> +}
> +
>   int writev_exact(int fd, const struct iovec *iov, int iovcnt)
>   {
>       int rc, i;
> @@ -675,6 +690,44 @@ int writev_exact(int fd, const struct iovec *iov, int iovcnt)
>       return 0;
>   }
>   #else
> +int readv_exact(int fd, const struct iovec *iov, int iovcnt)
> +{
> +    int rc = 0, idx = 0;
> +    ssize_t len;
> +
> +    while ( idx < iovcnt )
> +    {
> +        len = readv(fd, &iov[idx], min(iovcnt - idx, IOV_MAX));
> +        if ( len == -1 && errno == EINTR )
> +            continue;
> +        if ( len <= 0 )
> +        {
> +            rc = -1;

Is EOF really an error?

> +            goto out;
> +        }
> +        while ( len > 0 && idx < iovcnt )
> +        {
> +            if ( len >= iov[idx].iov_len )
> +            {
> +                len -= iov[idx].iov_len;
> +            }
> +            else
> +            {
> +                void *p = iov[idx].iov_base + len;
> +                size_t l = iov[idx].iov_len - len;
> +
> +                rc = read_exact(fd, p, l);
> +                if ( rc )
> +                    goto out;
> +                len = 0;

This will stop the loop, even if idx hasn't reached iovcnt.

> +            }
> +            idx++;
> +        }
> +    }
> +out:
> +    return rc;
> +}
> +


Juergen
Olaf Hering June 2, 2021, 10:57 a.m. UTC | #2
Am Wed, 2 Jun 2021 08:30:08 +0200
schrieb Juergen Gross <jgross@suse.com>:

> On 01.06.21 18:10, Olaf Hering wrote:
> > +int readv_exact(int fd, const struct iovec *iov, int iovcnt)

> > +        if ( len <= 0 )
> > +        {
> > +            rc = -1;  
> Is EOF really an error?

I think yes, that's what "exact" implies IMO.


> This will stop the loop, even if idx hasn't reached iovcnt.

Yes, it will trigger yet another readv().

The "while" might be a leftover from a variant which used repeated read_exact to finish the function. It should become a "if", and the "len = 0" can be removed.

Olaf
Olaf Hering June 2, 2021, 11:05 a.m. UTC | #3
Am Wed, 2 Jun 2021 12:57:10 +0200
schrieb Olaf Hering <olaf@aepfle.de>:

> > This will stop the loop, even if idx hasn't reached iovcnt.  
> 
> Yes, it will trigger yet another readv().
> 
> The "while" might be a leftover from a variant which used repeated read_exact to finish the function. It should become a "if", and the "len = 0" can be removed.

I think the code is correct.

The while() loop checks if the last iov had a short read, and finishes that iov with read_exact.

I will add a comment, if that helps.

Olaf
Jürgen Groß June 2, 2021, 11:41 a.m. UTC | #4
On 02.06.21 12:57, Olaf Hering wrote:
> Am Wed, 2 Jun 2021 08:30:08 +0200
> schrieb Juergen Gross <jgross@suse.com>:
> 
>> On 01.06.21 18:10, Olaf Hering wrote:
>>> +int readv_exact(int fd, const struct iovec *iov, int iovcnt)
> 
>>> +        if ( len <= 0 )
>>> +        {
>>> +            rc = -1;
>> Is EOF really an error?
> 
> I think yes, that's what "exact" implies IMO.

Shouldn't you check for zero length iovec elements as in the
writev_exact() case then?

>> This will stop the loop, even if idx hasn't reached iovcnt.
> 
> Yes, it will trigger yet another readv().

Ah, right.


Juergen
Olaf Hering June 7, 2021, 9:46 a.m. UTC | #5
Am Wed, 2 Jun 2021 13:41:02 +0200
schrieb Juergen Gross <jgross@suse.com>:

> Shouldn't you check for zero length iovec elements as in the
> writev_exact() case then?

I will double check if this is a hard requirement.

Olaf
Olaf Hering June 7, 2021, 11:31 a.m. UTC | #6
Am Wed, 2 Jun 2021 13:41:02 +0200
schrieb Juergen Gross <jgross@suse.com>:

> Shouldn't you check for zero length iovec elements as in the
> writev_exact() case then?

It is not clear to me what the purpose of skipping zero-length iov's at the beginning of the iov array in writev_exact is.

I assume the syscall (or libc) will just skip them anyway. The code would not handle empty iov's in the middle of the array. Perhaps writev_exact should be done in the same way as my readv_exact, that would also remove the need for malloc. I think in the past I had a patch to do just that. Maybe just locally, maybe I sent it to this list.


In case of readv_exact, only the trailing incomplete iov will be filled with read_exact. I think the length parameter for read_exact can not become zero, "len" would already be zero. Zero-length iov's will be skipped by the loop AFAICS.


Olaf
diff mbox series

Patch

diff --git a/tools/libs/ctrl/xc_private.c b/tools/libs/ctrl/xc_private.c
index d94f846686..ea420b9ba8 100644
--- a/tools/libs/ctrl/xc_private.c
+++ b/tools/libs/ctrl/xc_private.c
@@ -659,8 +659,23 @@  int write_exact(int fd, const void *data, size_t size)
 
 #if defined(__MINIOS__)
 /*
- * MiniOS's libc doesn't know about writev(). Implement it as multiple write()s.
+ * MiniOS's libc doesn't know about readv/writev().
+ * Implement it as multiple read/write()s.
  */
+int readv_exact(int fd, const struct iovec *iov, int iovcnt)
+{
+    int rc, i;
+
+    for ( i = 0; i < iovcnt; ++i )
+    {
+        rc = read_exact(fd, iov[i].iov_base, iov[i].iov_len);
+        if ( rc )
+            return rc;
+    }
+
+    return 0;
+}
+
 int writev_exact(int fd, const struct iovec *iov, int iovcnt)
 {
     int rc, i;
@@ -675,6 +690,44 @@  int writev_exact(int fd, const struct iovec *iov, int iovcnt)
     return 0;
 }
 #else
+int readv_exact(int fd, const struct iovec *iov, int iovcnt)
+{
+    int rc = 0, idx = 0;
+    ssize_t len;
+
+    while ( idx < iovcnt )
+    {
+        len = readv(fd, &iov[idx], min(iovcnt - idx, IOV_MAX));
+        if ( len == -1 && errno == EINTR )
+            continue;
+        if ( len <= 0 )
+        {
+            rc = -1;
+            goto out;
+        }
+        while ( len > 0 && idx < iovcnt )
+        {
+            if ( len >= iov[idx].iov_len )
+            {
+                len -= iov[idx].iov_len;
+            }
+            else
+            {
+                void *p = iov[idx].iov_base + len;
+                size_t l = iov[idx].iov_len - len;
+
+                rc = read_exact(fd, p, l);
+                if ( rc )
+                    goto out;
+                len = 0;
+            }
+            idx++;
+        }
+    }
+out:
+    return rc;
+}
+
 int writev_exact(int fd, const struct iovec *iov, int iovcnt)
 {
     struct iovec *local_iov = NULL;
diff --git a/tools/libs/ctrl/xc_private.h b/tools/libs/ctrl/xc_private.h
index f0b5f83ac8..5d2c7274fb 100644
--- a/tools/libs/ctrl/xc_private.h
+++ b/tools/libs/ctrl/xc_private.h
@@ -441,6 +441,7 @@  int xc_flush_mmu_updates(xc_interface *xch, struct xc_mmu *mmu);
 
 /* Return 0 on success; -1 on error setting errno. */
 int read_exact(int fd, void *data, size_t size); /* EOF => -1, errno=0 */
+int readv_exact(int fd, const struct iovec *iov, int iovcnt);
 int write_exact(int fd, const void *data, size_t size);
 int writev_exact(int fd, const struct iovec *iov, int iovcnt);