[Swan-dev] why there were useless assignments

[Antony Antony]

I am not familiar with this specific case, however, I know a similar one in 
kernel_netlink.c. DHR are these two cases the same?

I think we grand-fathered such warning(s) in kernel_netlink.c and few other 
ones too.? The new ones are getting removed my magic coverity fairy at the 
last minute, just before a release. At that moment it becomes no more new 
warnings instead thinking merits of such assignments?

Indeed such unused assignments are very useful. It would probably avoid many 
pitfalls. It saved me many times. 

Here is a scan-build output:
"Dead store Dead increment programs/pluto/kernel_netlink.c netlink_add_sa	
1580	"
https://swantest.libreswan.fi/scan-build-2019-02-21-084017-27164-1/report-8916ad.html#EndPath 

offending line
1580 attr = (struct rtattr *)((char *)attr + attr->rta_len);

And the one that started this thread is:
Dead assignment	lib/libswan/proposals.c	fmt_proposal	401
https://swantest.libreswan.fi/scan-build-2019-02-21-084017-27164-1/report-a4e611.html#EndPath

scan-build report 9 "Dead assignment" and one "Dead increment". Coverity may 
not agree 100% with scan-build. Coverity can compare previous scan outputs.
However, I am yet to find an easy way to compare scan-build outputs and 
summarize what is new.

There is one case where I wonder "not subsequently used" makes clutter.
The scans would report unused assignment on different lines
based on what is defined and what is not example lets scan with
USE_LABELED_IPSEC=no

Note the kernel_netlink.c:
1559 #ifdef HAVE_LABELED_IPSEC

When labeled ipsec is disabled the warning would move.

Dead increment	programs/pluto/kernel_netlink.c	netlink_add_sa	1556
https://swantest.libreswan.fi/scan-build-2019-02-21-090346-28362-1/report-e02073.html#EndPath

So the question that  arise is should we add the comment, 

/*XXX  not subsequently used */"  more than once?


1556 attr = (struct rtattr *)((char *)attr + attr->rta_len); /* attr not 
subsequently used */"
1580  attr = (struct rtattr *)((char *)attr + attr->rta_len); /* attr not 

That would look a bit strange:) Still I thin it is a good practice. I 
imagine in some cases a macro would help too.
I am playing with one for netlink use, for the code in xfrmi. Without 
freaking out macro police.

Also new "Dead assignment" would pop up with some features disabled.
I recollect a bit more nasty case when USE_DNSSEC=false. I haven't checked 
it recently, if it is still there.

On Wed, Feb 20, 2019 at 03:56:15PM -0500, D. Hugh Redelmeier wrote:
> Andrew has introduced several.  Here's one:
> 
> /home/hugh/libreswan/lib/libswan/proposals.c:401:21: warning: Value stored to 'ps' is never read
> 		lswlogs(log, as); ps = "-"; as = "+";
>                                   ^    ~~~

I suspect at the last minute, just before release, coverity fairy would 
remove this. And next cycle they get added again:)

> I like this useless assignment.  As I argued below.

How about having a practice? I am sure DHR magic would monitor them,
if we don't violently discard them.

size += lswlogs(log, sep); /* sep = "-"; sep not subsequently used */

or

size += lswlogs(log, sep); 
/* sep = "-"; sep not subsequently used */

If it is on the same line, the line length fairy would be sad.