"Dr. David Alan Gilbert" <dgilb...@redhat.com> wrote: > * Peter Xu (pet...@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.
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 Done. Once there, I spelled postcopy correctly O:-)
