Skip to content

Conversation

@CrazyboyQCD
Copy link

@CrazyboyQCD CrazyboyQCD commented Mar 26, 2025

Closes #748.
Leave construction of string argument to caller and make ffi process less panic-proned, and is benefit for users whose rust version is above 1.77 to use C-string literals

@CrazyboyQCD CrazyboyQCD force-pushed the cow-cstr branch 5 times, most recently from feb1938 to 7b95abf Compare March 26, 2025 08:45
@CrazyboyQCD CrazyboyQCD marked this pull request as draft March 26, 2025 08:58
@CrazyboyQCD CrazyboyQCD force-pushed the cow-cstr branch 6 times, most recently from 2274812 to 4504d53 Compare March 26, 2025 09:59
@ids1024
Copy link
Member

ids1024 commented Mar 26, 2025

If arguments take Cow, then all callers will have to explicitly convert to a Cow. A generic requiring Into<Cow<'_, CStr>> could be better, though that still isn't implemented for types like &str.

Perhaps a custom trait like Rustix uses would be best here: https://docs.rs/rustix/1.0.3/rustix/path/trait.Arg.html.

@ids1024
Copy link
Member

ids1024 commented Mar 26, 2025

In principle using CStr in the API is quite correct since Wayland requires null-terminated strings, but it may be annoying to deal with.

@i509VCB
Copy link
Member

i509VCB commented Mar 26, 2025

There is also the issue that strings in Wayland are rather undefined in their use. Nothing stops a client from sending you malformed UTF-8 or UTF-16.

The two exceptions are xdg-toplevel titles (explicitly defined as UTF-8) and the identifier the compositor created for ext-foreign-toplevel-handle (printable ASCII)

@ids1024
Copy link
Member

ids1024 commented Mar 26, 2025

Right. As defined in https://wayland.freedesktop.org/docs/html/ch04.html, I guess technically the only difference between strings and arrays is null-termination? And this seems to imply there shouldn't be other nul bytes, though I don't see that explicitly mentioned.

So CStr/CString is correct here, though a pain to work with. Life would be easier if wayland was more specific and libwayland did utf-8 validation for strings.

@CrazyboyQCD CrazyboyQCD force-pushed the cow-cstr branch 4 times, most recently from 12c8ae7 to 7ff83c6 Compare March 27, 2025 00:57
@CrazyboyQCD
Copy link
Author

@ids1024
After overview, I found a auto-convert trait argument will increase code complexity and make product bloaty, thought callers need to deal with this, it is more flexible for callers if they want to be more performant.

@CrazyboyQCD CrazyboyQCD marked this pull request as ready for review April 8, 2025 04:00
@ids1024
Copy link
Member

ids1024 commented Apr 8, 2025

Yeah, there's not a clear best way to do this.

Converting a &str to a CStr manually is a bit annoying, but strings aren't really that common in wayland protocols (and higher level APIs like smithay and smithay-client-toolkit can decide what is suitable as a higher level API for a specific protocol). And having to deal with CStrings that may or may not be valid UTF-8 in return values is necessary regardless.

@orowith2os
Copy link

I will also note that the Wayland protocol defines a string argument as:

Starts with an unsigned 32-bit length (including null terminator), followed by the string contents, including terminating null byte, then padding to a 32-bit boundary. A null value is represented with a length of 0.

We need the null terminator, but it's not necessarily used for counting the string's length. That's something C-based Wayland clients can do.

And here's a note in CStr's documentation on count_bytes:

Note: This method is currently implemented as a constant-time cast, but it is planned to alter its definition in the future to perform the length calculation whenever this method is called.

It would be better to take a &mut String (or a Cow<_, String>) and check for a NUL byte, add it if one doesn't exist, and then pass it as an argument. It would also need to make sure that there's no NUL byte in the middle of the string.

@orowith2os
Copy link

Recent versions of the core Wayland protocol will also require that all strings used are valid UTF-8: https://gitlab.freedesktop.org/wayland/wayland/-/merge_requests/387

@orowith2os
Copy link

orowith2os commented Apr 12, 2025

Correction: Cow<str> would probably be better? Then append NUL to that. Would also need to perform validation so that it doesn't pass a string with two NUL bytes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

String arguments should be CString

4 participants