Re: [Xen-devel] [PATCH v11 07/23] x86: refactor psr: L3 CAT: implement get value flow.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 31.05.17 at 10:05, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> On 17-05-31 01:45:45, Jan Beulich wrote:
>> >>> On 31.05.17 at 09:30, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
>> > On 17-05-30 08:05:02, Jan Beulich wrote:
>> >> >>> On 03.05.17 at 10:44, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
>> >> 
>> >> > --- a/xen/arch/x86/psr.c
>> >> > +++ b/xen/arch/x86/psr.c
>> >> > @@ -476,23 +476,34 @@ static struct psr_socket_info 
> *get_socket_info(unsigned int socket)
>> >> >      return socket_info + socket;
>> >> >  }
>> >> >  
>> >> > +static struct feat_node *psr_get_feat_and_type(unsigned int socket,
>> >> > +                                               enum cbm_type type,
>> >> > +                                               enum psr_feat_type 
> *feat_type)
>> >> > +{
>> >> > +    const struct psr_socket_info *info = get_socket_info(socket);
>> >> > +
>> >> > +    if ( IS_ERR(info) )
>> >> > +        return ERR_PTR(PTR_ERR(info));
>> >> > +
>> >> > +    *feat_type = psr_cbm_type_to_feat_type(type);
>> >> > +    if ( *feat_type >= ARRAY_SIZE(info->features) )
>> >> > +        return NULL;
>> >> 
>> >> Note how this return is not being taken care of by ...
>> >> 
>> >> > +    return info->features[*feat_type];
>> >> > +}
>> >> > +
>> >> >  int psr_get_info(unsigned int socket, enum cbm_type type,
>> >> >                   uint32_t data[], unsigned int array_len)
>> >> >  {
>> >> > -    const struct psr_socket_info *info = get_socket_info(socket);
>> >> >      const struct feat_node *feat;
>> >> >      enum psr_feat_type feat_type;
>> >> >  
>> >> >      ASSERT(data);
>> >> >  
>> >> > -    if ( IS_ERR(info) )
>> >> > -        return PTR_ERR(info);
>> >> > -
>> >> > -    feat_type = psr_cbm_type_to_feat_type(type);
>> >> > -    if ( feat_type >= ARRAY_SIZE(info->features) )
>> >> > -        return -ENOENT;
>> >> > +    feat = psr_get_feat_and_type(socket, type, &feat_type);
>> >> > +    if ( IS_ERR(feat) )
>> >> > +        return PTR_ERR(feat);
>> >> 
>> >> ... the check here. I think you want to alter the return above.
>> >> 
>> > This NULL is taken care by below code:
>> >     if ( !feat || !feat_props[feat_type] ) 
>> 
>> Oh, I see.
>> 
>> > The returned errors are handled separately. For PTR_ERR, it is handled
>> > above. For NULL, it is handled below.
>> > 
>> > I checked IS_ERR, I think it can handle the NULL case. The NULL will not
>> > be treated as an error.
>> 
>> "it can handle the NULL case" is rather ambiguous: The NULL
>> case normally (including the case here) also is an error, and
>> IS_ERR() _does not_ detect this error. Hence using it one the
>> result of a function that may return NULL is at least
>> questionable (IS_ERR_OR_NULL() is intended to be used in such
>> cases).
>> 
>> So while indeed the code is correct as is, I'd still like to ask you
>> to make the suggested change so that the code also ends up
>> being visibly correct at the first glance.
>> 
> Thank you! Will use 'IS_ERR_OR_NULL()' to check result.

But that's not what I did suggest, and doing so will make the
handling at the checking site more clumsy.

Jan


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