Skip to content

impl Drop for FileDescriptor/OwnedFileDescriptor#84

Merged
jkcoxson merged 3 commits intojkcoxson:masterfrom
abdullah-albanna:master
Apr 9, 2026
Merged

impl Drop for FileDescriptor/OwnedFileDescriptor#84
jkcoxson merged 3 commits intojkcoxson:masterfrom
abdullah-albanna:master

Conversation

@abdullah-albanna
Copy link
Copy Markdown
Contributor

No description provided.

@jkcoxson
Copy link
Copy Markdown
Owner

jkcoxson commented Apr 5, 2026

I've just created a test harness, do you mind running the tests to make sure everything still works? This code adds unsafe, so I want the afc test suite to be run beforehand.

@abdullah-albanna
Copy link
Copy Markdown
Contributor Author

yeah it works just fine

you wouldn't notice anything even if it's broken, because drop doesn't panic in here

you should test the dropping, like get the fd out, and drop it and try to open it again, if it panics then it's good

@abdullah-albanna
Copy link
Copy Markdown
Contributor Author

I'm not sure about the tokio::task::block_in_place and if it's appropriate for Drop

you can keep this pr open until you feel comfortable, or someone else confirms it's okay

@jkcoxson
Copy link
Copy Markdown
Owner

jkcoxson commented Apr 7, 2026

Do you mind upstreaming your changes so I can test it with the new harness? I can't do it myself since your branch is also master.

@jkcoxson
Copy link
Copy Markdown
Owner

jkcoxson commented Apr 7, 2026

I added a drop so we don't mutably borrow twice and tested it on the vphone test harness. If you agree with my change, we can merge this.

@abdullah-albanna
Copy link
Copy Markdown
Contributor Author

ok

@jkcoxson jkcoxson merged commit e267d8e into jkcoxson:master Apr 9, 2026
3 checks passed
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