Re: [PATCH 2/5] xc/tmem: Unchecked return value.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Nov 25, 2013 at 05:11:31PM +0000, Andrew Cooper wrote:
> On 25/11/13 17:00, Konrad Rzeszutek Wilk wrote:
> > CID 1055045 complains that the return value needs checking.
> >
> > The tmem op is a flush call and there is no expectation
> > right now of any return besides zero. But just in case
> > this changes lets blow up.
> >
> > CC: Bob Liu <bob.liu@oracle.com>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  tools/libxc/xc_domain_save.c | 6 ++++--
> >  tools/libxc/xc_tmem.c        | 4 ++--
> >  tools/libxc/xenctrl.h        | 2 +-
> >  3 files changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/libxc/xc_domain_save.c b/tools/libxc/xc_domain_save.c
> > index 42c4752..c3f0f19 100644
> > --- a/tools/libxc/xc_domain_save.c
> > +++ b/tools/libxc/xc_domain_save.c
> > @@ -2095,8 +2095,10 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t dom, uint32_t max_iter
> >          goto copypages;
> >      }
> >  
> > -    if ( tmem_saved != 0 && live )
> > -        xc_tmem_save_done(xch, dom);
> > +    if ( tmem_saved != 0 && live ) {
> > +        if ( xc_tmem_save_done(xch, dom) )
> 
> These could be combined into a single if statement.
> 
> > +            DPRINTF("Warning - failed to flush tmem on originating server.");
> 
> Does this need to bail at this point then?

No. This failing means that the host hypervisor has some pages it couldn't free.
It should never happen (ha!) - but in case it does it is OK to forget about them
and just leave the 'zombie' guest behind.

> 
> > +    }
> >  
> >      if ( live )
> >      {
> > diff --git a/tools/libxc/xc_tmem.c b/tools/libxc/xc_tmem.c
> > index 0918615..df50ef5 100644
> > --- a/tools/libxc/xc_tmem.c
> > +++ b/tools/libxc/xc_tmem.c
> > @@ -366,9 +366,9 @@ int xc_tmem_save_extra(xc_interface *xch, int dom, int io_fd, int field_marker)
> >  }
> >  
> >  /* only called for live migration */
> > -void xc_tmem_save_done(xc_interface *xch, int dom)
> > +int xc_tmem_save_done(xc_interface *xch, int dom)
> >  {
> > -    xc_tmem_control(xch,0,TMEMC_SAVE_END,dom,0,0,0,NULL);
> > +    return xc_tmem_control(xch,0,TMEMC_SAVE_END,dom,0,0,0,NULL);
> 
> As you are tweaking this, could you apply some coding style with spaces
> following commas?

Of course. I thought it was frowend to mix coding style changes with
bug-fixes? Or was it more of an follow-up patch that would clean this
mess up?

> 
> ~Andrew
> 
> >  }
> >  
> >  /* restore routines */
> > diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
> > index 4ac6b8a..18d4f78 100644
> > --- a/tools/libxc/xenctrl.h
> > +++ b/tools/libxc/xenctrl.h
> > @@ -1996,7 +1996,7 @@ int xc_tmem_control(xc_interface *xch,
> >  int xc_tmem_auth(xc_interface *xch, int cli_id, char *uuid_str, int arg1);
> >  int xc_tmem_save(xc_interface *xch, int dom, int live, int fd, int field_marker);
> >  int xc_tmem_save_extra(xc_interface *xch, int dom, int fd, int field_marker);
> > -void xc_tmem_save_done(xc_interface *xch, int dom);
> > +int xc_tmem_save_done(xc_interface *xch, int dom);
> >  int xc_tmem_restore(xc_interface *xch, int dom, int fd);
> >  int xc_tmem_restore_extra(xc_interface *xch, int dom, int fd);
> >  
>