logportmap.c:413]: (style) Suspicious condition

[dcb314]

Source code is

if ((sts = pmFetch(1, &pmid, &res) < 0))
goto ctxErr;

Maybe better code

if ((sts = pmFetch(1, &pmid, &res)) < 0)
goto ctxErr;

[fche]

Yup. (There's too much of that type of overly compacted pattern in the code for my taste.)

[kmcdonell]

Fixed upstream in my tree.

[kmcdonell]

@dcb314 ... based on your other github postings, I'm guessing you have a tool that you use to find this class of problem. Is that so? And if so, have you run it across all of the PCP code and this is the only botch discovered in the process?

Cheers and thanks.

[dcb314]

I'm guessing you have a tool that you use to find this class of problem. Is that so?

Yes, a static analyser called cppcheck, available from sourceforge.

have you run it across all of the PCP code

Not sure, but certainly the PCP code that appears in Fedora Rawhide.

is the only botch discovered in the process?

Oh no. There are many errors, warnings, style problems and performance problems
found by cppcheck. Only some of them are worth fixing ;->

Here are the errors:

[p_result.c:207]: (error) Uninitialized variable: pp
[p_result.c:211]: (error) Uninitialized variable: pp
[util.c:1796]: (error) Memory leak: names
[logutil.c:1496]: (error) Memory leak: pr

The warnings:

[p_result.c:235]: (warning) Suspicious code: sign conversion of - in calculation
, even though - can have a negative value
[pdubuf.c:156] -> [pdubuf.c:155]: (warning) Either the condition 'bcp!=0' is red
undant or there is possible null pointer dereference: bcp.
[pdubuf.c:205] -> [pdubuf.c:204]: (warning) Either the condition 'bcp!=0' is red
undant or there is possible null pointer dereference: bcp.
[pmns.c:497]: (warning) scanf without field width limits can crash with huge input data.
[spec.c:668] -> [spec.c:560]: (warning) Either the condition 'hsp==0' is redunda
nt or there is possible null pointer dereference: specp.
[spec.c:669] -> [spec.c:560]: (warning) Either the condition 'hsp==0' is redunda
nt or there is possible null pointer dereference: specp.
[spec.c:670] -> [spec.c:560]: (warning) Either the condition 'hsp==0' is redunda
nt or there is possible null pointer dereference: specp.
[spec.c:671] -> [spec.c:560]: (warning) Either the condition 'hsp==0' is redunda
nt or there is possible null pointer dereference: specp.
[spec.c:672] -> [spec.c:560]: (warning) Either the condition 'hsp==0' is redunda
nt or there is possible null pointer dereference: specp.
[spec.c:673] -> [spec.c:560]: (warning) Either the condition 'hsp==0' is redunda
nt or there is possible null pointer dereference: specp.
[optfetch.c:435]: (warning) %x in format string (no. 2) requires 'unsigned int'
but the argument type is 'fetchctl_t *'.
[optfetch.c:461]: (warning) %x in format string (no. 4) requires 'unsigned int'
but the argument type is 'fetchctl_t *'.
[discovery.c:308]: (warning) %u in format string (no. 3) requires 'unsigned int'
but the argument type is 'int'.
[discovery.c:310]: (warning) %u in format string (no. 3) requires 'unsigned int'
but the argument type is 'int'.
[events.c:34]: (warning) %lu in format string (no. 1) requires 'unsigned long' b
ut the argument type is 'size_t {aka unsigned long}'.
[secureserver.c:515] -> [secureserver.c:513]: (warning) Either the condition '(u
sername=strdup(username))==0' is redundant or there is possible null pointer der
eference: username.
[secureconnect.c:727]: (warning) %d in format string (no. 6) requires 'int' but
the argument type is 'unsigned int'.
[getdate.tab.c:1082]: (warning) Assignment of function parameter has no effect o
utside the function. Did you forget dereferencing it?

There are 33 style warnings, too many to list here.

[kmcdonell]

Unfortunately, the 4 "errors" above are all false positives ... so I'm not sure how much trust I'd place in cppcheck.

[fche]

The util.c:1796 memory leak seems legit; if names = realloc() succeeds but the [n-1]=malloc() fails, then names[] is unfreed.

Similarly, logutil.c:1516, that error could result in pr being unfreed.

[kmcdonell]

OK Frank ... the util.c one also means I should first free names[j] ... 0<=j<n-1 and then free names ... but this is a bit Nero-like in as much as the inner malloc() failing probably means the sky is about to fall in, so tossing a few bytes back into the pool probably won't prevent the end of the world.

The logutil.c one is legit, I agree.

[dcb314]

Unfortunately, the 4 "errors" above are all false positives ... so I'm not sure how much trust I'd place >in cppcheck.

It's not perfect, but I find that anything it has got to say is usually worth looking at more closely.

Flag --enable=all makes it say more than the default setting.