Skip to content

Conversation

@KowalczykBartek
Copy link
Contributor

Looks like this change 353812f is causing following error (when SimpleConsumer is being run) - at least on OSX

librdkafka validated config value is valid u64: ParseIntError { kind: InvalidDigit }

That is because rd_kafka_conf_get returns null terminated string,

// Allocate a buffer of that size and call again to get the actual
// string.
let mut buf = vec![0_u8; size];
let res = unsafe {
    rdsys::rd_kafka_conf_get(
        self.ptr(),
        key_c.as_ptr(),
        buf.as_mut_ptr() as *mut c_char,
        &mut size,
    )
};

ends with buf equal:

[51, 48, 48, 48, 48, 48, 0];

I guess reverting this change will cause issues on windows, so maybe my PR will be good enough to handle this.

This pr should also fix this case #683

@chklauser
Copy link

Just as a heads up for maintainers: there seem to be 3 PRs for this issue

  1. Use CStr::from_bytes_until_nul to read rdkafka config #671
  2. Remove nul chars from string before parsing to numerical values #688 (this PR)
  3. Fix null characters in configuration values #691

@fede1024 fede1024 merged commit 036e0bb into fede1024:master Aug 3, 2024
@mati865
Copy link

mati865 commented Aug 6, 2024

This PR feels like a workaround that is doomed to break in the future. Aby chance to revisit #671?

@fede1024
Copy link
Owner

fede1024 commented Aug 6, 2024

I agree that #671 is a better solution. @mati865 any chance you could file a PR with from_bytes_until_nul? #671 has a few other changes that are conflicting with the master branch. Thanks

@mati865
Copy link

mati865 commented Aug 6, 2024

I'll wait for @chklauser to reply if he wants to revive his PR, otherwise I'll rebase hit work.

@chklauser
Copy link

I have rebased the change as #705 (forgot to re-open before pushing the new branch). With the unrelated bump of MSRV to 1.70 we can now just use CStr::from_bytes_until_nul 🥳

Thanks for tagging me @mati865

@andrei-ionescu
Copy link

andrei-ionescu commented Aug 7, 2024

I also created #706 for this issue, not knowing that there is this. Not sure which one would get merged to fix the problem.

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.

5 participants