Re: [PATCH v3 2/4] vhost-vdpa: introduce STOP backend feature bit

[Dan Carpenter <dan.carpenter@oracle.com>]

On Thu, May 26, 2022 at 02:44:02PM +0200, Eugenio Perez Martin wrote:
> > >> +static bool vhost_vdpa_can_stop(const struct vhost_vdpa *v) {
> > >> +       struct vdpa_device *vdpa = v->vdpa;
> > >> +       const struct vdpa_config_ops *ops = vdpa->config;
> > >> +
> > >> +       return ops->stop;
> > >> [GD>>] Would it be better to explicitly return a bool to match the return type?
> > >
> > >I'm not sure about the kernel code style regarding that casting. Maybe
> > >it's better to return !!ops->stop here. The macros likely and unlikely
> > >do that.
> >
> > IIUC `ops->stop` is a function pointer, so what about
> >
> >      return ops->stop != NULL;
> >
> 
> I'm ok with any method proposed. Both three ways can be found in the
> kernel so I think they are all valid (although the double negation is
> from bool to integer in (0,1) set actually).
> 
> Maybe Jason or Michael (as maintainers) can state the preferred method here.

Always just do whatever the person who responded feels like because
they're likely the person who cares the most.  ;)

I don't think there are any static analysis tools which will complain
about this.  Smatch will complain if you return a negative literal.
It feels like returning any literal that isn't 1 or 0 should trigger a
warning...  I've written that and will check it out tonight.

Really anything negative should trigger a warning.  See new Smatch stuff
below.

regards,
dan carpenter

================ TEST CASE =========================

int x;
_Bool one(int *p)
{
        if (p)
                x = -2;
        return x;
}
_Bool two(int *p)
{
        return -4;  // returning 2 triggers a warning now
}

=============== OUTPUT =============================

test.c:10 one() warn: potential negative cast to bool 'x'
test.c:14 two() warn: signedness bug returning '(-4)'
test.c:14 two() warn: '(-4)' is not bool

=============== CODE ===============================

#include "smatch.h"
#include "smatch_extra.h"
#include "smatch_slist.h"

static int my_id;

static void match_literals(struct expression *ret_value)
{
	struct symbol *type;
	sval_t sval;

	type = cur_func_return_type();
	if (!type || sval_type_max(type).value != 1)
		return;

	if (!get_implied_value(ret_value, &sval))
		return;

	if (sval.value == 0 || sval.value == 1)
		return;

	sm_warning("'%s' is not bool", sval_to_str(sval));
}

static void match_any_negative(struct expression *ret_value)
{
	struct symbol *type;
	struct sm_state *extra, *sm;
	sval_t sval;
	char *name;

	type = cur_func_return_type();
	if (!type || sval_type_max(type).value != 1)
		return;

	extra = get_extra_sm_state(ret_value);
	if (!extra)
		return;
	FOR_EACH_PTR(extra->possible, sm) {
		if (estate_get_single_value(sm->state, &sval) &&
		    sval_is_negative(sval)) {
			name = expr_to_str(ret_value);
			sm_warning("potential negative cast to bool '%s'", name);
			free_string(name);
			return;
		}
	} END_FOR_EACH_PTR(sm);
}

void check_bool_return(int id)
{
	my_id = id;

	add_hook(&match_literals, RETURN_HOOK);
	add_hook(&match_any_negative, RETURN_HOOK);
}

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization