Re: [Xen-devel] [PATCH v11 08/23] x86: refactor psr: L3 CAT: set value: implement framework.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.06.17 at 10:18, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> On 17-06-06 01:43:00, Jan Beulich wrote:
>> >>> On 02.06.17 at 04:49, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
>> > On 17-06-01 04:45:58, Jan Beulich wrote:
>> >> >>> On 01.06.17 at 12:00, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
>> >> > On 17-05-30 08:32:59, Jan Beulich wrote:
>> >> >> >>> On 03.05.17 at 10:44, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
>> >> >> > + free_array:
>> >> >> > +    xfree(val_array);
>> >> >> > +    return ret;
>> >> >> > +
>> >> >> > + unlock_free_array:
>> >> >> > +    spin_unlock(&info->ref_lock);
>> >> >> > +    xfree(val_array);
>> >> >> > +    return ret;
>> >> >> > +}
>> >> >> 
>> >> >> I'm sure I've said so before - please don't duplicate error paths like
>> >> >> this. Here it's still easy to see all is fine, but what if each path 
>> >> >> gets
>> >> >> two or three more thing added. Please chain them together via goto.
>> >> >> 
>> >> > To make things clear, I wrote below codes. How about them?
>> >> >  unlock_free_array:
>> >> >     spin_unlock(&info->ref_lock);
>> >> > 
>> >> >  free_array:
>> >> >     xfree(val_array);
>> >> >     return ret;
>> >> 
>> >> I don't think that'll be okay for the case which previously fell
>> >> through to free_array.
>> >> 
>> > I tried to understand your meaning. Do you mean below codes?
>> > 
>> >     set_bit(d->domain_id, info->dom_ids); //Success path.
>> >     goto free_array;
>> > 
>> >  unlock_free_array:
>> >     spin_unlock(&info->ref_lock);
>> > 
>> >  free_array:
>> >     xfree(val_array);
>> >     return ret;
>> 
>> Coming close: Once again, using "goto" on error paths is half way
>> acceptable to me, while using it anywhere else normally isn't.
>> Hence you want the "unlock_free_array" path "goto free_array;"
>> rather than the normal (success) one. Alternatively you might use
>> a local variable to signal whether to release the lock.
>> 
> How about this which can avoid a local variable?
> 
>     set_bit(d->domain_id, info->dom_ids);
> 
>  free_array:
>     xfree(val_array);
>     return ret;
> 
>  unlock_free_array:
>     spin_unlock(&info->ref_lock);
>     goto free_array;

Yes, that's what I've been asking for.

Jan


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