liblua: Implement write support

[kevans]

Summary

Write support (even if it only works on UFS) will be needed for nextboot functionality.

[kevans]

Forgot to initialize "mode" to the default ("r")

[cem]

stand/liblua/lstd.c

45

"w" alone also sets O_CREAT | O_TRUNC.

50

"+" 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);
...

101

Ouch :-)

108

const void *ptr

stand/liblua/lutils.c

179

Should error check this

260

How is the API for io.write() defined? Is there a spec / could you write a short spec comment?

273–274

should we not still pushinteger(L, 0) ?

280

Why are we de-consting the result?

283

args is leaked

287–288

Why do we immediately const and de-const? fwrite() should take a const void *.

288

Lua strings can contain embedded nuls. There should be an API to get the length as well as the pointer from the stack argument.

[kevans]

stand/liblua/lstd.c

45

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

Ditto above comment- we don't have these in stand, as far as I know.

108

This is actually just copying a whole bunch of stand stupid-ness, that I'll describe in a minute.

stand/liblua/lutils.c

260

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.

273–274

Right

280

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.

283

Whoops =)

288

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)

[cem]

stand/liblua/lstd.c

45

We could at least stub it out? Idk.

stand/liblua/lutils.c

280

Then take a const pointer and __DECONST in fwrite(), to at least contain it there. Instead of pushing it further up the stack.

[imp]

great start

stand/liblua/lstd.c

45

libsa isn't sacrosanct. If we need to do so, we can add something :)

stand/liblua/lutils.c

280

We should fix the const issue, but not with this code review...

[kevans]

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.

[kevans]

stand/liblua/lstd.c

101

Not sure what the "Ouch" part is- this is in a section I didn't touch? =p

[kevans]

Remove a de-const from testing that snuck in...

[cem]

One last nit — otherwise LGTM.

stand/liblua/lstd.c

101

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

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.)

[kevans]

• Avoid the allocations, just make two passes
• Use luaL_fileresult, which was designed for passing back errors from file operations.

stand/liblua/lstd.c

101

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. =)

[kevans]

stand/liblua/lutils.c

280

Sorry, lost a semicolon here in diff fungling process.

[cem]

stand/liblua/lstd.c

101

Yep

114

This seems to have snuck back in

stand/liblua/lutils.c

262

wrsz appears unused

280

semicolon

283

I was surprised, but this looks correct. What a weird little language:

int lua_isstring(...) {
...
return (t == LUA_TSTRING || t == LUA_TNUMBER);

285

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.

[kevans]

Address more things; handle short-writes from our write(2)-alike and return an error on such an occasion.

stand/liblua/lutils.c

262

Whoops, I planned on using that for validation of fwrite's return, but got ahead of myself.

283

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

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).

[imp]

With all the comments, this has gotten hard to follow, but from what I am reading it looks good to me.

[cem]

stand/liblua/lstd.c

114

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

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.