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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.06.17 at 12:29, <wei.liu2@xxxxxxxxxx> wrote:
> On Wed, Jun 07, 2017 at 04:07:02AM -0600, Jan Beulich wrote:
>> >>> On 06.06.17 at 19:52, <wei.liu2@xxxxxxxxxx> 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


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