Re: [Xen-devel] [PATCH v16 12/13] x86/hvm: Remove redundant save functions

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.08.18 at 11:21, <aisaila@xxxxxxxxxxxxxxx> wrote:
> @@ -148,6 +145,11 @@ int hvm_save_one(struct domain *d, unsigned int 
> typecode, unsigned int instance,
>           !hvm_sr_handlers[typecode].save )
>          return -EINVAL;
>  
> +    if ( hvm_sr_handlers[typecode].kind == HVMSR_PER_VCPU &&
> +        instance >= d->max_vcpus )
> +        return -ENOENT;
> +    if ( hvm_sr_handlers[typecode].kind == HVMSR_PER_DOM )
> +        instance = 0;

How can you validly override what the caller has requested? There's a
use further down in the function (" if ( instance == desc->instance )")
which you break with the above. As said at least once before - we
have an example of a two-instance per-domain save record, and you
should keep that code functioning even if there's currently no in-tree
caller.

Also when checking a field (here: .kind) like this, please use a
switch() statement. But perhaps here if/else would suffice to avoid
the redundant field reference.

> @@ -223,17 +225,29 @@ int hvm_save(struct domain *d, hvm_domain_context_t *h)
>      for ( i = 0; i <= HVM_SAVE_CODE_MAX; i++ )
>      {
>          struct vcpu *v;
> -        hvm_save_vcpu_handler save_one_handler = hvm_sr_handlers[i].save_one;
> -        hvm_save_handler handler = hvm_sr_handlers[i].save;;
> +        hvm_save_handler handler = hvm_sr_handlers[i].save;
>  
> -        if ( save_one_handler )
> +        if ( handler )
>          {
> -            for_each_vcpu ( d, v )
> +            if ( hvm_sr_handlers[i].kind == HVMSR_PER_VCPU )
>              {
> -                printk(XENLOG_G_INFO "HVM %pv save: %s\n",
> -                       v, hvm_sr_handlers[i].name);
> -
> -                if ( save_one_handler(v, h) != 0 )
> +                for_each_vcpu ( d, v )

Please avoid re-indenting all of this code, by simply inverting the outer
if() to

        if ( !handler )
            continue;

For reviewers this will also make more obvious what the actual
changes are.

> +                {
> +                    printk(XENLOG_G_INFO "HVM %pv save: %s\n",
> +                           v, hvm_sr_handlers[i].name);
> +
> +                    if ( handler(v, h) != 0 )
> +                    {
> +                        printk(XENLOG_G_ERR
> +                               "HVM %pv save: failed to save type 
> %"PRIu16"\n",
> +                               v, i);
> +                        return -ENODATA;
> +                    }
> +                }
> +            }
> +            else
> +            {
> +                if ( handler(v, h) != 0 )

I can't seem to be able to spot where v would get set before the use
here. Did you test your code, making sure that migration still works?

Also note how this could easily be "else if ()".

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel