Skip to content

Commit

Permalink
warnings cleanup: hdb+loop_timerlist: Wsign-compare: (canary?) variables
Browse files Browse the repository at this point in the history
In case of hdb.c, the problem is that `check` public API (qb_hdb_handle
member struct) item (which should not be exposed publicly like this
in the first place!) is typed as `int32_t`, whereas it was to be
compared to `uint32_t` implementation-possessed local variables
(presumably derived from the same source), which made the compiler
upset (even though there was no real reason, integer promotion to
unsigned type would happily occur, which is furthermore expected to
be fully defined as these values come from `random` that shall
return non-negative integers below `INT32_MAX`).

Hence:
- type these local variables to `int32_t` just as well, which allows to
- simplify `random` return value handling, since they are expected to be
  zero-or-greater and the previously extra tested all-bits-on pattern
  makes undoubtfully for a negative numeric value in case of a signed
  integer with specified width (c.f. 7.18.1.1/C99), hence falling into
  complement of zero-or-greater; zero itself is also excluded for the
  reasons stated in the comment (which was pretty hazy and incorrect,
  so it gets overhaul as well)
- also superfluous typecasts are removed

Similar situation is with loop_timerlist.c, where we are actually fully
in charge of the struct member (private API), but there are good reasons
to stay consistent with the former file as the same applies to the
source of that value -- it comes from `random` (equivalent comment
is added here for greater symmetry).

Signed-off-by: Jan Pokorný <jpokorny@redhat.com>
  • Loading branch information
