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

refactor: improve code readability of struct initialization #633

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

Integral-Tech
Copy link
Contributor

Background

Checklist

Full Changelogs

  • Improve code readability of struct initialization

Test Result

Copy link
Contributor

@dae-prow dae-prow bot left a comment

Choose a reason for hiding this comment

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

🧪 Since the PR has been fully tested, please consider merging it.

Copy link
Contributor

@mzz2017 mzz2017 left a comment

Choose a reason for hiding this comment

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

LGTM

@mzz2017 mzz2017 merged commit 8255400 into daeuniverse:main Sep 13, 2024
27 checks passed
@Integral-Tech Integral-Tech deleted the refactor-struct branch September 13, 2024 16:56
@jschwinger233
Copy link
Member

👍

其实在 bpf c 代码里结构体先零值初始化再逐字段赋值是最佳实践。考虑以下代码:

struct entry {
	__u8 ifindex;
	__u8 from_wan;
};

struct entry entry = { . from_wan = 1 };
bpf_map_delete_elem(&MAP, &entry);

就有可能被 verifier 拒绝,因为 clang 编译上面的代码发现 ifindex 字段没有被使用,索性不初始化那个字节的栈内存,留下一个空洞,但是 bpf verifier 检查 bpf_map_delete_elem 的时候会要求所有参数指针指向的内存必须被初始化,所以程序就挂了。

这个 PR 没有问题是因为那些结构体所有字段最终都被赋值了,没有空洞。

@mzz2017
Copy link
Contributor

mzz2017 commented Sep 14, 2024

@jschwinger233 解释非常清晰,先把这个 pr 回退,意下如何?
@Integral-Tech

@Integral-Tech
Copy link
Contributor Author

Integral-Tech commented Sep 14, 2024

👍

其实在 bpf c 代码里结构体先零值初始化再逐字段赋值是最佳实践。考虑以下代码:

struct entry {
	__u8 ifindex;
	__u8 from_wan;
};

struct entry entry = { . from_wan = 1 };
bpf_map_delete_elem(&MAP, &entry);

就有可能被 verifier 拒绝,因为 clang 编译上面的代码发现 ifindex 字段没有被使用,索性不初始化那个字节的栈内存,留下一个空洞,但是 bpf verifier 检查 bpf_map_delete_elem 的时候会要求所有参数指针指向的内存必须被初始化,所以程序就挂了。

这个 PR 没有问题是因为那些结构体所有字段最终都被赋值了,没有空洞。

但是根据 cppreference:

All members that are not initialized explicitly are empty-initialized.

In some cases, an object is empty-initialized if it is not initialized explicitly, that is:
...
all elements of arrays, all members of structs, and the first members of unions are empty-initialized, recursively, plus all padding bits are initialized to zero

另外,在 eBPF 官网上,也有直接初始化的写法:

@jschwinger233
Copy link
Member

@Integral-Tech 还有一种情况是结构体本身有空洞,比如

struct A {
    u8 a
    u32 b
}

为了内存对齐这个结构体会在两个字段之间插入 padding,这时候就算直接所有字段都直接赋值也会有空洞,verifier 会拒绝。

@jschwinger233
Copy link
Member

还有一点是逐字段赋值能 CO-RE,但是直接初始化不能。

@Integral-Tech
Copy link
Contributor Author

Integral-Tech commented Sep 14, 2024

@Integral-Tech 还有一种情况是结构体本身有空洞,比如

struct A {
    u8 a
    u32 b
}

为了内存对齐这个结构体会在两个字段之间插入 padding,这时候就算直接所有字段都直接赋值也会有空洞,verifier 会拒绝。

所以一般建议所有情况都先初始化为 {}{ 0 },然后再逐字段赋值?

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.

3 participants