Re: [PATCH 2/2] HVM: clean up hvm_save_one()

From: Wei Liu <wei.liu2@citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <Ian.Jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
	xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 2/2] HVM: clean up hvm_save_one()
Date: Tue, 6 Jun 2017 18:52:13 +0100	[thread overview]
Message-ID: <20170606175213.ow35l2vtlrnxsd5d@citrix.com> (raw)
In-Reply-To: <592E8C06020000780015DFD4@prv-mh.provo.novell.com>

On Wed, May 31, 2017 at 01:25:26AM -0600, Jan Beulich wrote:
> Eliminate the for_each_vcpu() loop and the associated local variables,
> don't override the save handler's return code, and correct formatting.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/common/hvm/save.c
> +++ b/xen/common/hvm/save.c
> @@ -79,36 +79,27 @@ size_t hvm_save_size(struct domain *d)
>  int hvm_save_one(struct domain *d, unsigned int typecode, unsigned int instance,
>                   XEN_GUEST_HANDLE_64(uint8) handle, uint64_t *bufsz)
>  {
> -    int rv = -ENOENT;
> -    size_t sz = 0;
> -    struct vcpu *v;
> -    hvm_domain_context_t ctxt = { 0, };
> +    int rv;
> +    hvm_domain_context_t ctxt = { };
>      const struct hvm_save_descriptor *desc;
>  
> -    if ( d->is_dying 
> -         || typecode > HVM_SAVE_CODE_MAX 
> -         || hvm_sr_handlers[typecode].size < sizeof(*desc)
> -         || hvm_sr_handlers[typecode].save == NULL )
> +    if ( d->is_dying ||
> +         typecode > HVM_SAVE_CODE_MAX ||
> +         hvm_sr_handlers[typecode].size < sizeof(*desc) ||
> +         !hvm_sr_handlers[typecode].save )
>          return -EINVAL;
>  
> +    ctxt.size = hvm_sr_handlers[typecode].size;
>      if ( hvm_sr_handlers[typecode].kind == HVMSR_PER_VCPU )
> -        for_each_vcpu(d, v)
> -            sz += hvm_sr_handlers[typecode].size;
> -    else 
> -        sz = hvm_sr_handlers[typecode].size;
> -    
> -    ctxt.size = sz;
> -    ctxt.data = xmalloc_bytes(sz);
> +        hvm_sr_handlers[typecode].size *= d->max_vcpus;

Why is size updated with a particular d->max_vcpus here? AFAICT (after
going through layers of macros ...) hvm_sr_handlers is global and needed
when saving any hvm guests. The "size" field contains the length of one
record.

Also, you set ctxt.size before this loop without taking into account the
number of vcpus, which looks wrong to me. Shouldn't it be (when not
updating hvm_sr_handlers[typecode].size)

   ctxt.size = hvm_sr_handlers[typecode].size * d->max_vcpus

?

> +    ctxt.data = xmalloc_bytes(ctxt.size);
>      if ( !ctxt.data )
>          return -ENOMEM;
>  
> -    if ( hvm_sr_handlers[typecode].save(d, &ctxt) != 0 )
> -    {
> -        printk(XENLOG_G_ERR "HVM%d save: failed to save type %"PRIu16"\n",
> -               d->domain_id, typecode);
> -        rv = -EFAULT;
> -    }
> -    else if ( ctxt.cur >= sizeof(*desc) )
> +    if ( (rv = hvm_sr_handlers[typecode].save(d, &ctxt)) != 0 )
> +        printk(XENLOG_G_ERR "HVM%d save: failed to save type %"PRIu16" (%d)\n",
> +               d->domain_id, typecode, rv);
> +    else if ( rv = -ENOENT, ctxt.cur >= sizeof(*desc) )

I guess the intent here is to set rv while at the same time only test
ctxt.cur? But why? Can the code be reorganised so that it is easier to
reason about.

Wei.

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