Re: [Qemu-devel] [PATCH COLO-Frame v11 14/39] 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.
> 
> Signed-off-by: zhanghailiang <address@hidden>

OK, I see why you're doing this from the next patch.

> ---
> v11:
> - New patch
> ---
>  migration/ram.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index cfe78aa..a161620 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2136,9 +2136,9 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, 
> void *host)
>   * 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,
> +                                              ram_addr_t offset,
> +                                              int flags)
>  {
>      static RAMBlock *block = NULL;
>      char id[256];
> @@ -2150,22 +2150,31 @@ static inline void *host_from_stream_offset(QEMUFile 
> *f,
>              return NULL;
>          }
>  
> -        return block->host + offset;
> +        return block;
>      }
> -
>      len = qemu_get_byte(f);
>      qemu_get_buffer(f, (uint8_t *)id, len);
>      id[len] = 0;
>  
>      block = qemu_ram_block_by_name(id);
>      if (block && block->max_length > offset) {
> -        return block->host + offset;
> +        return block;
>      }
>  
>      error_report("Can't find block %s", id);
>      return NULL;
>  }
>  
> +static inline void *host_from_ram_block_offset(RAMBlock *block,
> +                                               ram_addr_t offset)
> +{
> +    if (!block) {
> +        return NULL;
> +    }
> +
> +    return block->host + offset;
> +}

That's almost the same as ramblock_ptr in include/exec/ram_addr.h, but
it assert's rather than doing NULL on errors.

I'm not sure about this, but can I suggest:

   ram_block_from_stream(QEMUFile *f, int flags)

  doesn't have the offset; just finds the block and handles the CONT.

   bool offset_in_ramblock(RAMBlock *b, ram_addr_t offset);

  actually does the check; put this in exec.c, and declare it in 
include/exec/ram_addr.h

   void *ramblock_ptr_try(RAMBlock *block, ram_addr_t offset)
  which returns NULL if offset_in_ramblock fails, and otherwise returns the 
result
of ramblock_ptr - again put that in include/exec/ram_addr.h

(I'm not sure about this - I almost suggested changing ramblock_ptr to not do
the checks, and just add a call to assert(offset_in_ramblock) before each use, 
but
that sounded too painful).

Hmm - we check here for block->max_length > offset - where as the check in
ram_addr.h is used_length - I wonder if we should be using used_length?

Dave

> +
>  /*
>   * If a page (or a whole RDMA chunk) has been
>   * determined to be zero, then zap it.
> @@ -2310,7 +2319,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, addr, flags);
> +
> +            host = host_from_ram_block_offset(block, addr);
>              if (!host) {
>                  error_report("Illegal RAM offset " RAM_ADDR_FMT, addr);
>                  ret = -EINVAL;
> @@ -2441,7 +2452,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, addr, 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