Skip to content
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

Redbean: Support for unshare() mount() prctl() in Lua #801

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

yael333
Copy link

@yael333 yael333 commented Apr 8, 2023

These changes should extend Redbean's Lua's functionality to add additional unix syscalls per requested here #794.

I wasn't able to find any direct support for the unshare syscall in the Cosmopolitan C lib, so for now I am opening this pull request as incomplete without support for it.
Few notes I have to leave for the maintainers:

  • I made sure to add fields for the unix module for all flags and options for these syscalls, something that I perceived as something that pollutes the unix module so please let me know if there's any other way to implement it.
  • Where can I add these syscalls to the documentation? And is there any need to add tests for them?
  • I apologize in advance for any mistake whether it be a grave error or a styling mistake, this is my first PR for this project (and one the first for an open source project 💖)

@pkulchenko
Copy link
Collaborator

pkulchenko commented Apr 9, 2023

Thank you for the patch! Overall, this PR looks good to me, but there are a couple of things that need to be fixed. This looks strange:

       if (lua_isnumber(L, 2)) {
            arg2 = lua_tointeger(L, 2);
        } else if (lua_isstring(L, 2)) {
            arg2 = (unsigned long) lua_tostring(L, 2);
        }

Why would the result of lua_tostring be converted to unsigned long if it's char*? I think what you really want to do is to use lua_tointegerx (or lua_tonumberx if needed), as it would convert to a number and tell you if the conversion was successful or not.

Also the closing bracket in the comments should have a slightly different character:

      └─→ nil, unix.Errno

Where can I add these syscalls to the documentation? And is there any need to add tests for them?

Tests would be ideal, but these two may be difficult to test (although you may still be able to test for some basic parameters).

The documentation is in tool/net/help.txt; I can update it after this PR is merged or you can add it there yourself as part of the same PR.

@pkulchenko pkulchenko self-requested a review April 9, 2023 00:31
@yael333
Copy link
Author

yael333 commented Apr 9, 2023

Thank you so much for the lovely and relevant feedback 🐈
The Lua comment is really nice to learn and much better for sure, will get on all of the changes tomorrow.

@yael333
Copy link
Author

yael333 commented Apr 9, 2023

I added corrections for the relevant feedback.
Although I wasn't able to mitigate the (unsigned long) cast, the prctl syscall accepts arguments in the form of unsigned longs that are polymorphic in the sense they can be either a pointer or an integer. It does work in both cases so for now and I'm leaving it as it is and if there's something that is missing then I'll correct as needed🎇
Thank you so much.

Copy link
Owner

@jart jart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approval for copyright.


// unix.umount(target: str[, flags: int)
// ├─→ true
// ├─→ nil, unix.Errrno
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor tweak, but the character should be (as n prctl).


// unix.mount(source:str, target:str, filesystemtype: str[, mountflags: int[, data: str]])
// ├─→ true
// ├─→ nil, unix.Errno
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor tweak, but the character should be (as n prctl).

luaL_optinteger(L, 4, 0), luaL_optstring(L, 5, NULL)));
}

// unix.umount(target: str[, flags: int)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing closing ].

// unix.prctl(option: int[, arg2: int[, arg3: int[, arg4: int[, arg5: int]]]])
// ├─→ result: int
// └─→ nil, unix.Errno
static int LuaUnixPrctl(lua_State *L) {
Copy link
Collaborator

@pkulchenko pkulchenko Jul 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of the get-like prctl variants are going to work, as there is no way to get the values back to the Lua layer. These values either need to be returned as prctl results (it seems like most of them are stored as arg2, so may be need to be returned as a string or number) or need to be taken as one table parameter, in which case the table can be modified to pass the values back. The first option is going to be more consistent with how the rest of unix.* functions operate.

I'm not familiar with the implementation details of prctl to access its coverage in cosmopolitan, but I'm sure @jart can comment on that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants