Skip to content

Conversation

@fogti
Copy link

@fogti fogti commented Oct 31, 2025

Also add a way to skip the CString allocation.

Note that this can break existing code by making type inference ambiguous, thus this also bumps the crate minor version.

Summary of the PR

Please summarize here why the changes in this PR are needed.

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR have Signed-Off-By trailers (with
    git commit -s), and the commit message has max 60 characters for the
    summary and max 75 characters for each description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

@fogti fogti changed the title feat: Make property_string_list take an IntoIterator as input feat: Make property_string_list take an IntoIterator as input Oct 31, 2025
Also add a way to skip the CString allocation.

Note that this can break existing code by making type inference ambiguous,
thus this also bumps the crate minor version.

Signed-off-by: Ellen Εμιλία Άννα Zscheile <[email protected]>
@jounathaen
Copy link

I'm currently working on an alternative approach that uses AsRef<str>, which would hopefully not break the API.

@jounathaen
Copy link

Is this change necessary, if #76 would get merged?

@fogti
Copy link
Author

fogti commented Oct 31, 2025

I think it would still make sense to expose a version that accepts a list of CString's instead.

@danielverkamp
Copy link
Collaborator

I agree that adding a CString (or &CStr, which would work for CString as well as C string literals; see note about c"..." below) in place of &str for the strings would be a nice enhancement.

I don't have a strong opinion about the exact API; maybe it would make sense to keep the existing &str/String APIs as is (potentially with the AsRef<str> enhancement from #76) for compatibility, and add completely new functions that accept &CStr, with _cstring instead of _string in the function names (similar to this proposal, but I would lean toward adding completely new functions to avoid API breakage; the &str version could convert to CString and then call the _cstring variant internally to minimize code duplication).

The main reason the string APIs accept &str rather than a C string directly is that when this crate was originally written, there was no convenient way to write C string constants in Rust (callers would have to do the clunky &str to CStr/CString conversion manually if we had written it that way), but now that C-string literals have been stabilized, it is probably feasible to avoid &str entirely and just write e.g. .property_cstring(c"property-name", c"property-value"), potentially dropping the &str APIs eventually. (c"..." was stabilized in Rust 1.77; I'm not sure of the overall rust-vmm MSRV strategy, so maybe this is not guaranteed to be broadly available yet.)

@fogti
Copy link
Author

fogti commented Nov 1, 2025

the &str version could convert to CString and then call the _cstring variant internally to minimize code duplication

sounds quite difficult to me, unless we also introduce a try_ variant where we take an iterator with an item type of Result<CString, E>, to avoid having to allocate a vector just for the collection of the CStrings.

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