Write support (even if it only works on UFS) will be needed for nextboot functionality.
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| stand/liblua/lstd.c | ||
|---|---|---|
| 45 ↗ | (On Diff #39632) | "w" alone also sets O_CREAT | O_TRUNC. |
| 50 ↗ | (On Diff #39632) | "+" shouldn't clear O_CREAT | O_TRUNC for "w+." I'd suggest copying a subset of stdio/flags.c: int m, flags;
...
flags = 0;
if (mode[0] == 'r')
m = O_RDONLY;
else {
m = O_WRONLY;
flags = O_CREAT | O_TRUNC;
}
if (mode[1] == '+')
m = O_RDWR;
fd = open(filename, m | flags);
... |
| 89 ↗ | (On Diff #39632) | Ouch :-) |
| 96 ↗ | (On Diff #39632) | const void *ptr |
| stand/liblua/lutils.c | ||
| 179 ↗ | (On Diff #39632) | Should error check this |
| 255 ↗ | (On Diff #39632) | How is the API for io.write() defined? Is there a spec / could you write a short spec comment? |
| 268–269 ↗ | (On Diff #39632) | should we not still pushinteger(L, 0) ? |
| 275 ↗ | (On Diff #39632) | Why are we de-consting the result? |
| 278 ↗ | (On Diff #39632) | args is leaked |
| 282–283 ↗ | (On Diff #39632) | Why do we immediately const and de-const? fwrite() should take a const void *. |
| 283 ↗ | (On Diff #39632) | Lua strings can contain embedded nuls. There should be an API to get the length as well as the pointer from the stack argument. |
| stand/liblua/lstd.c | ||
|---|---|---|
| 45 ↗ | (On Diff #39632) | The problem is we don't actually have O_CREAT | O_TRUNC support in stand. We really just need to be able to translate some of this stuff to what we've got (O_RDONLY, O_WRONLY, O_RWDR -- that's it) so that we can implement at least a subset of the functionality io wants. |
| 50 ↗ | (On Diff #39632) | Ditto above comment- we don't have these in stand, as far as I know. |
| 96 ↗ | (On Diff #39632) | This is actually just copying a whole bunch of stand stupid-ness, that I'll describe in a minute. |
| stand/liblua/lutils.c | ||
| 255 ↗ | (On Diff #39632) | I've not found much of anything worthwhile, other than "it takes a file and any number of string arguments" and every example ignores its return value. |
| 268–269 ↗ | (On Diff #39632) | Right |
| 275 ↗ | (On Diff #39632) | Because of the above-mentioned write stupidity. write() in stand-land takes a non-const buffer. Correcting this requires another mountain of fun, because that error is cascaded through to a bunch of filesystems that would also need corrected. |
| 278 ↗ | (On Diff #39632) | Whoops =) |
| 283 ↗ | (On Diff #39632) | Right, I'll fix that. The const and de-const was copy-pasta, we should really just deconst it and be done (because the way this write stuff works here is dumb) |
Comment Actions
I think this addresses most of @cem's concerns... I've added the other O_ flags that we don't implement yet for the sake of correctness. I can probably actually implement those sometime in the next week or so, but for the moment we won't be writing anything that depends on their behavior I would assume.
| stand/liblua/lstd.c | ||
|---|---|---|
| 89 ↗ | (On Diff #39632) | Not sure what the "Ouch" part is- this is in a section I didn't touch? =p |
Comment Actions
One last nit — otherwise LGTM.
| stand/liblua/lstd.c | ||
|---|---|---|
| 89 ↗ | (On Diff #39632) | Ouch is the casting the ssize_t return value of read to size_t and totally ignoring possible errors. Yes, it's not something you touched. I only saw it because it was nearby. |
| stand/liblua/lutils.c | ||
| 278 ↗ | (On Diff #39661) | I think we can/should just avoid these memory allocations entirely. Just do one pass where we check that each argument is convertible to a string. Then do a second pass where we get the pointer+size and write it. (lua_tostring() actually converts the stack object in-place, so this shouldn't be "expensive" even if invoked with a lot of non-string objects.) |
Comment Actions
- Avoid the allocations, just make two passes
- Use luaL_fileresult, which was designed for passing back errors from file operations.
| stand/liblua/lstd.c | ||
|---|---|---|
| 89 ↗ | (On Diff #39632) | Ah, right- I had copied that same mistake into fwrite at first, then decided it was especially wrong given that it adds to the stream->offset and returns it. We can fix that in another iteration. =) |
| stand/liblua/lutils.c | ||
|---|---|---|
| 280 ↗ | (On Diff #39663) | Sorry, lost a semicolon here in diff fungling process. |
| stand/liblua/lstd.c | ||
|---|---|---|
| 114 ↗ | (On Diff #39663) | This seems to have snuck back in |
| 89 ↗ | (On Diff #39632) | Yep |
| stand/liblua/lutils.c | ||
| 262 ↗ | (On Diff #39663) | wrsz appears unused |
| 280 ↗ | (On Diff #39663) | semicolon |
| 283 ↗ | (On Diff #39663) | I was surprised, but this looks correct. What a weird little language: int lua_isstring(...) {
...
return (t == LUA_TSTRING || t == LUA_TNUMBER); |
| 285 ↗ | (On Diff #39663) | We could store the filename in our userdata for lua file objects to enhance this error message. That's probably out of scope for this patch, though. |
Comment Actions
Address more things; handle short-writes from our write(2)-alike and return an error on such an occasion.
| stand/liblua/lutils.c | ||
|---|---|---|
| 262 ↗ | (On Diff #39663) | Whoops, I planned on using that for validation of fwrite's return, but got ahead of myself. |
| 283 ↗ | (On Diff #39663) | I went ahead and threw in a mention to what's going on with lua_isstring/lua_tolstring, so that it's a little more obvious at a glance that it also accepts Number arguments. |
| 285 ↗ | (On Diff #39663) | Right, that sounds like a good idea. We should also rewrite lua_readfile's return data to use luaL_fileresult, since that's also the more correct behavior there (and other io.* file functions, IIRC). |
Comment Actions
With all the comments, this has gotten hard to follow, but from what I am reading it looks good to me.
| stand/liblua/lstd.c | ||
|---|---|---|
| 114 ↗ | (On Diff #39673) | If there's a short write here, should fwrite(3) loop and attempt to write(2) again? I'm ok with leaving as-is to limit scope. |
| stand/liblua/lutils.c | ||
| 296–297 ↗ | (On Diff #39673) | Hm. I'm not sure if this should be an error or if we should just return the short write result. I'm ok with the error behavior for now though. |