Flux-core: cppcheck: [src/modules/kvs/kvstxn.c:509]: (error) Uninitialized variable: ref

[grondo]

The newer cppcheck in Ubuntu bionic flagged some new uninitialized variable warnings. Most of them are pretty straightforward, but this one from kvstxn_append() didn't seem clear to me on a quick pass anyway.

I'm not sure to what ref should be initialized -- or if ref is actually returned by value in kvstxn_val_data_to_cache() then I think cppcheck is confused by the blobref_t type hiding a pointer
(I was actually confused for a minute too, and might still be!) and this is a false positive.

    else if (treeobj_is_valref (entry)) {
        blobref_t ref;
        json_t *cpy;

        /* treeobj is valref, so we need to append the new data's
         * blobref to this tree object.  Before doing so, we must save
         * off the new data to the cache and mark it dirty for
         * flushing later (if necessary)
         *
         * Note that we make a copy of the original entry and
         * re-insert it into the directory.  We do not want to
         * accidentally alter any json object pointers that may be
         * sitting in the KVS cache.
         */

        if (kvstxn_val_data_to_cache (kt, current_epoch, dirent, ref) < 0)
            return -1;                                  

[grondo]

cppcheck also flags other places where an uninitialized blobref_t is passed to a function that I guess is supposed to fill in the blobref on return. I think perhaps cppcheck configuration which defines the blobref_t type for cppcheck would help suppress these errors. (nevermind: that might only work for platform-independent typedefs, I misread the docs)

[grondo]

Actually I'm thinking maybe we disable cppcheck in travis for now. I'm also seeing some unitialized variable warnings in json_object_foreach and json_array_foreach which are clearly false positives because cppcheck doesn't know the definition of these external macros. Cppcheck does have some basic ability to configure function definitions, but I'm unable to tell it explicitly that argument initialization is a side-effect of these functions, and unnecessarily initializing variables or adding inline suppressions just seems tedious.

Thoughts?

[garlick]

Ultimately kvstxn_val_data_to_cache() passes ref to blobref_hash(). It's an output variable with a fixed size:

typedef char blobref_t[BLOBREF_MAX_STRING_SIZE];

/* Compute hash over data and return null-terminated blobref string in
 * 'blobref'.  The hash algorithm is selected by 'hashtype', e.g. "sha1".
 * Returns 0 on success, -1 on error with errno set.
 */
int blobref_hash (const char *hashtype,
                  const void *data, int len,
                  blobref_t blobref);

Possibly the blobref_t type should not have been defined, and we should be passing around a char *buf, int len instead....

I just did a similar thing in kvs_eventlog.h

typedef char flux_kvs_event_name_t[65];
typedef char flux_kvs_event_context_t[257];

Anti-pattern?

[grondo]

Anti-pattern?

Hm, I don't feel qualified to answer that one, but it does seem confusing to me.

Here's a stackoverflow answer that also suggests hiding the fact that a type is an array type is possibly a bad idea.

[garlick]

Right, it seems in line with our "don't hide struct *" project standard to also not hide arrays. Two examples given in that stackoverflow article of the pitfalls of hiding this are:

• sizeof may give the wrong result
• returning a value of that type won't work

It would probably be more clear to define these arrays outright with BLOBREF_MAX_STRING_SIZE and pass the sizeof (array) in with the array pointer, since that's the more common, straightforward idiom. It's a false economy to define those types at the expense of being clear IMHO.

[chu11]

Should this be closed with #1692 closed? Or should we leave open until #1702 / #1703 are completed?

[chu11]

i think this is done