Re: [Qemu-devel] [PATCH V12 3/6] qed_read_string to bdrv_read_string

[Michael Roth <mdroth@linux.vnet.ibm.com>]

On Fri, Aug 10, 2012 at 11:39:42PM +0800, Dong Xu Wang wrote:
> Make qed_read_string function to a common interface, so move it to block.c.
> 
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> ---
>  block.c     |   27 +++++++++++++++++++++++++++
>  block.h     |    2 ++
>  block/qed.c |   29 +----------------------------
>  3 files changed, 30 insertions(+), 28 deletions(-)
> 
> diff --git a/block.c b/block.c
> index c13d803..d906b35 100644
> --- a/block.c
> +++ b/block.c
> @@ -213,6 +213,33 @@ int path_has_protocol(const char *path)
>      return *p == ':';
>  }
> 
> +/**
> + * Read a string of known length from the image file
> + *
> + * @bs:         Image file
> + * @offset:     File offset to start of string, in bytes
> + * @n:          String length in bytes
> + * @buf:        Destination buffer
> + * @buflen:     Destination buffer length in bytes
> + * @ret:        0 on success, -errno on failure
> + *
> + * The string is NUL-terminated.
> + */
> +int bdrv_read_string(BlockDriverState *bs, uint64_t offset, size_t n,
> +                           char *buf, size_t buflen)

Small alignment issue   ^

> +{
> +    int ret;
> +    if (n >= buflen) {
> +        return -EINVAL;
> +    }
> +    ret = bdrv_pread(bs, offset, buf, n);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +    buf[n] = '\0';
> +    return 0;
> +}
> +
>  int path_is_absolute(const char *path)
>  {
>  #ifdef _WIN32
> diff --git a/block.h b/block.h
> index 54e61c9..e5dfcd7 100644
> --- a/block.h
> +++ b/block.h
> @@ -154,6 +154,8 @@ int bdrv_pwrite_sync(BlockDriverState *bs, int64_t offset,
>      const void *buf, int count);
>  int coroutine_fn bdrv_co_readv(BlockDriverState *bs, int64_t sector_num,
>      int nb_sectors, QEMUIOVector *qiov);
> +int bdrv_read_string(BlockDriverState *bs, uint64_t offset, size_t n,
> +    char *buf, size_t buflen);

Another one here        ^

>  int coroutine_fn bdrv_co_copy_on_readv(BlockDriverState *bs,
>      int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);
>  int coroutine_fn bdrv_co_writev(BlockDriverState *bs, int64_t sector_num,
> diff --git a/block/qed.c b/block/qed.c
> index 5f3eefa..311c589 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -217,33 +217,6 @@ static bool qed_is_image_size_valid(uint64_t image_size, uint32_t cluster_size,
>  }
> 
>  /**
> - * Read a string of known length from the image file
> - *
> - * @file:       Image file
> - * @offset:     File offset to start of string, in bytes
> - * @n:          String length in bytes
> - * @buf:        Destination buffer
> - * @buflen:     Destination buffer length in bytes
> - * @ret:        0 on success, -errno on failure
> - *
> - * The string is NUL-terminated.
> - */
> -static int qed_read_string(BlockDriverState *file, uint64_t offset, size_t n,
> -                           char *buf, size_t buflen)
> -{
> -    int ret;
> -    if (n >= buflen) {
> -        return -EINVAL;
> -    }
> -    ret = bdrv_pread(file, offset, buf, n);
> -    if (ret < 0) {
> -        return ret;
> -    }
> -    buf[n] = '\0';
> -    return 0;
> -}
> -
> -/**
>   * Allocate new clusters
>   *
>   * @s:          QED state
> @@ -437,7 +410,7 @@ static int bdrv_qed_open(BlockDriverState *bs, int flags)
>              return -EINVAL;
>          }
> 
> -        ret = qed_read_string(bs->file, s->header.backing_filename_offset,
> +        ret = bdrv_read_string(bs->file, s->header.backing_filename_offset,
>                                s->header.backing_filename_size, bs->backing_file,
>                                sizeof(bs->backing_file));

Here too                          ^

Looks good otherwise.

>          if (ret < 0) {
> -- 
> 1.7.1
> 
>