> > How about bpf_copy_from_user_task() ? > > The task is the second to last argument, so the name fits ? > > yeah, I like the name I'll change the name to `bpf_copy_from_user_task`. > > Especially if we call it this way it would be best to align > > return codes with bpf_copy_from_user. > > Adding memset() in case of failure is mandatory too. > > I've missed this bit earlier. > > Yep, good catch! Seems like copy_from_user() currently returns amount > of bytes *not* read and memsets those unread bytes to zero. So for > efficiency we could probably memset only those that were read. > > > The question is to decide what to do with > > ret > 0 && ret < size condition. > > Is it a failure and we should memset() the whole buffer and > > return -EFAULT or memset only the leftover bytes and return 0? > > I think the former is best to align with bpf_copy_from_user. > > Yeah, I think all or nothing approach (either complete success and > zero return, or memset and error return) is best and most in line with > other similar helpers. Thanks for the suggestions! I'll go with the all-or-nothing approach to be consistent with `bpf_copy_from_user` and will make the following changes: * Return value: returns 0 on success, or negative error on failure. * If we had a partial read, we will memset the read bytes to 0 and return -EFAULT > Another thing, I think it's important to mention that this helper can > be used only from sleepable BPF programs. Will add that to the docs. > > That would be difficult. There is no suitable kernel api for that. > > Ok, but maybe we can add it later. Otherwise it will be hard to > profiler Python processes and such, because you most certainly will > need to read zero-terminated strings there. I will NOT add a C string helper in this patch series, and I'll explore how to add this in the future once this patch series is merged. > > + skel = bpf_iter_task__open_and_load(); > > + if (CHECK(!skel, "bpf_iter_task__open_and_load", > > + "skeleton open_and_load failed\n")) > > Please use ASSERT_OK_PTR() instead. Will fix. > > + numread = bpf_access_process_vm(&user_data, > > + sizeof(uint32_t), > > + ptr, > > + task, > > + 0); > > nit: keep it on one line (up to 100 characters is ok) Will fix. Thanks for the suggestions everyone! Kenny
