Re: [PATCH 5/8] migration: move calling control_save_page to the common place

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

* Xiao Guangrong (guangrong.xiao@gmail.com) wrote:
> 
> 
> On 03/15/2018 07:47 PM, Dr. David Alan Gilbert wrote:
> 
> > >       /* Check the pages is dirty and if it is send it */
> > >       if (migration_bitmap_clear_dirty(rs, pss->block, pss->page)) {
> > > +        RAMBlock *block = pss->block;
> > > +        ram_addr_t offset = pss->page << TARGET_PAGE_BITS;
> > > +
> > > +        if (control_save_page(rs, block, offset, &res)) {
> > > +            goto page_saved;
> > 
> > OK, but I'd prefer if you avoided this forward goto;  we do use goto but
> > we tend to keep it just for error cases.
> > 
> 
> There is a common operation, clearing unsentmap, for save_control,
> save_zero, save_compressed and save_normal, if we do not use 'goto',
> the operation would to be duplicated several times or we will have
> big if...elseif...elseif... section.
> 
> So it may be not too bad to have 'goto' under this case? :)

The problem is it always tends to creep a bit, and then you soon have
a knot of goto's.

I suggest you add a 'page_saved' bool, set it instead of taking the
goto, and then add a if (!page_saved) around the next section.
It doesn't need to nest for the last section; you just do another
if (!page_saved) if around that.

Dave

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