Re: [Xen-devel] [PATCH v4 7/9] tools/libxc: x86 pv restore implementation

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2014-05-07 at 16:37 +0100, Andrew Cooper wrote:
> On 07/05/14 15:10, Ian Campbell wrote:
> >
> >> +    if ( xc_domain_getinfo(xch, dom, 1, &ctx.dominfo) != 1 )
> >> +    {
> >> +        PERROR("Failed to get domain info");
> >> +        return -1;
> >> +    }
> >> +
> >> +    if ( ctx.dominfo.domid != dom )
> >> +    {
> >> +        ERROR("Domain %d does not exist", dom);
> >> +        return -1;
> >> +    }
> >> +
> >> +    ctx.domid = dom;
> >> +    IPRINTF("Restoring domain %d", dom);
> >> +
> >> +    if ( read_headers(&ctx) )
> >> +        return -1;
> >> +
> >> +    if ( ctx.dominfo.hvm )
> >> +    {
> >> +        ERROR("HVM Restore not supported yet");
> >> +        return -1;
> >> +    }
> >> +    else
> > Unneeded.
> 
> Perhaps, but it makes more readable patches as hvm restore is added later.

OK, I think this one got dumped in with all the other actually
unnecessary elses in my mind.

> 
> Furthermore, all of this will need tweaking when this series stops
> trying to coexist alongside the legacy code.

From a reviewer's point of view the sooner this happens the better.


> >
> >
> >> +        return -1;
> >> +    }
> >> +    else if ( rec->length > sizeof *vcpu + 128 )
> > There's an awful lot of magic numbers throughout this series. Can we try
> > and use meaningful symbolic names for things please.
> 
> This one, no.  It is magic even in the legacy stream, and is sizeof
> domctl.u on the sending toolstack, which thinking forward is not
> necessarily sizeof domctl.u on the receiving toolstack.

OK. Where a legitimate magic number remains then please add a comment.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel