On 3/29/23 1:44 PM, Linus Torvalds wrote: > On Wed, Mar 29, 2023 at 11:41 AM Jens Axboe <axboe@xxxxxxxxx> wrote: >> >> Passes testing, and verified we do the right thing for 1 and multi >> segments. > > Apart from the pointer casting rant, this looks sane to me. > > I feel like 02/11 has a few potential cleanups: > > (a) it feels like a few too many "iter.__iov" uses remaining, but > they mostly (all?) look like assignments. > > I do get the feeling that any time you assign __iov, you should also > assign "nr_segs", and it worries me a bit that I see one without the > other. Maybe room for another helper that enforces a "if you set the > __iov pointer, you must be setting nr_segs too"? > > And maybe I'm just being difficult. No, I think that's valid, and the cover letter does touch upon that. The thought of doing an iov assign helper has occurred to me as well. I just wanted to get general feelings on the direction first, then do a round of polish when prudent rather than prematurely. > (b) I see at least one "iov = iter_iov(from)" that precedes a later > check for "iter_is_iovec()", which again means that *if* we add some > debug sanity test to "iter_iov()", it might trigger when it shouldn't? > > The one I see is in snd_pcm_writev(), but I th ink the same thing > happens in snd_pcm_readv() but just isn't visible in the patch due to > not having the context lines. I think that's mostly a patch ordering issue. Should probably just push the sound and IB patches to the front of the series. -- Jens Axboe
