[libvirt] [PATCH 4/6] storage: Make virStorageFileReadHeader more universal

Peter Krempa posted 6 patches 8 years, 7 months ago
[libvirt] [PATCH 4/6] storage: Make virStorageFileReadHeader more universal
Posted by Peter Krempa 8 years, 7 months ago
Allow specifying offset to read an arbitrary position in the file. This
warrants a rename to virStorageFileRead.
---
 src/qemu/qemu_driver.c                |  3 +--
 src/storage/storage_backend.h         |  9 +++++----
 src/storage/storage_backend_fs.c      | 20 +++++++++++++------
 src/storage/storage_backend_gluster.c | 37 ++++++++++++++++++++++-------------
 src/storage/storage_source.c          | 30 +++++++++++++++-------------
 src/storage/storage_source.h          |  7 ++++---
 6 files changed, 63 insertions(+), 43 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 052c7f0fc..4c4a6a036 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11584,8 +11584,7 @@ qemuStorageLimitsRefresh(virQEMUDriverPtr driver,
             goto cleanup;
         }
     } else {
-        if ((len = virStorageFileReadHeader(src, VIR_STORAGE_MAX_HEADER,
-                                            &buf)) < 0)
+        if ((len = virStorageFileRead(src, 0, VIR_STORAGE_MAX_HEADER, &buf)) < 0)
             goto cleanup;
     }

diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h
index 21ac84552..193cf134d 100644
--- a/src/storage/storage_backend.h
+++ b/src/storage/storage_backend.h
@@ -154,9 +154,10 @@ typedef int
                              struct stat *st);

 typedef ssize_t
-(*virStorageFileBackendReadHeader)(virStorageSourcePtr src,
-                                   ssize_t max_len,
-                                   char **buf);
+(*virStorageFileBackendRead)(virStorageSourcePtr src,
+                             size_t offset,
+                             size_t len,
+                             char **buf);

 typedef const char *
 (*virStorageFileBackendGetUniqueIdentifier)(virStorageSourcePtr src);
