Page MenuHomeFreeBSD

Remove many write-only variables from kernel
ClosedPublic

Authored by kan on Apr 13 2017, 3:11 PM.

Details

Summary

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

Diff Detail

Event Timeline

kan created this revision.Apr 13 2017, 3:11 PM
andrew edited edge metadata.Apr 13 2017, 3:27 PM

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

kib accepted this revision.Apr 13 2017, 3:31 PM

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

jhibbits added inline comments.
sys/cam/scsi/scsi_da.c
1509

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

kan marked an inline comment as done.Apr 13 2017, 4:00 PM
kan added inline comments.
sys/cam/scsi/scsi_da.c
1509

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

emaste added inline comments.Apr 13 2017, 4:11 PM
sys/cam/scsi/scsi_da.c
1509

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

OK

sys/ufs/ffs/ffs_alloc.c
2446

OK

sys/ufs/ffs/ffs_vnops.c
1370

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

erj added a subscriber: erj.Apr 13 2017, 4:49 PM

@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 edited edge metadata.Apr 13 2017, 4:58 PM

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 added a comment.Apr 13 2017, 4:59 PM

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 added inline comments.
sys/netinet6/udp6_usrreq.c
1221

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

kan marked an inline comment as done.Apr 13 2017, 6:19 PM
kan added inline comments.
sys/netinet6/udp6_usrreq.c
1221

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

rwatson added inline comments.Apr 13 2017, 6:33 PM
sys/netinet6/udp6_usrreq.c
1221

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 updated this revision to Diff 27424.Apr 13 2017, 7:20 PM
kan edited edge metadata.
kan marked an inline comment as done.

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

kan added inline comments.Apr 13 2017, 7:28 PM
sys/netinet6/udp6_usrreq.c
1221

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

(void)(*pru->pru_disconnect)(so);
rwatson added inline comments.Apr 13 2017, 7:38 PM
sys/netinet6/udp6_usrreq.c
1221

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 added inline comments.Apr 13 2017, 9:20 PM
sys/netinet6/in6_src.c
161

This look like the part of commented debugging code.

vangyzen added inline comments.
sys/cam/scsi/scsi_da.c
2305

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

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

if (flags & IFF_PROMISC) {
495

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

sys/dev/vnic/nicvf_queues.c
1721

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

sys/kern/kern_synch.c
178

p is used here #ifdef ktr.

251

p is used here #ifdef ktr.

sys/netinet6/nd6.c
2519

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

vangyzen added inline comments.Apr 13 2017, 10:25 PM
sys/kern/kern_synch.c
178

#ifdef KTR, that is.

251

#ifdef KTR, that is.

sbruno edited edge metadata.Jun 17 2017, 8:24 PM
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 accepted this revision.Jul 13 2017, 8:45 PM

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

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

kan added a comment.Dec 25 2017, 4:25 AM

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

sys/dev/vnic/nicvf_main.c
486

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

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.

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

This revision was not accepted when it landed; it landed in state Needs Review.Jan 16 2018, 4:24 AM
This revision was automatically updated to reflect the committed changes.