On 12/15/23 17:19, Martin KaFai Lau wrote:
On 12/15/23 1:42 PM, Kui-Feng Lee wrote:
On 12/14/23 18:22, Martin KaFai Lau wrote:
On 12/8/23 4:26 PM, thinker.li@xxxxxxxxx wrote:
+const struct bpf_struct_ops_desc *btf_get_struct_ops(struct btf
*btf, u32 *ret_cnt)
+{
+ if (!btf)
+ return NULL;
+ if (!btf->struct_ops_tab)
*ret_cnt = 0;
unless the later patch checks the return value NULL before using
*ret_cnt.
Anyway, better to set *ret_cnt to 0 if the btf has no struct_ops.
The same should go for the "!btf" case above but I suspect the above
!btf check is unnecessary also and the caller should have checked for
!btf itself instead of expecting a list of struct_ops from a NULL
btf. Lets continue the review on the later patches for now to confirm
where the above !btf case might happen.
Checking callers, I didn't find anything that make btf here NULL so far.
It is safe to remove !btf check. For the same reason as assigning
*ret_cnt for safe, this check should be fine here as well, right?
If for safety, why ref_cnt is not checked for NULL also? The userspace
passed-in btf should have been checked for NULL long time before
reaching here. There is no need to be over protective here. It would
really need a BUG_ON instead if btf was NULL here (don't add a BUG_ON
though).
afaict, no function in btf.c is checking the btf argument for NULL also.
I don't have strong opinion here. What I though is to keep the values
as it is without any side-effect if the function call fails and if
possible. And, the callers should not expect the callee to set some
specific values when a call fails.
For *ref_cnt stay uninit, there is a bug in patch 10 which exactly
assumes 0 is set in *ret_cnt when there is no struct_ops. It is a good
signal on how this function will be used.
I think it is arguable whether returning NULL here is failure. I would
argue getting a 0 struct_ops_desc array is not a failure. It is why the
!btf case confuses the return NULL case to mean a never would happen
error instead of meaning there is no struct_ops. Taking out the !btf
case, NULL means there is no struct_ops (instead of failure), so 0 cnt.
Anyhow, either assign 0 to *ret_cnt here, or fix patch 10 to init the
local cnt 0 and write a warning comment here in btf_get_struct_ops()
saying ret_cnt won't be set when there is no struct_ops in the btf.
I will fix at the patch 10 by setting local cnt 0.
When looking at it again, how about moving the bpf_struct_ops_find_*()
to btf.c. Then it will avoid the need of the new btf_get_struct_ops()
function. bpf_struct_ops_find_*() can directly use the btf->struct_ops_tab.
I prefer to keep them in bpf_struct_ops.c if it is ok to you.
Fixing the initialization issue of bpf_struct_ops_find()
should be enough.
+ return NULL;
+
+ *ret_cnt = btf->struct_ops_tab->cnt;
+ return (const struct bpf_struct_ops_desc
*)btf->struct_ops_tab->ops;
+}
