littlekernel / lk

LK embedded kernel
MIT License
3.11k stars 613 forks source link

[lib][stdio] wrap file io APIs with stdio APIs #340

Closed chiehmin closed 2 months ago

chiehmin commented 1 year ago

Implement stdio file io APIs with LK lib/fs APIs

Signed-off-by: Chieh-Min Wang cmwang@google.com

travisg commented 1 year ago

Oh I like this! I'll need to think about it a little bit since there are two (minor) issues I'd like to consider:

1) this CL will force lib/fs to always be built in all configurations, even in situations where fses aren't really needed (deeply embedded stuff). If it could be made to be conditional if lib/fs is present that might be helpful, though that can be a bit messy in the header space. 2) the switch between using an io_handle_t and a filehandle is a little clumsy. Perhaps it'd make a bit more sense to extend io_handle_t to wrap a file? or even union the two in the FILE so that it's a bit more obvious it's one or the other? I don't know of all of the downstream users of io_handle_t but i suspect there are more than just what's in the tree, so it might be a bit better to test on something ike filehandle being non-null, or store a bool or flag that says which one is active.

For 1 it is possoble to test for the presence of another module by testing for WITH_LIB_FS, though it might be a bit nasty ifdef wise.

chiehmin commented 1 year ago

@travisg I have push a new commit to fix the issues you mentioned. Please take a look at it to see if it matches your expection. Thanks! 😄

chiehmin commented 1 year ago

@travisg I have resolved all the comments and updated this PR. Please give another review of it. Thanks!

chiehmin commented 1 year ago

@travisg Is there other considerations for this CL?

travisg commented 1 year ago

I'll take a look tonight. Thanks for the work!

chiehmin commented 1 year ago

@travisg Any update 😃 ?

travisg commented 1 year ago

Not yet, haven't had a chance to run it.

travisg commented 1 year ago

Just want to comment that I haven't forgotten about this, just have been really busy the last few weeks. Currently on a business trip out of country.

chiehmin commented 1 year ago

@travisg Any update for this 😃 ?

travisg commented 2 months ago

looks good! apologize for waiting so long, but i'm generally back on LK nowadays and have been going through backlog.