Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

port_create race test and fix #215

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

redpig
Copy link

@redpig redpig commented Dec 22, 2017

port_create() has a known race condition in name reservation because of the separation of name uniqueness validation and name addition. This pull request adds a test that verifies the race is reachable, fixes the race, and updates the test with slight changes to support it.

This change adds a test to the port_tests suite
to trigger the port_create() race.  When tested with
an SMP (3) qemu instance, the race is discovered in
less than twenty iterations and often much sooner.
In the current design, ports may be created with the same name
because of time-of-check-time-of-use behavior.

This change adds a new port magic, PORTHOLD_MAGIC, which cannot be
accessed using the normal accessors. It is injected into the list
upon a successful, locked search for a name.  It holds the name
until the allocation happens. At which point the port-hold is
deleted and the real port is added while, again, under the thread
lock.

This change assumes using a stack-allocated port is desirable over
preallocating and freeing on each port_create().  If the overhead
of allocation is low, then pre-allocating the new port would be
the cleanest option: (1) allocate, (2) check for collisions,
(3) add to list.

This fix is confirmed by the two_threads_race test completing its
256 iterations without a race. It also fixes the test to expect
ERR_BUSY and to exit cleanly, as it had never completed before.
@@ -291,6 +291,244 @@ static int ping_pong_thread(void *arg)
return __LINE__;
}

const char *kStatusPortNames[] = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sure all of these are static so the compiler/linker will strip them if not building with the console.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reposting with that change now -- thanks!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Some of the functions in there can be made static too and I can make/test that for this pull or just do a second. Whichever works!)

This change makes the shared constants static
as well as const-ifies shared packets.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants