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

Implement fmemopen #875

Merged
merged 4 commits into from
Feb 20, 2024
Merged

Implement fmemopen #875

merged 4 commits into from
Feb 20, 2024

Conversation

no92
Copy link
Member

@no92 no92 commented Jun 8, 2023

Supersedes #540.
Fixes #527.

@no92 no92 marked this pull request as draft June 10, 2023 13:43
@no92 no92 force-pushed the fmemopen branch 5 times, most recently from 3fa3ad1 to d52b068 Compare June 10, 2023 18:43
@no92 no92 force-pushed the fmemopen branch 3 times, most recently from 53deb63 to a9d514e Compare July 12, 2023 12:52
@no92 no92 force-pushed the fmemopen branch 3 times, most recently from eb42301 to fea5525 Compare August 11, 2023 11:27
@no92
Copy link
Member Author

no92 commented Aug 11, 2023

The failing assert will not be done on glibc due to (incorrectly) failing there. NetBSD's libc tests also guard against this in the same way. My implementation is adhering to POSIX, and thus correct; musl and the BSDs agree with me here. This should be ready for review now.

@no92 no92 marked this pull request as ready for review August 11, 2023 11:32
Copy link
Contributor

@ikbenlike ikbenlike left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@Geertiebear Geertiebear left a comment

Choose a reason for hiding this comment

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

Haven't taken a good look at this yet, will do later, but just a nitpick already when skimming it over: the comments aren't in the same style :) In general we follow that comments should // Look like this., if you could fix that in the relevant places that would be really nice.

@no92
Copy link
Member Author

no92 commented Aug 12, 2023

Fixed the comments.

Copy link
Member

@Geertiebear Geertiebear left a comment

Choose a reason for hiding this comment

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

In general, I'm not a fan of this code. It seems that the _allowResizing flag was used to cram two different classes into one implementation, which makes for a very confusing read of the code. In this case it would be appropriate to have two different classes, and possibly a common superclass that abstracts some of the common behaviour away. Right now (what should be) virtual calls are emulated using the _allowResizing flag.

In summary, please add missing test cases, since there were several untested cases I've noticed. And separate the two different implementations using inheritance, this will clean up the implementation and hopefully reduce the likelihood of bugs.

options/posix/generic/posix-file-io.cpp Show resolved Hide resolved
tests/posix/fmemopen.c Show resolved Hide resolved
tests/posix/fmemopen.c Show resolved Hide resolved
options/posix/include/mlibc/posix-file-io.hpp Outdated Show resolved Hide resolved
@no92 no92 force-pushed the fmemopen branch 2 times, most recently from 5b70b30 to c7f343a Compare August 15, 2023 20:07
Copy link
Member

@Geertiebear Geertiebear left a comment

Choose a reason for hiding this comment

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

I feel like io_seek() and io_read() can be fully abstracted by just making _buffer_size() a protected virtual function in the base class. io_write is unfortunately a little bit trickier it seems since there is a special case for fmemopen().

@no92 no92 force-pushed the fmemopen branch 3 times, most recently from 87939fa to 0691bc1 Compare August 31, 2023 18:09
@no92 no92 force-pushed the fmemopen branch 2 times, most recently from 154d4cf to 9e33902 Compare February 20, 2024 21:06
Copy link
Member

@Dennisbonke Dennisbonke left a comment

Choose a reason for hiding this comment

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

LGTM

@Dennisbonke Dennisbonke merged commit 4fb2603 into managarm:master Feb 20, 2024
29 of 30 checks passed
@no92 no92 deleted the fmemopen branch February 20, 2024 22:05
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.

posix: fmemopen() unimplemented
4 participants