Page MenuHomeFreeBSD

if_vmove: return proper error status
ClosedPublic

Authored by kevans on Dec 12 2019, 2:33 PM.

Details

Summary

if_vmove can fail if it lost a race and the vnet's already been moved. The callers (and their callers) can generally cope with this, but right now success is assumed. Plumb out the ENOENT from if_detach_internal if it happens so that the error's properly reported to userland.

This is a prereq to work that adds a busy mechanism to ifnet and an additional constraint to if_vmove that attempting to if_vmove on a detaching or in-motion (vnet-wise) ifnet will return EBUSY.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kevans created this revision.Dec 12 2019, 2:33 PM
kp added a comment.Dec 12 2019, 7:12 PM

There's another call to if_vmove() in vnet_if_return(). We probably can't handle errors there (and I expect that D22691 will ensure it can't fail), but we should at least assert that it succeeded.

In D22780#498714, @kp wrote:

There's another call to if_vmove() in vnet_if_return(). We probably can't handle errors there (and I expect that D22691 will ensure it can't fail), but we should at least assert that it succeeded.

Yeah, so I was kinda worried about asserting it without the additional synchronization provided by D22691 -- I suspect we'll get an easy panic without it, since there's absolutely no coordination between vnet shutdown and if_detach. With this patch alone, failure is actually somewhat tolerable in that case because failure means the ifnet is no longer in our vnet, which is all we're really trying to achieve. =-D D22691 should likely add the assert, since the theory is that either we definitively know the ifnet is being detached by another thread *or* we're responsible for doing it and we must not fail.

kp accepted this revision.Dec 12 2019, 8:14 PM
This revision is now accepted and ready to land.Dec 12 2019, 8:14 PM
bz accepted this revision.Thu, Jan 2, 10:34 AM

Looks good to me.

This revision was automatically updated to reflect the committed changes.