@@ -186,7 +187,7 @@ struct _virStorageFileBackend {
      * error on failure. */
     virStorageFileBackendInit backendInit;
     virStorageFileBackendDeinit backendDeinit;
-    virStorageFileBackendReadHeader storageFileReadHeader;
+    virStorageFileBackendRead storageFileRead;
     virStorageFileBackendGetUniqueIdentifier storageFileGetUniqueIdentifier;

     /* The following group of callbacks is expected to set errno
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index bf1d7de43..847dbc9e0 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -779,9 +779,10 @@ virStorageFileBackendFileStat(virStorageSourcePtr src,


 static ssize_t
-virStorageFileBackendFileReadHeader(virStorageSourcePtr src,
-                                    ssize_t max_len,
-                                    char **buf)
+virStorageFileBackendFileRead(virStorageSourcePtr src,
+                              size_t offset,
+                              size_t len,
+                              char **buf)
 {
     int fd = -1;
     ssize_t ret = -1;
@@ -793,7 +794,14 @@ virStorageFileBackendFileReadHeader(virStorageSourcePtr src,
         return -1;
     }

-    if ((ret = virFileReadHeaderFD(fd, max_len, buf)) < 0) {
+    if (offset > 0) {
+        if (lseek(fd, offset, SEEK_SET) == (off_t) -1) {
+            virReportSystemError(errno, _("cannot seek into '%s'"), src->path);
+            goto cleanup;
+        }
+    }
+
+    if ((ret = virFileReadHeaderFD(fd, len, buf)) < 0) {
         virReportSystemError(errno,
                              _("cannot read header '%s'"), src->path);
         goto cleanup;
@@ -850,7 +858,7 @@ virStorageFileBackend virStorageFileBackendFile = {
     .storageFileCreate = virStorageFileBackendFileCreate,
     .storageFileUnlink = virStorageFileBackendFileUnlink,
     .storageFileStat = virStorageFileBackendFileStat,
-    .storageFileReadHeader = virStorageFileBackendFileReadHeader,
+    .storageFileRead = virStorageFileBackendFileRead,
     .storageFileAccess = virStorageFileBackendFileAccess,
     .storageFileChown = virStorageFileBackendFileChown,

@@ -865,7 +873,7 @@ virStorageFileBackend virStorageFileBackendBlock = {
     .backendDeinit = virStorageFileBackendFileDeinit,

     .storageFileStat = virStorageFileBackendFileStat,
-    .storageFileReadHeader = virStorageFileBackendFileReadHeader,
+    .storageFileRead = virStorageFileBackendFileRead,
     .storageFileAccess = virStorageFileBackendFileAccess,
     .storageFileChown = virStorageFileBackendFileChown,

diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c
index 93dce4042..8ea7e603c 100644
--- a/src/storage/storage_backend_gluster.c
+++ b/src/storage/storage_backend_gluster.c
@@ -151,20 +151,20 @@ virStorageBackendGlusterOpen(virStoragePoolObjPtr pool)


 static ssize_t
-virStorageBackendGlusterReadHeader(glfs_fd_t *fd,
-                                   const char *name,
-                                   ssize_t maxlen,
-                                   char **buf)
+virStorageBackendGlusterRead(glfs_fd_t *fd,
+                             const char *name,
+                             size_t len,
+                             char **buf)
 {
     char *s;
     size_t nread = 0;

-    if (VIR_ALLOC_N(*buf, maxlen) < 0)
+    if (VIR_ALLOC_N(*buf, len) < 0)
         return -1;

     s = *buf;
-    while (maxlen) {
-        ssize_t r = glfs_read(fd, s, maxlen, 0);
+    while (len) {
+        ssize_t r = glfs_read(fd, s, len, 0);
         if (r < 0 && errno == EINTR)
             continue;
         if (r < 0) {
@@ -175,7 +175,7 @@ virStorageBackendGlusterReadHeader(glfs_fd_t *fd,
         if (r == 0)
             return nread;
         s += r;
-        maxlen -= r;
+        len -= r;
         nread += r;
     }
     return nread;
@@ -292,7 +292,7 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state,
         goto cleanup;
     }

-    if ((len = virStorageBackendGlusterReadHeader(fd, name, len, &header)) < 0)
+    if ((len = virStorageBackendGlusterRead(fd, name, len, &header)) < 0)
         goto cleanup;

     if (!(meta = virStorageFileGetMetadataFromBuf(name, header, len,
@@ -721,9 +721,10 @@ virStorageFileBackendGlusterStat(virStorageSourcePtr src,


 static ssize_t
-virStorageFileBackendGlusterReadHeader(virStorageSourcePtr src,
-                                       ssize_t max_len,
-                                       char **buf)
+virStorageFileBackendGlusterRead(virStorageSourcePtr src,
+                                 size_t offset,
+                                 size_t len,
+                                 char **buf)
 {
     virStorageFileBackendGlusterPrivPtr priv = src->drv->priv;
     glfs_fd_t *fd = NULL;
@@ -737,8 +738,16 @@ virStorageFileBackendGlusterReadHeader(virStorageSourcePtr src,
         return -1;
     }

-    ret = virStorageBackendGlusterReadHeader(fd, src->path, max_len, buf);
+    if (offset > 0) {
+        if (glfs_lseek(fd, offset, SEEK_SET) == (off_t) -1) {
+            virReportSystemError(errno, _("cannot seek into '%s'"), src->path);
+            goto cleanup;
+        }
+    }
+
+    ret = virStorageBackendGlusterRead(fd, src->path, len, buf);

+ cleanup:
     if (fd)
         glfs_close(fd);

@@ -851,7 +860,7 @@ virStorageFileBackend virStorageFileBackendGluster = {
     .storageFileCreate = virStorageFileBackendGlusterCreate,
     .storageFileUnlink = virStorageFileBackendGlusterUnlink,
     .storageFileStat = virStorageFileBackendGlusterStat,
-    .storageFileReadHeader = virStorageFileBackendGlusterReadHeader,
+    .storageFileRead = virStorageFileBackendGlusterRead,
     .storageFileAccess = virStorageFileBackendGlusterAccess,
     .storageFileChown = virStorageFileBackendGlusterChown,

diff --git a/src/storage/storage_source.c b/src/storage/storage_source.c
index c91d629c8..130c2d2f3 100644
--- a/src/storage/storage_source.c
+++ b/src/storage/storage_source.c
@@ -64,7 +64,7 @@ virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src)
     }

     return backend->storageFileGetUniqueIdentifier &&
-           backend->storageFileReadHeader &&
+           backend->storageFileRead &&
            backend->storageFileAccess;
 }

@@ -263,10 +263,11 @@ virStorageFileStat(virStorageSourcePtr src,


 /**
- * virStorageFileReadHeader: read the beginning bytes of a file into a buffer
+ * virStorageFileRead: read the beginning bytes of a file into a buffer
  *
  * @src: file structure pointing to the file
- * @max_len: maximum number of bytes read from the storage file
+ * @offset: number of bytes to skip in the storage file
+ * @len: maximum number of bytes read from the storage file
  * @buf: buffer to read the data into. buffer shall be freed by caller)
  *
  * Returns the count of bytes read on success and -1 on failure, -2 if the
@@ -274,9 +275,10 @@ virStorageFileStat(virStorageSourcePtr src,
  * Libvirt error is reported on failure.
  */
 ssize_t
-virStorageFileReadHeader(virStorageSourcePtr src,
-                         ssize_t max_len,
-                         char **buf)
+virStorageFileRead(virStorageSourcePtr src,
+                   size_t offset,
+                   size_t len,
+                   char **buf)
 {
     ssize_t ret;

@@ -286,18 +288,18 @@ virStorageFileReadHeader(virStorageSourcePtr src,
         return -1;
     }

-    if (!src->drv->backend->storageFileReadHeader) {
+    if (!src->drv->backend->storageFileRead) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("storage file header reading is not supported for "
+                       _("storage file reading is not supported for "
                          "storage type %s (protocol: %s)"),
                        virStorageTypeToString(src->type),
                        virStorageNetProtocolTypeToString(src->protocol));
         return -2;
     }

-    ret = src->drv->backend->storageFileReadHeader(src, max_len, buf);
+    ret = src->drv->backend->storageFileRead(src, offset, len, buf);

-    VIR_DEBUG("read of storage header %p: ret=%zd", src, ret);
+    VIR_DEBUG("read of storage %p: ret=%zd", src, ret);

     return ret;
 }
@@ -444,8 +446,8 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
     if (virHashAddEntry(cycle, uniqueName, (void *)1) < 0)
         goto cleanup;

-    if ((headerLen = virStorageFileReadHeader(src, VIR_STORAGE_MAX_HEADER,
-                                              &buf)) < 0)
+    if ((headerLen = virStorageFileRead(src, 0, VIR_STORAGE_MAX_HEADER,
+                                        &buf)) < 0)
         goto cleanup;

     if (virStorageFileGetMetadataInternal(src, buf, headerLen,
@@ -565,8 +567,8 @@ virStorageFileGetBackingStoreStr(virStorageSourcePtr src)
     if (virStorageFileAccess(src, F_OK) < 0)
         return NULL;

-    if ((headerLen = virStorageFileReadHeader(src, VIR_STORAGE_MAX_HEADER,
-                                              &buf)) < 0)
+    if ((headerLen = virStorageFileRead(src, 0, VIR_STORAGE_MAX_HEADER,
+                                        &buf)) < 0)
         return NULL;

     if (!(tmp = virStorageSourceCopy(src, false)))
diff --git a/src/storage/storage_source.h b/src/storage/storage_source.h
index 6b3362244..6462baf6a 100644
--- a/src/storage/storage_source.h
+++ b/src/storage/storage_source.h
@@ -32,9 +32,10 @@ int virStorageFileCreate(virStorageSourcePtr src);
 int virStorageFileUnlink(virStorageSourcePtr src);
 int virStorageFileStat(virStorageSourcePtr src,
                        struct stat *stat);
-ssize_t virStorageFileReadHeader(virStorageSourcePtr src,
-                                 ssize_t max_len,
-                                 char **buf);
+ssize_t virStorageFileRead(virStorageSourcePtr src,
+                           size_t offset,
+                           size_t len,
+                           char **buf);
 const char *virStorageFileGetUniqueIdentifier(virStorageSourcePtr src);
 int virStorageFileAccess(virStorageSourcePtr src, int mode);
 int virStorageFileChown(const virStorageSource *src, uid_t uid, gid_t gid);
-- 
2.12.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/6] storage: Make virStorageFileReadHeader more universal
Posted by John Ferlan 8 years, 7 months ago

On 06/23/2017 09:33 AM, Peter Krempa wrote:
> Allow specifying offset to read an arbitrary position in the file. This
> warrants a rename to virStorageFileRead.
> ---
>  src/qemu/qemu_driver.c                |  3 +--
>  src/storage/storage_backend.h         |  9 +++++----
>  src/storage/storage_backend_fs.c      | 20 +++++++++++++------
>  src/storage/storage_backend_gluster.c | 37 ++++++++++++++++++++++-------------
>  src/storage/storage_source.c          | 30 +++++++++++++++-------------
>  src/storage/storage_source.h          |  7 ++++---
>  6 files changed, 63 insertions(+), 43 deletions(-)
> 

[...]

> diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c
> index 93dce4042..8ea7e603c 100644
> --- a/src/storage/storage_backend_gluster.c
> +++ b/src/storage/storage_backend_gluster.c
> @@ -151,20 +151,20 @@ virStorageBackendGlusterOpen(virStoragePoolObjPtr pool)
> 
> 
>  static ssize_t
> -virStorageBackendGlusterReadHeader(glfs_fd_t *fd,
> -                                   const char *name,
> -                                   ssize_t maxlen,
> -                                   char **buf)
> +virStorageBackendGlusterRead(glfs_fd_t *fd,
> +                             const char *name,
> +                             size_t len,
> +                             char **buf)
>  {
>      char *s;
>      size_t nread = 0;
> 
> -    if (VIR_ALLOC_N(*buf, maxlen) < 0)
> +    if (VIR_ALLOC_N(*buf, len) < 0)
>          return -1;
> 
>      s = *buf;
> -    while (maxlen) {
> -        ssize_t r = glfs_read(fd, s, maxlen, 0);
> +    while (len) {
> +        ssize_t r = glfs_read(fd, s, len, 0);
>          if (r < 0 && errno == EINTR)
>              continue;
>          if (r < 0) {
> @@ -175,7 +175,7 @@ virStorageBackendGlusterReadHeader(glfs_fd_t *fd,
>          if (r == 0)
>              return nread;
>          s += r;
> -        maxlen -= r;
> +        len -= r;
>          nread += r;
>      }
>      return nread;
> @@ -292,7 +292,7 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state,
>          goto cleanup;
>      }
> 

Should :

    ssize_t len = VIR_STORAGE_MAX_HEADER;

change to size_t and a new variable "ssize_t read_len" be created and
used for the following and subsequent virStorageFileGetMetadataFromBuf
call?  (although that also takes a size_t for the 3rd param).

> -    if ((len = virStorageBackendGlusterReadHeader(fd, name, len, &header)) < 0)
> +    if ((len = virStorageBackendGlusterRead(fd, name, len, &header)) < 0)
>          goto cleanup;
> 
>      if (!(meta = virStorageFileGetMetadataFromBuf(name, header, len,
> @@ -721,9 +721,10 @@ virStorageFileBackendGlusterStat(virStorageSourcePtr src,
> 
> 
>  static ssize_t
> -virStorageFileBackendGlusterReadHeader(virStorageSourcePtr src,
> -                                       ssize_t max_len,
> -                                       char **buf)
> +virStorageFileBackendGlusterRead(virStorageSourcePtr src,
> +                                 size_t offset,
> +                                 size_t len,
> +                                 char **buf)
>  {
>      virStorageFileBackendGlusterPrivPtr priv = src->drv->priv;
>      glfs_fd_t *fd = NULL;
> @@ -737,8 +738,16 @@ virStorageFileBackendGlusterReadHeader(virStorageSourcePtr src,
>          return -1;
>      }
> 
> -    ret = virStorageBackendGlusterReadHeader(fd, src->path, max_len, buf);
> +    if (offset > 0) {
> +        if (glfs_lseek(fd, offset, SEEK_SET) == (off_t) -1) {
> +            virReportSystemError(errno, _("cannot seek into '%s'"), src->path);
> +            goto cleanup;
> +        }
> +    }
> +
> +    ret = virStorageBackendGlusterRead(fd, src->path, len, buf);
> 
> + cleanup:
>      if (fd)
>          glfs_close(fd);
> 
> @@ -851,7 +860,7 @@ virStorageFileBackend virStorageFileBackendGluster = {
>      .storageFileCreate = virStorageFileBackendGlusterCreate,
>      .storageFileUnlink = virStorageFileBackendGlusterUnlink,
>      .storageFileStat = virStorageFileBackendGlusterStat,
> -    .storageFileReadHeader = virStorageFileBackendGlusterReadHeader,
> +    .storageFileRead = virStorageFileBackendGlusterRead,
>      .storageFileAccess = virStorageFileBackendGlusterAccess,
>      .storageFileChown = virStorageFileBackendGlusterChown,
> 
> diff --git a/src/storage/storage_source.c b/src/storage/storage_source.c
> index c91d629c8..130c2d2f3 100644
> --- a/src/storage/storage_source.c
> +++ b/src/storage/storage_source.c
> @@ -64,7 +64,7 @@ virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src)
>      }
> 
>      return backend->storageFileGetUniqueIdentifier &&
> -           backend->storageFileReadHeader &&
> +           backend->storageFileRead &&
>             backend->storageFileAccess;
>  }
> 
> @@ -263,10 +263,11 @@ virStorageFileStat(virStorageSourcePtr src,
> 
> 
>  /**
> - * virStorageFileReadHeader: read the beginning bytes of a file into a buffer
> + * virStorageFileRead: read the beginning bytes of a file into a buffer

Probably should put the description below the args and restate since it
reading @len bytes of data starting at @offset

>   *
>   * @src: file structure pointing to the file
> - * @max_len: maximum number of bytes read from the storage file
> + * @offset: number of bytes to skip in the storage file
> + * @len: maximum number of bytes read from the storage file
>   * @buf: buffer to read the data into. buffer shall be freed by caller)
>   *
>   * Returns the count of bytes read on success and -1 on failure, -2 if the
> @@ -274,9 +275,10 @@ virStorageFileStat(virStorageSourcePtr src,
>   * Libvirt error is reported on failure.
>   */
>  ssize_t
> -virStorageFileReadHeader(virStorageSourcePtr src,
> -                         ssize_t max_len,
> -                         char **buf)
> +virStorageFileRead(virStorageSourcePtr src,
> +                   size_t offset,
> +                   size_t len,
> +                   char **buf)
>  {
>      ssize_t ret;
> 
> @@ -286,18 +288,18 @@ virStorageFileReadHeader(virStorageSourcePtr src,
>          return -1;
>      }
> 
> -    if (!src->drv->backend->storageFileReadHeader) {
> +    if (!src->drv->backend->storageFileRead) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("storage file header reading is not supported for "
> +                       _("storage file reading is not supported for "
>                           "storage type %s (protocol: %s)"),
>                         virStorageTypeToString(src->type),
>                         virStorageNetProtocolTypeToString(src->protocol));
>          return -2;
>      }
> 
> -    ret = src->drv->backend->storageFileReadHeader(src, max_len, buf);
> +    ret = src->drv->backend->storageFileRead(src, offset, len, buf);
> 
> -    VIR_DEBUG("read of storage header %p: ret=%zd", src, ret);
> +    VIR_DEBUG("read of storage %p: ret=%zd", src, ret);

"read %zd bytes from storage %p starting at offset %zu", ret, src, offset

With a few tweaks:

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

[...]

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/6] storage: Make virStorageFileReadHeader more universal
Posted by Peter Krempa 8 years, 7 months ago
On Sat, Jul 01, 2017 at 10:43:09 -0400, John Ferlan wrote:
> 
> 
> On 06/23/2017 09:33 AM, Peter Krempa wrote:
> > Allow specifying offset to read an arbitrary position in the file. This
> > warrants a rename to virStorageFileRead.
> > ---
> >  src/qemu/qemu_driver.c                |  3 +--
> >  src/storage/storage_backend.h         |  9 +++++----
> >  src/storage/storage_backend_fs.c      | 20 +++++++++++++------
> >  src/storage/storage_backend_gluster.c | 37 ++++++++++++++++++++++-------------
> >  src/storage/storage_source.c          | 30 +++++++++++++++-------------
> >  src/storage/storage_source.h          |  7 ++++---
> >  6 files changed, 63 insertions(+), 43 deletions(-)
> > 
> 
> [...]
> 
> > diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c
> > index 93dce4042..8ea7e603c 100644
> > --- a/src/storage/storage_backend_gluster.c
> > +++ b/src/storage/storage_backend_gluster.c

[...]

> > @@ -292,7 +292,7 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state,
> >          goto cleanup;
> >      }
> > 
> 
> Should :
> 
>     ssize_t len = VIR_STORAGE_MAX_HEADER;
> 
> change to size_t and a new variable "ssize_t read_len" be created and
> used for the following and subsequent virStorageFileGetMetadataFromBuf
> call?  (although that also takes a size_t for the 3rd param).

I actually just used VIR_STORAGE_MAX_HEADER constant directly in the
call of virStorageBackendGlusterRead. The original variable will thus be
used only for the return value, which is signed.


> 
> > -    if ((len = virStorageBackendGlusterReadHeader(fd, name, len, &header)) < 0)
> > +    if ((len = virStorageBackendGlusterRead(fd, name, len, &header)) < 0)
> >          goto cleanup;
> > 
> >      if (!(meta = virStorageFileGetMetadataFromBuf(name, header, len,
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list