Page MenuHomeFreeBSD

liblua: Implement write support
ClosedPublic

Authored by kevans on Feb 23 2018, 2:37 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, May 3, 10:00 AM
Unknown Object (File)
Tue, Apr 30, 8:22 PM
Unknown Object (File)
Tue, Apr 30, 8:22 PM
Unknown Object (File)
Tue, Apr 30, 8:22 PM
Unknown Object (File)
Tue, Apr 30, 8:21 PM
Unknown Object (File)
Tue, Apr 30, 8:21 PM
Unknown Object (File)
Tue, Apr 30, 8:19 PM
Unknown Object (File)
Tue, Apr 30, 8:19 PM
Subscribers
None

Details

Summary

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

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 15232

Event Timeline

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

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.

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)

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.

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 marked 12 inline comments as done.

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 added inline comments.
stand/liblua/lstd.c
101

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

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

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

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

stand/liblua/lutils.c
280

Sorry, lost a semicolon here in diff fungling process.

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.

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

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

This revision is now accepted and ready to land.Feb 24 2018, 2:06 AM
cem added inline comments.
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.

This revision was automatically updated to reflect the committed changes.