request for a new port review [memtest86+]

[Andriy Gapon]

on 11/03/2009 16:52 Wesley Shields said the following:
> On Tue, Mar 10, 2009 at 07:49:27PM +0200, Andriy Gapon wrote:
>> Guys,
>>
>> could you please review the below port for correctness, style and
>> general approach taken by me.  This is a port of memtest86+. Unlike
>> existing sysutils/memtest86 this port is not a
>> download/extraction/version-tracking aid, rather it is designed to
>> build a stand-alone ELF image (bootable by boot2 or loader) and/or an
>> ISO image from sources.  The first option does not need any additional
>> justification, I think. The second option can be useful if you want to
>> add some local patches on top of vendor sources.
>>
>> I am very grateful for the idea for this port and many technical
>> details of it to Stephan Eisvogel. I also thank Eygene Ryabinkin for
>> teaching me some things about ld.
>>
>> Alternatives for /boot/opt are welcome :)
>>
>> P.S. Stephan, I plan to create a distinct port for your version very
>> soon.  I am also considering making it a port option for this proposed
>> port, the option that would apply an extra patch.
> 
> You seem to be using two tabs on every line.  I think you should use
> only one.  Just a style nit.

Good point, I used to have a variable with a long name but it's gone, so I'll
condense the assignments.

> I don't know if /boot/opt is the best place for it.  It seems like
> /boot/local fits in better with the concept of /usr/local.  Of course,
> it's just a name and I'm not attached strongly to any of them.

Me neither. I opted for "opt" :-) because it's shorter and right now there is no
auto-completion for boot2 or loader prompts.

> What's wrong with making these changes to the existing memtest port, as
> OPTIONS?

Two reasons:
(1) memtest seems to be tailored to a different purpose and it currently has a
maintainer whom I haven't consulted about any changes yet;
(2) [more important] original memtest86 has a peculiarity that it needs to be
loaded in conventional memory (first 640k) and that doesn't work very well with
our loader (yet); I have some ideas, but they need additional work, so memtest86+
was the lower-hanging fruit.

-- 
Andriy Gapon