Re: [Xen-devel] [PATCH v15 11/14] x86/domctl: Use hvm_save_vcpu_handler

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.08.18 at 15:53, <aisaila@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/save.c
> +++ b/xen/arch/x86/hvm/save.c
> @@ -196,7 +196,10 @@ int hvm_save(struct domain *d, hvm_domain_context_t *h)
>      struct hvm_save_header hdr;
>      struct hvm_save_end end;
>      hvm_save_handler handler;
> +    hvm_save_vcpu_handler save_one_handler;
>      unsigned int i;
> +    int rc;
> +    struct vcpu *v;

Please move the declarations you add into the scopes where they're
actually needed (but please avoid replicating rc). I realize pre-existing
code isn't in line with this, but please le's not widen the problem. In
fact I wouldn't mind at all if you moved handler down right away. But
as that's slated to go away, that's probably not very important.

> @@ -224,11 +227,32 @@ int hvm_save(struct domain *d, hvm_domain_context_t *h)
>      for ( i = 0; i <= HVM_SAVE_CODE_MAX; i++ )
>      {
>          handler = hvm_sr_handlers[i].save;
> -        if ( handler != NULL )
> +        save_one_handler = hvm_sr_handlers[i].save_one;
> +        if ( save_one_handler != NULL )

Would you mind omitting the redundant "!= NULL" here and below?

> +        {
> +            for_each_vcpu ( d, v )
> +            {
> +                printk(XENLOG_G_INFO "HVM %pv save: %s\n",
> +                       v, hvm_sr_handlers[i].name);
> +                rc = save_one_handler(v, h);
> +
> +                if( rc != 0 )

Missing blank, and just like above "!= 0" is redundant and could be
omitted (same below).

> +                {
> +                    printk(XENLOG_G_ERR
> +                           "HVM %pv save: failed to save type %"PRIu16"\n",
> +                           v, i);
> +                    return -EFAULT;

Why -EFAULT? The pre-existing bad use does not count as an excuse.
If the value of rc can't be used (perhaps because there may be positive
values or -1 coming back), pick something that at least comes a little
closer to representing the actual condition (EIO, ENODATA, EOPNOTSUPP
all come to mind, but much depends on what conditions actually exist).
I'd then encourage you to also change the pre-existing bad use.

> +                }
> +            }
> +        }
> +        else if ( handler != NULL )
>          {
>              printk(XENLOG_G_INFO "HVM%d save: %s\n",
>                     d->domain_id, hvm_sr_handlers[i].name);
> -            if ( handler(d, h) != 0 )
> +
> +            rc = handler(d, h);
> +
> +            if( rc != 0 )

Please either omit the blank line ahead of the invocation of handler(),
or the one following it. First and foremost: Have this block be
consistent blank line wise with the one above.

Jan



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