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

Modify pointer type from i8 to c_char #1792

Merged
merged 1 commit into from
Apr 11, 2023
Merged

Conversation

kemkemG0
Copy link
Contributor

@kemkemG0 kemkemG0 commented Apr 7, 2023

[Environment Info]

root ➜ /workspaces/youki  $ uname -a
Linux 80f9ee1dbb2e 5.15.49-linuxkit  aarch64  GNU/Linux

Description

When running the command:

$ make unittest

I encountered the following error:

error[E0308]: mismatched types
  --> crates/libcgroups/src/v2/devices/bpf.rs:39:22
   |
39 |             log_buf: ptr::null_mut::<i8>(),
   |                      ^^^^^^^^^^^^^^^^^^^^^ expected `u8`, found `i8`
   |
   = note: expected raw pointer `*mut u8`
              found raw pointer `*mut i8`

I found a similar error in another repository, which can be found here: remacs/remacs#1393 (comment)

My understanding is that the pointer type on ARM and x86 is defined differently. Following the advice on the issue, I used the c_char type instead of i8.

I also noticed that while the issue uses libc::c_char, this repository uses ::std::os::raw::c_char in some other parts. After reading this thread: https://users.rust-lang.org/t/libc-c-char-vs-std-os-c-char/21198, I decided to use ::std::os::raw::*.

Therefore, I used ::std::os::raw::c_char in my modifications.

@codecov-commenter
Copy link

codecov-commenter commented Apr 7, 2023

Codecov Report

Merging #1792 (3849a97) into main (edd63c8) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1792   +/-   ##
=======================================
  Coverage   68.63%   68.63%           
=======================================
  Files         121      121           
  Lines       13325    13325           
=======================================
  Hits         9146     9146           
  Misses       4179     4179           

@kemkemG0
Copy link
Contributor Author

kemkemG0 commented Apr 7, 2023

Well, I think I made a mistake on the GitHub operation... I'll fix it soon ;(

maybe fixed :)

@kemkemG0
Copy link
Contributor Author

kemkemG0 commented Apr 7, 2023

Now I realized that #1579 is related.

@Furisto
Copy link
Collaborator

Furisto commented Apr 9, 2023

Hey @kemkemG0 changes looks good. Can you please rebase to get rid of the merge commits? The integration test is failing because of a clippy lint. Would you like to fix that as well or should we do it separately?

@kemkemG0
Copy link
Contributor Author

kemkemG0 commented Apr 10, 2023

Hi, @Furisto thanks for your review. I will rebase to get rid of the merge commits.

However, I don't see why the clippy lint fails as I didn't modify the part clippy is warning.

Should I fix that part as it suggests?

I apologize if I misunderstood the point ;)

Screenshot 2023-04-09 at 5 07 39 PM

@kemkemG0
Copy link
Contributor Author

kemkemG0 commented Apr 10, 2023

I apologize for messing up the commit logs .. :(
I am not used to rebasing or dealing with forked repo...
I'll fix it soon

Though some logs are here, I see only one commit now.

Is it fixed?

@kemkemG0
Copy link
Contributor Author

Hi, @Furisto thanks for your review. I will rebase to get rid of the merge commits.

However, I don't see why the clippy lint fails as I didn't modify the part clippy is warning.

Should I fix that part as it suggests?

I apologize if I misunderstood the point ;)

Screenshot 2023-04-09 at 5 07 39 PM

Now, the test has passed somehow :)

@Furisto
Copy link
Collaborator

Furisto commented Apr 11, 2023

Thanks @kemkemG0

@Furisto Furisto merged commit 307e941 into youki-dev:main Apr 11, 2023
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