The 12/08/2022 18:59, Dan Carpenter wrote:
Hi Dan,
>
> Hello Horatiu Vultur,
>
> The patch 610c32b2ce66: "net: microchip: vcap: Add vcap_get_rule"
> from Dec 3, 2022, leads to the following Smatch static checker
> warning:
>
> drivers/net/ethernet/microchip/vcap/vcap_api_debugfs.c:275 vcap_show_admin()
> warn: passing zero to 'PTR_ERR'
>
> drivers/net/ethernet/microchip/vcap/vcap_api_debugfs.c
> 263 static int vcap_show_admin(struct vcap_control *vctrl,
> 264 struct vcap_admin *admin,
> 265 struct vcap_output_print *out)
> 266 {
> 267 struct vcap_rule_internal *elem;
> 268 struct vcap_rule *vrule;
> 269 int ret = 0;
> 270
> 271 vcap_show_admin_info(vctrl, admin, out);
> 272 list_for_each_entry(elem, &admin->rules, list) {
> 273 vrule = vcap_get_rule(vctrl, elem->data.id);
> 274 if (IS_ERR_OR_NULL(vrule)) {
> --> 275 ret = PTR_ERR(vrule);
> 276 break;
> 277 }
>
> There aren't any comments explaing what a NULL means...
>
> Intuitively this doesn't feel like the correct way to handle NULL
> returns. It feels like it should be:
Thanks for reporting this.
You are right, it is not really a hard error if the vrule is NULL.
We are planning to change a little bit the things here, as the locking
is not exactly correct.
>
> vrule = vcap_get_rule(vctrl, elem->data.id);
> if (!vrule)
> continue;
> if (IS_ERR(vrule)) {
> ret = PTR_ERR(vrule);
> break;
> }
>
> 278
> 279 out->prf(out->dst, "\n");
> 280 vcap_show_admin_rule(vctrl, admin, out, to_intrule(vrule));
> 281 vcap_free_rule(vrule);
> 282 }
> 283 return ret;
> 284 }
>
> regards,
> dan carpenter
--
/Horatiu