jnpkrn committed Dec 20, 2017
1 parent aed01bb commit cfb15c7
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 19 deletions.
29 changes: 15 additions & 14 deletions lib/hdb.c
Expand Up @@ -60,7 +60,7 @@ qb_hdb_handle_create(struct qb_hdb *hdb, int32_t instance_size,
{
int32_t handle;
int32_t res = 0;
uint32_t check;
int32_t check;
int32_t found = QB_FALSE;
void *instance;
int32_t i;
Expand Down Expand Up @@ -98,15 +98,16 @@ qb_hdb_handle_create(struct qb_hdb *hdb, int32_t instance_size,
}

/*
* This code makes sure the random number isn't zero
* We use 0 to specify an invalid handle out of the 1^64 address space
* If we get 0 200 times in a row, the RNG may be broken
* Make sure just positive integers are used for the integrity(?)
* checks within 2^32 address space, if we miss 200 times in a row
* (just 0 is concerned per specification of random), the PRNG may be
* broken -> the value is unspecified, subject of stack allocation.
*/
for (i = 0; i < 200; i++) {
check = random();

if (check != 0 && check != UINT32_MAX) {
break;
if (check > 0) {
break; /* covers also check == UINT32_MAX */
}
}

Expand All @@ -125,7 +126,7 @@ qb_hdb_handle_create(struct qb_hdb *hdb, int32_t instance_size,
int32_t
qb_hdb_handle_get(struct qb_hdb * hdb, qb_handle_t handle_in, void **instance)
{
uint32_t check = ((uint32_t) (((uint64_t) handle_in) >> 32));
int32_t check = handle_in >> 32;
uint32_t handle = handle_in & UINT32_MAX;
struct qb_hdb_handle *entry;
int32_t handle_count;
Expand All @@ -143,7 +144,7 @@ qb_hdb_handle_get(struct qb_hdb * hdb, qb_handle_t handle_in, void **instance)
return (-EBADF);
}

if (check != UINT32_MAX && check != entry->check) {
if (check != (int32_t) UINT32_MAX && check != entry->check) {
return (-EBADF);
}
qb_atomic_int_inc(&entry->ref_count);
Expand All @@ -163,7 +164,7 @@ qb_hdb_handle_get_always(struct qb_hdb * hdb, qb_handle_t handle_in,
int32_t
qb_hdb_handle_put(struct qb_hdb * hdb, qb_handle_t handle_in)
{
uint32_t check = ((uint32_t) (((uint64_t) handle_in) >> 32));
int32_t check = handle_in >> 32;
uint32_t handle = handle_in & UINT32_MAX;
struct qb_hdb_handle *entry;
int32_t handle_count;
Expand All @@ -176,7 +177,7 @@ qb_hdb_handle_put(struct qb_hdb * hdb, qb_handle_t handle_in)
}

if (qb_array_index(hdb->handles, handle, (void **)&entry) != 0 ||
(check != UINT32_MAX && check != entry->check)) {
(check != (int32_t) UINT32_MAX && check != entry->check)) {
return (-EBADF);
}

Expand All @@ -193,7 +194,7 @@ qb_hdb_handle_put(struct qb_hdb * hdb, qb_handle_t handle_in)
int32_t
qb_hdb_handle_destroy(struct qb_hdb * hdb, qb_handle_t handle_in)
{
uint32_t check = ((uint32_t) (((uint64_t) handle_in) >> 32));
int32_t check = handle_in >> 32;
uint32_t handle = handle_in & UINT32_MAX;
int32_t res;
struct qb_hdb_handle *entry;
Expand All @@ -207,7 +208,7 @@ qb_hdb_handle_destroy(struct qb_hdb * hdb, qb_handle_t handle_in)
}

if (qb_array_index(hdb->handles, handle, (void **)&entry) != 0 ||
(check != UINT32_MAX && check != entry->check)) {
(check != (int32_t) UINT32_MAX && check != entry->check)) {
return (-EBADF);
}

Expand All @@ -219,7 +220,7 @@ qb_hdb_handle_destroy(struct qb_hdb * hdb, qb_handle_t handle_in)
int32_t
qb_hdb_handle_refcount_get(struct qb_hdb * hdb, qb_handle_t handle_in)
{
uint32_t check = ((uint32_t) (((uint64_t) handle_in) >> 32));
int32_t check = handle_in >> 32;
uint32_t handle = handle_in & UINT32_MAX;
struct qb_hdb_handle *entry;
int32_t handle_count;
Expand All @@ -233,7 +234,7 @@ qb_hdb_handle_refcount_get(struct qb_hdb * hdb, qb_handle_t handle_in)
}

if (qb_array_index(hdb->handles, handle, (void **)&entry) != 0 ||
(check != UINT32_MAX && check != entry->check)) {
(check != (int32_t) UINT32_MAX && check != entry->check)) {
return (-EBADF);
}

Expand Down
16 changes: 11 additions & 5 deletions lib/loop_timerlist.c
Expand Up @@ -35,7 +35,7 @@ struct qb_loop_timer {
enum qb_loop_priority p;
timer_handle timerlist_handle;
enum qb_poll_entry_state state;
uint32_t check;
int32_t check;
uint32_t install_pos;
};

Expand Down Expand Up @@ -123,15 +123,15 @@ _timer_from_handle_(struct qb_timer_source *s,
struct qb_loop_timer **timer_pt)
{
int32_t rc;
uint32_t check;
int32_t check;
uint32_t install_pos;
struct qb_loop_timer *timer;

if (handle_in == 0) {
return -EINVAL;
}

check = ((uint32_t) (((uint64_t) handle_in) >> 32));
check = handle_in >> 32;
install_pos = handle_in & UINT32_MAX;

rc = qb_array_index(s->timers, install_pos, (void **)&timer);
Expand Down Expand Up @@ -202,11 +202,17 @@ qb_loop_timer_add(struct qb_loop * lp,
t->p = p;
qb_list_init(&t->item.list);

/*
* Make sure just positive integers are used for the integrity(?)
* checks within 2^32 address space, if we miss 200 times in a row
* (just 0 is concerned per specification of random), the PRNG may be
* broken -> the value is unspecified, subject of previous assignment.
*/
for (i = 0; i < 200; i++) {
t->check = random();

if (t->check != 0 && t->check != UINT32_MAX) {
break;
if (t->check > 0) {
break; /* covers also t->check == UINT32_MAX */
}
}

Expand Down

0 comments on commit cfb15c7

Please sign in to comment.