[syslinux] [RFC][PATCH 4/4] elflink: Move cli lib code out of core

[Matt Fleming]

On Wed, 23 Feb 2011 08:00:22 -0500
Jim Cromie <jim.cromie at gmail.com> wrote:
 
> hi Matt,
> 
> in 0/4 msg, you RFC'd your approach here.
> Is there something specific you thought needed discussion ?
> Im not active here, but nothing struck me as less than straight
> forward,

I was just trying to gather some opinions on whether the approach I
took in [PATCH 4/4] was one that other people thought was OK. If you
think it's straight forward then great! However, Peter raised a few
concerns about it on IRC that are worth repeating here,

1. More functionality should be moved out of the core.

I initially planned on doing the move in a few stages but there's
really no reason to do it that way, and instead I'm gonna end up doing
it as one patch. As an example, the configuration parser also needs
moving out of the core.

2. Having many modules complicates the failure case

If many modules are required to get a working system then it becomes
much more likely that you'll forget to compile/install one, thereby
breaking your system. Worse still, the missing module may not be
apparent until you try to use a certain feature, say, the config
parser. Instead, Peter suggested creating a "ldlinux.c32" module that
contains _all_ of the features that do not absolutely have to be in
the core proper. This is a much more sensible solution since if
ldlinux.c32 cannot be found in the install directory it is no different
than not being able to find ldlinux.sys, e.g. your installation is
hosed.

> except perhaps a git usage question:
> 
> 
> >  com32/elflink/modules/cli.c     |  437
> > +++++++++++++++++++++++++++++++++++++++
> >  core/elflink/cli.c              |  410
> > ------------------------------------
> >
> 
> I looked at those 2 lines, and wondered why git didnt show them as a
> rename. If this were done in 2 separate steps, would it appear as a
> rename automatically,
> or would you need to do something specific/different ?
> Would 2 steps have left you with a non-functional revision, bad for
> bisect ? Is there a simple way to reformulate your patch to make it
> both a rename, and bisectable ?

Hmm.. I suspect the way I generated the patch caused git not to display
this as a rename. I'm pretty sure that if you applied the patch and did
a `git diff --stat -M` it would show it as a rename. I'm probably
missing something in my git config options for generating patches. Oh,
looks like -M is the command-line option I missed for git format-patch.

> Oh. and in Makefile, you added cli.32 to an otherwize unchanged line,
> and added 2 new modules to newline below it.  I wasted a few seconds
> scanning the -+ lines for the difference, having expected just 1 new
> line.

Right, this was because the single [PATCH 4/4] was originally split out
into more patches that each added a single .c32 module to the Makefile.

I'm gonna do another version of this patch that addresses Peter's
comments. Thanks for the review!

-- 
Matt Fleming, Intel Open Source Technology Center