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

[Dong Xu Wang]

On Fri, Sep 7, 2012 at 1:32 AM, Michael Roth <address@hidden> wrote:
> 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 <address@hidden>
>> ---
>>  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
>>
>>
>
Thank you Michael .