Re: [Qemu-devel] [PATCH 01/51] ram: Update all functions comments

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Peter Xu (peterx@redhat.com) wrote:
> Hi, Juan,
> 
> Got several nitpicks below... (along with some questions)
> 
> On Thu, Mar 23, 2017 at 09:44:54PM +0100, Juan Quintela wrote:
> 
> [...]

> > @@ -1157,11 +1186,12 @@ static bool get_queued_page(MigrationState *ms, PageSearchStatus *pss,
> >  }
> >  
> >  /**
> > - * flush_page_queue: Flush any remaining pages in the ram request queue
> > - *    it should be empty at the end anyway, but in error cases there may be
> > - *    some left.
> > + * flush_page_queue: flush any remaining pages in the ram request queue
> 
> Here the comment says (just like mentioned in function name) that we
> will "flush any remaining pages in the ram request queue", however in
> the implementation, we should be only freeing everything in
> src_page_requests. The problem is "flush" let me think about "flushing
> the rest of the pages to the other side"... while it's not.
> 
> Would it be nice we just rename the function into something else, like
> migration_page_queue_free()? We can tune the comments correspondingly
> as well.

Yes that probably would be a better name.

> [...]
> 
> > -/*
> > - * Helper for postcopy_chunk_hostpages; it's called twice to cleanup
> > - *   the two bitmaps, that are similar, but one is inverted.
> > +/**
> > + * postcopy_chuck_hostpages_pass: canocalize bitmap in hostpages
>                   ^ should be n?     ^^^^^^^^^^ canonicalize?
> 
> >   *
> > - * We search for runs of target-pages that don't start or end on a
> > - * host page boundary;
> > - * unsent_pass=true: Cleans up partially unsent host pages by searching
> > - *                 the unsentmap
> > - * unsent_pass=false: Cleans up partially dirty host pages by searching
> > - *                 the main migration bitmap
> > + * Helper for postcopy_chunk_hostpages; it's called twice to
> > + * canonicalize the two bitmaps, that are similar, but one is
> > + * inverted.
> >   *
> > + * Postcopy requires that all target pages in a hostpage are dirty or
> > + * clean, not a mix.  This function canonicalizes the bitmaps.
> > + *
> > + * @ms: current migration state
> > + * @unsent_pass: if true we need to canonicalize partially unsent host pages
> > + *               otherwise we need to canonicalize partially dirty host pages
> > + * @block: block that contains the page we want to canonicalize
> > + * @pds: state for postcopy
> >   */
> >  static void postcopy_chunk_hostpages_pass(MigrationState *ms, bool unsent_pass,
> >                                            RAMBlock *block,
> 
> [...]
> 
> > +/**
> > + * ram_save_setup: iterative stage for migration
>       ^^^^^^^^^^^^^^ should be ram_save_iterate()?
> 
> > + *
> > + * Returns zero to indicate success and negative for error
> > + *
> > + * @f: QEMUFile where to send the data
> > + * @opaque: RAMState pointer
> > + */
> >  static int ram_save_iterate(QEMUFile *f, void *opaque)
> >  {
> >      int ret;
> > @@ -2091,7 +2169,16 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
> >      return done;
> >  }
> 
> [...]
> 
> > -/*
> > - * Allocate data structures etc needed by incoming migration with postcopy-ram
> > - * postcopy-ram's similarly names postcopy_ram_incoming_init does the work
> > +/**
> > + * ram_postococpy_incoming_init: allocate postcopy data structures
> > + *
> > + * Returns 0 for success and negative if there was one error
> > + *
> > + * @mis: current migration incoming state
> > + *
> > + * Allocate data structures etc needed by incoming migration with
> > + * postcopy-ram postcopy-ram's similarly names
> > + * postcopy_ram_incoming_init does the work
> 
> This sentence is slightly hard to understand... But I think the
> function name explained itself enough though. :)

A '.' after the first 'postcopy-ram' would make it more readable.

Dave

> Thanks,
> 
> -- peterx
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK