Re: [Xen-devel] [PATCH v7 01/10] x86/hypervisor: make hypervisor_ap_setup return an error code

[Wei Liu <wl@xen.org>]

On Wed, Feb 05, 2020 at 12:12:30PM +0100, Jan Beulich wrote:
> On 04.02.2020 16:36, Wei Liu wrote:
> > @@ -215,18 +220,19 @@ static void init_evtchn(void)
> >      rc = xen_hypercall_set_evtchn_upcall_vector(this_cpu(vcpu_id),
> >                                                  evtchn_upcall_vector);
> >      if ( rc )
> > -        panic("Unable to set evtchn upcall vector: %d\n", rc);
> > +    {
> > +        printk("Unable to set evtchn upcall vector: %d\n", rc);
> > +        goto out;
> 
> There's no need for "goto" here - "return rc" is all you need
> instead. As stated elsewhere, when there's complex cleanup or
> a fair risk of leaving out an important cleanup step, I can
> live with "goto" getting used. But I don't think it should be
> used to replace a simple "return".

OK. That can be fixed.

> 
> With this
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with one more (optional!) suggestion and one more remark:

Thanks.

> 
> > @@ -254,14 +260,20 @@ static void __init setup(void)
> >                 XEN_LEGACY_MAX_VCPUS);
> >      }
> >  
> > -    init_evtchn();
> > +    BUG_ON(init_evtchn());
> >  }
> >  
> > -static void ap_setup(void)
> > +static int ap_setup(void)
> >  {
> > +    int rc;
> > +
> >      set_vcpu_id();
> > -    map_vcpuinfo();
> > -    init_evtchn();
> > +
> > +    rc = map_vcpuinfo();
> > +    if ( rc )
> > +        return rc;
> > +
> > +    return init_evtchn();
> >  }
> 
> To avoid a local variable, how about
> 
>     return map_vcpuinfo() ?: init_evtchn();
> 
> ?

ISTR this is a GNU extension, but seeing that there is already quite a
lot of it in hypercisor code, I will make the change.

Wei.

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