-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
libc: add support for custom streams with fopencookie() #10602
Conversation
9e6a5fd
to
23d1968
Compare
23d1968
to
87526a1
Compare
I personally think you are going down the wrong path with this ad hoc logic. This is very high risk because I don't think either of you are considering all of the issues and complexities. You apparently seem focused on avoiding a clean context switch (which I believe is the correct, clean, flexible solution). I believe we need to at least insist on a good test for this code to give people like me confidence. A good test would include:
If it could pass a pretty rigorous test then I would become a believer. That, I think is a requirement for merging. I am not a believer now. |
I once had a look at that to handle a struct libc_stdio_file
{
// ... stdio specifics;
libc_stdio_fread_t read;
libc_stdio_fwrite_t write;
libc_stdio_fseek_t seek;
libc_stdio_fclose_t close;
libc_stdio_cookie_t cookie;
file_struct * internal;
}
typedef libc_stdio_file FILE; So
If few folks are interested in architecting a new Other option: we dig up actual Note: |
@patacongo My main argument againts hooks you described above (various notifications, events, signal handlers etc.) is that it would lead to additional requirements for user application (different from how @mu578 I like the structure you proposed, that would solve a lot of issues with current I would do this primarily for I also think we should probably move this discussion to mailing list now. |
we don't need add a new struct libc_stdio_file. Here is the current definition:
We can make a simple change to it:
and replace all read/write/seek/close with stream->iofunc.read/write/seek/close, fs_fd with fs_cookie.
and initialize fs_iofunc and fs_cookie in fopen like this:
|
The marshaled data should be indisguishable from a direct function (provided an intermediate marshaling/de-marshaling,. My main argument against this approach is that I have not heard any solution that will actually work in all conditions and build modes. |
@patacongo I think @michallenc initial design may confuse you. stdio is a pure userspace library, the implementation of fopencookie or fmemopen doesn't need involve any kernel stuff: it's enough to add the callback(read/write/seek/close) to struct file_struct(FILE). |
you would if you want to keep separated internal implementation to actual in-memory Os layout and a libc stdio frontend concern. Indeed
But again do not hot patch such. |
If you don't like file_struct is contained in nuttx/fs.h, the right fix is moving it to stdio.h, but don't create a new libc_stdio_file since libc_stdio_file lack many fields which is required to implement buffer I/O and fungetc etc.
|
AFAIK that is not true. The user/supervisor mode is only one of several issues. I have enumerated many others below that this design pure user-space design will not handle. The bottom line: You cannot call from one task/process address space into another. In the FLAT build, that might work in some very, very simple cases. But it will not work in general. It will never work at all in the KERNEL build if the client and server lie in different process address spaces. The virtual user address spaces overlap and it you try calling into it you will certainly crash. In order to that successfully and reliably, a context switch is required. I don't see any reason for me to participate in this any further. I've tried to explain all of this as carefully as I can but I still see you guys marching ahead to fall off the cliff. I have unsubscribed from this PR. Please be sure to test all of these cases before merging an incorrect design. |
This case isn't suitable for stdio functions. How can you pass FILE * from one task/process to other task/process in kernel mode? So it's impossible to call the callback function cross the address space boundary since FILE* return from fopencookie can only be used by the caller's task/process and is always invalid and impossible to pass FILE* to another task/process at the first place. |
I don't see the relevance of the FILE*. I was more concerned at the io_funcs and cookie. But looking at the prototype, it seems impossible that the io_funcs could like in a different address space. from the caller of fopencookie(). |
Aggreed, it would require to pound defined and only allowing such APIs in usermode. However, it could be doable by making a new vfs interface handling such but it would require a whole revamp and further thoughts to make it possible, as is : it is not hot patchable -> so my proposal to support indirection for user-mode build only, at least it can be undone and evolve without breaking. That is impossible to callback from two distinct regions ; it would require some kind of trampoline jump with copies back and forth. E.g if Myself, I would go to support standard only: |
Yes, this is the key point: both io_funcs and cookie can only be used in the same proccess(address space), which simplify the thing a lot if the implementation can avoid the involve any kernel space stuff. |
Of course, all FILE functions can only be called inside the usermode, NuttX kernel forbid kernel code to use any FILE functions.
@mu578 you may misunderstand NuttX file system:
Do you want to implement the userspace file system? NuttX already support it by socket:
If we want to support all open_memstream/open_wmemstream/fmemopen, it's important to setup a callback infrastructure otherwise fread/fwrite need to add many if/else:
If we need to add the callback to FILE, why not follow fopencookie which exist in most libc implementation(glibc, newlib, *BSD), but add a NuttX specific function instead? I don't see any benefit. |
To summarize the so far discussion a bit: The problem discussed here is a that this implementation reuses I quite like the proposition of reworked/new file system @mu578 suggested but as he mentioned, it is not a stuff for one patch and probably should be discussed on a mailing list first. I also do not agree with the statement
as Regarding kernel protected builds, address space (and also priorities to keep strict scheduling), I think it should be sufficient to ensure user calls read/write/seek/close do not enter kernel space i.e. We will also have to avoid calling |
I fully understand vfs, no callback infrastructure is needed, that's a glibc patch, who cares? just deal with a flag about memory provider that's it. |
But I care where the implementation is elegance and extendable. The code like spaghetti should avoid as mention in:
even for memory stream, there are at least two cases:
The detail is evil, you have to add more flags/fields and many if/else in many common places to handle the different behavior required by spec. The callback is better design here to encapsulate the difference behind the differerent callback(fd vs. mem, fixed vs. dynamically). |
yes make a context buffer |internal|user_malloc|user_dont_touch|
|
87526a1
to
c5bd1b1
Compare
I have updated the implementation with internal callbacks so now the code should entirely avoid kernel address space. How are we going to design some tests that would ensure the correctness? |
https://github.com/apache/nuttx-apps/blob/master/testing/smart_test/smart_test.c contain some test related to stdio. |
c5bd1b1
to
0e4c3cc
Compare
Some initial tests can be found here: michallenc/incubator-nuttx-apps@1cd88d5 I have only tested it in FLAT mode so far as I have not been able to compile NuttX in protected mode (regardless of |
Yes, I fixed some on Saturday but I suppose the rest is somehow related to change to |
Ok, let's wait the merging of #10913, so you can move the FILE allocation to fopencookie. |
29cb5ac
to
7702fc8
Compare
@michallenc #10913 was merged, you can rebase your patch on top of it now. |
7702fc8
to
116afdc
Compare
@xiaoxiang781216 Updated. Local CI runs were successful, lets see what GitHub CI does. |
Hmm, sim-01 fails without actually logging any error. |
7fe9d99
to
f0e8304
Compare
please fix:
|
1f1fad4
to
2e75156
Compare
2e75156
to
514282d
Compare
514282d
to
a0a414c
Compare
This commit adds support for custom stream via fopencookie function. The function allows the programmer the create his own custom stream for IO operations and hook his custom functions to it. This is a non POSIX interface defined in Standard C library and implemented according to it. The only difference is in usage of off_t instead of off64_t. Programmer can use 64 bits offset if CONFIG_FS_LARGEFILE is enabled. In that case off_t is defined as int64_t (int32_t otherwise). Field fs_fd is removed from file_struct and fs_cookie is used instead as a shared variable for file descriptor or user defined cookie. The interface will be useful for future fmemopen implementation. Signed-off-by: Michal Lenc <[email protected]>
This adds fopencookie test for simulator CI. Signed-off-by: Michal Lenc <[email protected]>
a0a414c
to
4b7fe2c
Compare
Summary
This commit adds support for custom stream via
fopencookie
function. The function allows the programmer the create his own custom stream for IO operations and hook his custom functions to it.This is a non POSIX interface defined in Standard C library and implemented according to it. The only difference is in usage of
off_t
instead ofoff64_t
. Programmer can use 64 bits offset ifCONFIG_FS_LARGEFILE
is enabled. In that caseoff_t
is defined asint64_t
(int32_t
otherwise).The interface will be useful for future fmemopen implementation.
Impact
New interface.
Testing
Tested with example application on man page on SAMv7 MCU.