Messages in this thread | | | Date | Fri, 26 Aug 2022 21:10:21 +0200 | Subject | Re: [PATCH v12 02/10] btf: Handle dynamic pointer parameter in kfuncs | From | Roberto Sassu <> |
| |
On 8/26/2022 6:41 PM, Jarkko Sakkinen wrote: > On Fri, Aug 26, 2022 at 07:32:54PM +0300, Jarkko Sakkinen wrote: >> On Fri, Aug 26, 2022 at 05:34:57PM +0200, Roberto Sassu wrote: >>> On Fri, 2022-08-26 at 17:43 +0300, Jarkko Sakkinen wrote: >>>> On Fri, Aug 26, 2022 at 08:46:14AM +0300, Jarkko Sakkinen wrote: >>>>> On Thu, Aug 25, 2022 at 10:16:14PM -0700, Alexei Starovoitov wrote: >>>>>> On Thu, Aug 25, 2022 at 9:54 PM Jarkko Sakkinen < >>>>>> jarkko@kernel.org> wrote: >>>>>>>> -static bool is_dynptr_reg_valid_init(struct bpf_verifier_env >>>>>>>> *env, struct bpf_reg_state *reg, >>>>>>>> - enum bpf_arg_type >>>>>>>> arg_type) >>>>>>>> +bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, >>>>>>>> struct bpf_reg_state *reg, >>>>>>>> + enum bpf_arg_type arg_type) >>>>>>>> { >>>>>>>> struct bpf_func_state *state = func(env, reg); >>>>>>>> int spi = get_spi(reg->off); >>>>>>>> -- >>>>>>>> 2.25.1 >>>>>>>> >>>>>>> >>>>>>> Might be niticking but generally I'd consider splitting >>>>>>> exports as commits of their own. >>>>>> >>>>>> -static bool >>>>>> +bool >>>>>> >>>>>> into a separate commit? >>>>>> >>>>>> I guess it makes sense for people whose salary depends on >>>>>> number of commits. >>>>>> We don't play these games. >>>>> >>>>> What kind of argument is that anyway. >>>> >>>> "Separate each *logical change* into a separate patch." [*] >>> >>> The logical change, as per the patch subject, is allowing the >>> possibility of including eBPF dynamic pointers in a kfunc definition. >>> It requires to call an existing function that was already defined >>> elsewhere. >>> >>> Maybe I'm wrong, but I don't see only exporting a function definition >>> to an include file as a logical change. To me, the changes in this >>> patch are clearly connected. Or even better, they tell why the function >>> definition has been exported, that would not appear if moving the >>> function definition is a standalone patch. >>> >>>> >>>> To add, generally any user space visible space should be an >>>> isolated patch. >>> >>> As far as I understood, definitions visible to user space should be in >>> include/uapi. >> >> It does change e.g. the output of kallsyms. >> >> It's not ABI but it's still user space visble. >> >>> >>>> >>>> Please, stop posting nonsense. >>> >>> If I may, saying this does not encourage people to try to submit their >>> code. I feel it is a bit strong, and I kindly ask you to express your >>> opinion in a more gentle way. >> >> I agree. That's why I was wondering what is this nonsense >> about salary and games. > > Please denote that I started my review with "Might be nitpicking...". > > It's neither particularly disencouraging nor enforcing for anyone.
Thanks for clarifying. Yes, it is not. Sorry, I misunderstood.
Roberto
| |