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

OS-8007 want PORT_SOURCE_DEVICE for device-specific port events #227

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

joyent-automation
Copy link

OS-8007 want PORT_SOURCE_DEVICE for device-specific port events

This PR was migrated-from-gerrit, https://cr.joyent.us/#/c/6948/.
The raw archive of this CR is here.
See MANTA-4594 for info on Joyent Eng's migration from Gerrit.

CR discussion

@johnlevon commented at 2019-10-09T17:35:48

Patch Set 1:

(12 comments)

Patch Set 1 code comments
usr/src/man/man3c/port_associate.3c#159 @johnlevon

nit, s/in in/in /

usr/src/man/man3c/port_create.3c#109 @johnlevon

I'm unclear on the fork semantics of device ports, given that we cache the pid. For example, a child can't dissociate a port.

usr/src/uts/common/fs/portfs/port.c#85 @johnlevon

this is missing PORT_SOURCE_DEV

usr/src/uts/common/fs/portfs/port.c#194 @johnlevon

this is missing PORT_SOURCE_DEV

usr/src/uts/common/fs/portfs/port.c#239 @johnlevon

this is missing PORT_SOURCE_DEV

usr/src/uts/common/fs/portfs/port_dev.c#19 @johnlevon

We could do with a rationale here: I'm guessing something just pointing out that poll+ioctl means a lot of hand-written infrastructure better served by a port source

usr/src/uts/common/fs/portfs/port_dev.c#99 @johnlevon

nit, missing space after 'the'

usr/src/uts/common/fs/portfs/port_dev.c#157 @johnlevon

the naming here is pretty confusing:

port_dev_hash_get() - get the hash table
port_dev_hash_destroy() - destroy the hash table
port_dev_hash_delete() - internal helper to remove an item from the hash
port_dev_hash_find*() - find an item in the hash
port_dev_hash_insert() - insert an item into the hash

Could we maybe rename the first two? port_dev_hashtable_ maybe? Or remove hash from the others?

usr/src/uts/common/fs/portfs/port_dev.c#210 @johnlevon

it's kind of nasty that we have to manage our own lists here.
Why aren't we using mod_hash_create_extended() with a key type of object+pid ?

usr/src/uts/common/fs/portfs/port_dev.c#380 @johnlevon

I'm not following the "sizeof (port_dev_t *)" here, is that from a previous version?

usr/src/uts/common/fs/portfs/port_dev.c#644 @johnlevon

We don't hold the device in any way while we have active
associations. What is stopping the device from detaching here while such ports are active? Can't ->p_ops become stale then?

Basically, I don't understand the relationship between a particular driver's minor device lifetime, and the lifetime of a particular port associated with it, and the user objects associated on top.

usr/src/uts/common/os/port_subr.c#522 @johnlevon

This was changed, but :536 and co wasn't. Can we do one or the either, and get rid of :517-:518

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