| Message ID | 592E8C06020000780015DFD4@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
>>> On 06.06.17 at 19:52, <wei.liu2@citrix.com> wrote: > 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 > > ? Right, this is complete rubbish. Should be ctxt.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? Well, we can't use -ENOENT as initializer anymore, as rv now is being modified above. Before entering the body of the "else if" it needs to be -ENOENT though. > Can the code be reorganised so that it is easier to reason about. It probably could be, at the expense of assigning -ENOENT in two places. Jan
On Wed, Jun 07, 2017 at 04:07:02AM -0600, Jan Beulich wrote: > >>> On 06.06.17 at 19:52, <wei.liu2@citrix.com> wrote: > > 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 > > > > ? > > Right, this is complete rubbish. Should be > > ctxt.size *= d->max_vcpus; > Yes, this looks right to me now. > >> + 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? > > Well, we can't use -ENOENT as initializer anymore, as rv now is > being modified above. Before entering the body of the "else if" > it needs to be -ENOENT though. > > > Can the code be reorganised so that it is easier to reason about. > > It probably could be, at the expense of assigning -ENOENT in two > places. > How about: if ( (rv = hvm_sr_handlers ...)) != 0 ) { } else { rv = -ENOENT; if ( ctx.cur >= sizeof(*desc) ) { } } > Jan >
>>> On 07.06.17 at 12:29, <wei.liu2@citrix.com> wrote: > On Wed, Jun 07, 2017 at 04:07:02AM -0600, Jan Beulich wrote: >> >>> On 06.06.17 at 19:52, <wei.liu2@citrix.com> wrote: >> > On Wed, May 31, 2017 at 01:25:26AM -0600, Jan Beulich wrote: >> >> 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? >> >> Well, we can't use -ENOENT as initializer anymore, as rv now is >> being modified above. Before entering the body of the "else if" >> it needs to be -ENOENT though. >> >> > Can the code be reorganised so that it is easier to reason about. >> >> It probably could be, at the expense of assigning -ENOENT in two >> places. >> > > How about: > > if ( (rv = hvm_sr_handlers ...)) != 0 ) > { > } else { > rv = -ENOENT; > > if ( ctx.cur >= sizeof(*desc) ) > { > > } > > } Well, that would require re-indenting the entire body, which I wanted to avoid as well. Jan
--- 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; + 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) ) { uint32_t off;