Re: [Qemu-devel] [PATCH COLO-Frame v12 14/38] ram: Split host_from_stream_offset() into two helper functions

[Dr. David Alan Gilbert]

* zhanghailiang (address@hidden) wrote:
> Split host_from_stream_offset() into two parts:
> One is to get ram block, which the block idstr may be get from migration
> stream, the other is to get hva (host) address from block and the offset.
> Besides, we will do the check working in a new helper offset_in_ramblock().
> 
> Signed-off-by: zhanghailiang <address@hidden>
> ---
> v12:
> - Remove the offset parameter for ram_block_from_stream() and
>   check the validity of the related value in a new helper. (Dave's suggestion)
> v11:
> - New patch
> 
> Signed-off-by: zhanghailiang <address@hidden>

Reviewed-by: Dr. David Alan Gilbert <address@hidden>

> ---
>  include/exec/ram_addr.h |  8 ++++++--
>  migration/ram.c         | 40 +++++++++++++++++++++++++---------------
>  2 files changed, 31 insertions(+), 17 deletions(-)
> 
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index 7115154..2b31279 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -38,10 +38,14 @@ struct RAMBlock {
>      int fd;
>  };
>  
> +static inline bool offset_in_ramblock(RAMBlock *b, ram_addr_t offset)
> +{
> +    return (b && b->host && offset < b->used_length) ? true : false;
> +}
> +
>  static inline void *ramblock_ptr(RAMBlock *block, ram_addr_t offset)
>  {
> -    assert(offset < block->used_length);
> -    assert(block->host);
> +    assert(offset_in_ramblock(block, offset));
>      return (char *)block->host + offset;
>  }
>  
> diff --git a/migration/ram.c b/migration/ram.c
> index a709471..09fe6e6 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2138,28 +2138,24 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, 
> void *host)
>   * Returns a pointer from within the RCU-protected ram_list.
>   */
>  /*
> - * Read a RAMBlock ID from the stream f, find the host address of the
> - * start of that block and add on 'offset'
> + * Read a RAMBlock ID from the stream f.
>   *
>   * f: Stream to read from
> - * offset: Offset within the block
>   * flags: Page flags (mostly to see if it's a continuation of previous block)
>   */
> -static inline void *host_from_stream_offset(QEMUFile *f,
> -                                            ram_addr_t offset,
> -                                            int flags)
> +static inline RAMBlock *ram_block_from_stream(QEMUFile *f,
> +                                              int flags)
>  {
>      static RAMBlock *block = NULL;
>      char id[256];
>      uint8_t len;
>  
>      if (flags & RAM_SAVE_FLAG_CONTINUE) {
> -        if (!block || block->max_length <= offset) {
> +        if (!block) {
>              error_report("Ack, bad migration stream!");
>              return NULL;
>          }
> -
> -        return block->host + offset;
> +        return block;
>      }
>  
>      len = qemu_get_byte(f);
> @@ -2167,12 +2163,22 @@ static inline void *host_from_stream_offset(QEMUFile 
> *f,
>      id[len] = 0;
>  
>      block = qemu_ram_block_by_name(id);
> -    if (block && block->max_length > offset) {
> -        return block->host + offset;
> +    if (!block) {
> +        error_report("Can't find block %s", id);
> +        return NULL;
>      }
>  
> -    error_report("Can't find block %s", id);
> -    return NULL;
> +    return block;
> +}
> +
> +static inline void *host_from_ram_block_offset(RAMBlock *block,
> +                                               ram_addr_t offset)
> +{
> +    if (!offset_in_ramblock(block, offset)) {
> +        return NULL;
> +    }
> +
> +    return block->host + offset;
>  }
>  
>  /*
> @@ -2319,7 +2325,9 @@ static int ram_load_postcopy(QEMUFile *f)
>          trace_ram_load_postcopy_loop((uint64_t)addr, flags);
>          place_needed = false;
>          if (flags & (RAM_SAVE_FLAG_COMPRESS | RAM_SAVE_FLAG_PAGE)) {
> -            host = host_from_stream_offset(f, addr, flags);
> +            RAMBlock *block = ram_block_from_stream(f, flags);
> +
> +            host = host_from_ram_block_offset(block, addr);
>              if (!host) {
>                  error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
>                  ret = -EINVAL;
> @@ -2450,7 +2458,9 @@ static int ram_load(QEMUFile *f, void *opaque, int 
> version_id)
>  
>          if (flags & (RAM_SAVE_FLAG_COMPRESS | RAM_SAVE_FLAG_PAGE |
>                       RAM_SAVE_FLAG_COMPRESS_PAGE | RAM_SAVE_FLAG_XBZRLE)) {
> -            host = host_from_stream_offset(f, addr, flags);
> +            RAMBlock *block = ram_block_from_stream(f, flags);
> +
> +            host = host_from_ram_block_offset(block, addr);
>              if (!host) {
>                  error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
>                  ret = -EINVAL;
> -- 
> 1.8.3.1
> 
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK