Messages in this thread | | | From | Rusty Russell <> | Subject | Re: [PATCH 2/2] module: fix bne2 "gave up waiting for init of module libcrc32c" | Date | Thu, 3 Jun 2010 14:50:52 +0930 |
| |
On Thu, 3 Jun 2010 03:31:06 am Linus Torvalds wrote: > > On Wed, 2 Jun 2010, Rusty Russell wrote: > > > > And load_module is down to 259 lines. The label chain at the end is no > > shorter tho :( I'll leave those cleanups until next merge window. > > Btw, here's a patch that _looks_ large, but it really pretty trivial, and > sets things up so that it would be way easier to split off pieces of the > module loading. > > The reason it looks large is that it creates a "module_info" structure > that contains all the module state that we're building up while loading, > instead of having individual variables for all the indices etc. > > So the patch ends up being large, because every "symindex" access instead > becomes "info.index.sym" etc. That may be a few characters longer, but it > then means that we can just pass a pointer to that "info" structure > around. and let all the pieces fill it in very naturally. > > As an example of that, the patch also moves the initialization of all > those convenience variables into a "setup_module_info()" function. And at > this point it really does become very natural to start to peel off some of > the error labels and move them into the helper functions - now the > "truncated" case is gone, and is handled inside that setup function > instead. > > So maybe you don't like this approach, and it does make the variable > accesses a bit longer, but I don't think unreadably so. And the patch > really does look big and scary, but there really should be absolutely no > semantic changes - most of it was a trivial and mindless rename. > > In fact, it was so mindless that I on purpose kept the existing helper > functions looking like this: > > - err = check_modinfo(mod, sechdrs, infoindex, versindex); > + err = check_modinfo(mod, info.sechdrs, info.index.info, info.index.vers); > > rather than changing them to just take the "info" pointer. IOW, a second > phase (if you think the approach is ok) would change that calling > convention to just do > > err = check_modinfo(mod, &info); > > (and same for "layout_sections()", "layout_symtabs()" etc.) Similarly, > while right now it makes things _look_ bigger, with things like this: > > versindex = find_sec(hdr, sechdrs, secstrings, "__versions"); > > becoming > > info->index.vers = find_sec(info->hdr, info->sechdrs, info->secstrings, "__versions"); > > in the new "setup_module_info()" function, that's again just a result of > it being a search-and-replace patch. By using the 'info' pointer, we could > just change the 'find_sec()' interface so that it ends up being > > info->index.vers = find_sec(info, "__versions"); > > instead, and then we'd actually have a shorter and more readable line. So > for a lot of those mindless variable name expansions there's would be room > for separate cleanups. > > I didn't move quite everything in there - if we do this to layout_symtabs, > for example, we'd want to move the percpu, symoffs, stroffs, *strmap > variables to be fields in that module_info structure too. But that's a > much smaller patch, I moved just the really core stuff that is currently > being set up and used in various parts. > > But even in this rough form, it removes close to 70 lines from that > function (but adds 22 lines overall, of course - the structure definition, > the helper function declarations and call-sites etc etc).
Applied. I thought about the same thing but had the same doubts as you.
However, you're right that it has potential. I'll rename module_info to load_info if you don't mind tho: contains more semantic punch IMHO.
On top of this, I'm right now closing on another ideal of mine: encapsulate all the "before we move module" into one function. That before vs. after always made me nervous...
Thanks! Rusty.
| |