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

[John Ferlan]

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 at redhat.com>

John

[...]