John Hubbard
2025-Dec-01 22:52 UTC
[PATCH v3] rust: clist: Add support to interface with C linked lists
On 12/1/25 2:43 PM, Joel Fernandes wrote:> On 12/1/2025 5:17 PM, John Hubbard wrote: >> On 12/1/25 12:32 PM, Joel Fernandes wrote: >>> On 11/30/2025 7:34 PM, John Hubbard wrote: >>>> On 11/29/25 1:30 PM, Joel Fernandes wrote: >> ... > You may want to read [1]. CONFIG_RUST_KERNEL_DOCTESTS are run at runtime. You > enable it and boot the kernel. The documentation clearly says "doctests get > compiled as Rust kernel objects, allowing them to run against a built kernel.". > And this is how I have run it as well. > > [1] https://docs.kernel.org/rust/testing.html > > This also explains why you think list_add_tail() is a noop in my patch, which it > is not.Yes, I forgot that they are actually run, you are right.> >> >> I would humbly suggest that you build and *run* your own samples code, for >> new code that has no users yet. > > Yes, I already have an internal tree running it. :) I am not sure why the > assume_init() triggered for you but not for me, I don't think has anything to do > with doctests since the doctests is in fact just rust code compiled as KUNIT tests.I think it's because I wrote separate code that was not a doctest, and that code is naturally different from however the doctest exercised it. But it is a good question.> >> Because if you are skipping steps like this (posting the code before >> there is an actual caller), then the documentation of how to use it >> is not "just documentation" anymore--it really needs to run correctly. > > No, that's the thing, these are run. You really are in the wrong here and appear > to not understand how doctests work.That's a reasonable statement. :)> >> And actually, after writing the above...I still think it would be better >> to post this with its first caller (DRM_BUDDY, or BUDDY_DRM_ALUMNI, or >> however it ends up), so that we can see how it looks and behaves in >> practice. >> >> What's the rush? > > Who said anything about a rush? I am really confused by what you mean. It is > useful to post patches even if there are external dependencies to get feedback. > So this is also an invalid review comment unfortunately. There is no rush, this > is v3 now, did you miss that? >I mean, doctests are far weaker than actual code that uses the new API. It feels rushed to propose merging code without a caller. And I don't think doctests are a "real enough" caller. thanks, -- John Hubbard
Joel Fernandes
2025-Dec-01 23:09 UTC
[PATCH v3] rust: clist: Add support to interface with C linked lists
On 12/1/2025 5:52 PM, John Hubbard wrote:>>> And actually, after writing the above...I still think it would be better >>> to post this with its first caller (DRM_BUDDY, or BUDDY_DRM_ALUMNI, or >>> however it ends up), so that we can see how it looks and behaves in >>> practice. >>> >>> What's the rush? >> Who said anything about a rush? I am really confused by what you mean. It is >> useful to post patches even if there are external dependencies to get feedback. >> So this is also an invalid review comment unfortunately. There is no rush, this >> is v3 now, did you miss that? >> > I mean, doctests are far weaker than actual code that uses the new API. > It feels rushed to propose merging code without a caller. And I don't > think doctests are a "real enough" caller.Actually I was already rebasing my DRM buddy bindings patches today. So the next version was already going to be with the actual DRM buddy bindings (inclusive of the clist patches), now that clist has mostly settled. The point of posting the clist series was to focus on just that part and get it right. If you notice, my first version included the DRM buddy user as well but clist required a lot of changes first. I don't think one needs to include all users in a series if the series is sufficiently complex (as long as you posted the user or share a tree using it - which I already did in the v1). Then the maintainers can decide if it needs to be pulled in advance or with the user. That's really up to a maintainer. I certainly want clist to merged only once the drm buddy bindings go with it - why else would we want to do that? There is absolutely no reason. I am unable to find where you go the idea that I was proposing merging clist without the drm buddy bindings - there is little reason to do that considering clist.rs is mostly independent of other things and is really easy to rebase. thanks, - Joel