Remove many write-only variables from kernel

[kan]

Summary

Reduce amount of warnings recent GCC versions spit while compiling
FreeBSD kernel.

[andrew]

Fell free to commit the sys/arm and sys/arm64 bits with Reviewed By: andrew

[kib]

I did not found anything obviously wrong from an eye inspection.

[jhibbits]

sys/cam/scsi/scsi_da.c

1509 ↗

(On Diff #27411)

Are we sure the return (0) down below is appropriate when there's an error?

[kan]

sys/cam/scsi/scsi_da.c

1509 ↗

(On Diff #27411)

Well, this is close path, it should not fail, even if cache flush fails.

[emaste]

sys/cam/scsi/scsi_da.c

1509 ↗

(On Diff #27411)

Either way it's a good reminder for anyone looking at this review to consider the changes carefully, in case the warning is actually giving a clue about a bug.

sys/security/mac/mac_syscalls.c

233 ↗

(On Diff #27411)

OK

sys/ufs/ffs/ffs_alloc.c

2446 ↗

(On Diff #27411)

OK

sys/ufs/ffs/ffs_vnops.c

1370 ↗

(On Diff #27411)

this one lgtm; maybe was cut-and-paste from similar functions?

[erj]

@sbruno is it ok to remove "first" in igb_txrx.c? It looks like it might be a leftover from the pre-iflib converted version of the driver, but I don't know if it was intended to be used for something in the current version.

[jhb]

In general these look fine to me. You can put me down as "Reviewed by:" for sys/dev/pci/* sys/kern/vfs_aio.c and sys/kern/kern_synch.c.

[jhb]

Oh, one other note is that there are several of these warnings in sys/riscv (I recently did some test builds of riscv since I broke it's build last week and noticed them), so if you are doing a sweep you might do a riscv build to find those.

[rwatson]

sys/netinet6/udp6_usrreq.c

1221 ↗

(On Diff #27411)

Surely the bug to fix here is a lack of "return (error)" rather than the setting of error?

[kan]

sys/netinet6/udp6_usrreq.c

1221 ↗

(On Diff #27411)

Is the error code _expected_ in this condition? That is surely the real question :)

[rwatson]

sys/netinet6/udp6_usrreq.c

1221 ↗

(On Diff #27411)

It looks like in UDPv4, an error is expected to be returned:

1898 static int
1899 udp_disconnect(struct socket *so)
1900 {
1901         struct inpcb *inp;
1902         struct inpcbinfo *pcbinfo;
1903 
1904         pcbinfo = udp_get_inpcbinfo(so->so_proto->pr_protocol);
1905         inp = sotoinpcb(so);
1906         KASSERT(inp != NULL, ("udp_disconnect: inp == NULL"));
1907         INP_WLOCK(inp);
1908         if (inp->inp_faddr.s_addr == INADDR_ANY) {
1909                 INP_WUNLOCK(inp);
1910                 return (ENOTCONN);
1911         }
1912         INP_HASH_WLOCK(pcbinfo);
1913         in_pcbdisconnect(inp);
1914         inp->inp_laddr.s_addr = INADDR_ANY;
1915         INP_HASH_WUNLOCK(pcbinfo);
1916         SOCK_LOCK(so);
1917         so->so_state &= ~SS_ISCONNECTED;                /* XXX */
1918         SOCK_UNLOCK(so);
1919         INP_WUNLOCK(inp);
1920         return (0);
1921 }

So it is likely a UDPv6 bug that the error is not properly returned.

[kan]

Return error from udp6_disconnect. sodisonnect translates it to 0 anyway,
but that keeps up conistend with UDPv4.

[kan]

sys/netinet6/udp6_usrreq.c

1221 ↗

(On Diff #27411)

Umm, but the code above, that does ipv4 fallback is explicitly ignoring pru_disconnect's return code:

(void)(*pru->pru_disconnect)(so);

[rwatson]

sys/netinet6/udp6_usrreq.c

1221 ↗

(On Diff #27411)

Sounds like another IPv6 bug, but perhaps out-of-scope for fixing in this particular patch. In general, I think we should be aiming for consistency with UDPv4 when it comes to UPv6 socket semantics, and differences are likely due to KAME bugs, changes made later to UDPv4 and not propagated to UDPv6, or bugs introduced in UDPv6 during fine-grained locking work. Thanks for making this change -- it's probably better than the alternative.

[ae]

sys/netinet6/in6_src.c

161 ↗

(On Diff #27424)

This look like the part of commented debugging code.

[vangyzen]

sys/cam/scsi/scsi_da.c

2305 ↗

(On Diff #27411)

Should methods be used to validate that the requested delete method is supported by the device? Something like this:

if ((i & methods) == 0)
        return (EINVAL);

sys/dev/vnic/nicvf_main.c

486 ↗

(On Diff #27411)

I think flags should be used here, since it represents the flags that are being changed:

if (flags & IFF_PROMISC) {

495 ↗

(On Diff #27411)

flags could be used here, too. (Perhaps modified_flags would be a better name.)

sys/dev/vnic/nicvf_queues.c

1721 ↗

(On Diff #27411)

One wonders if tail should be used in this loop. I have no idea.

sys/kern/kern_synch.c

178 ↗

(On Diff #27411)

p is used here #ifdef ktr.

251 ↗

(On Diff #27411)

p is used here #ifdef ktr.

sys/netinet6/nd6.c

2519 ↗

(On Diff #27411)

I'm not familiar with this, but I wonder if outifp should be used here instead of ifp. Can someone from network comment?

[vangyzen]

sys/kern/kern_synch.c

178 ↗

(On Diff #27411)

#ifdef KTR, that is.

251 ↗

(On Diff #27411)

#ifdef KTR, that is.

[sbruno]

In D10385#215182, @erj wrote:

@sbruno is it ok to remove "first" in igb_txrx.c? It looks like it might be a leftover from the pre-iflib converted version of the driver, but I don't know if it was intended to be used for something in the current version.

Yeah, it should be purged.

[erj]

With the answer from @sbruno, I approve of the change to em(4).

[emaste]

Perhaps commit the ones that have been explicitly acknowledged (e.g. drivers) and rebase?

[kan]

fixed kern_synch.c to use td->td_proc in one place where proc is needed.

sys/dev/vnic/nicvf_main.c

486 ↗

(On Diff #27411)

The body of IF blocks is ifdefed out anyway, so the code is not complete. I'll remove the _now_ unused variable anyway, if someone who'll be completing this implementation need it. they can safely resurrect it when actually needed.

sys/dev/vnic/nicvf_queues.c

1721 ↗

(On Diff #27411)

Again, as of not it is a pure write-only variable that is not affecting existing code. If one needs one in the future, one is welcome to resurrect it. This patch is not about implementing missing code.

[rmacklem]

The ones for nfs look ok. (I have not reviewed the rest of them.)
I would suggest that they be done as separate commits.