Jeremy,
Jeremy Katz wrote: [Mon Jun 26 2006, 03:21:18PM EDT]
Overall, these look pretty good.
Good to hear. :-)
A few stylistic-ish things
* The patches don't work individually -- applying them in sequence ends
up with some not compiling bits. Was pretty easy to fix up, but worth
noting
Thanks, and sorry for the inconvenience.
* dprintf is already included in the standard library as a GNU
extension, but with a different functionality -- something like
dbgprintf would be better just to avoid problems there
Okay
* Instead of having the getLineByType2 thing, it would probably be a
little cleaner to just change the types to be a normal int instead of an
enum and then be able to determine matches via bitwise operators
Heh, I wanted to do this but was already nervous about the volume of
code being changed. I agree it would be better. I'll change this for
the next submission.
In the bigger realm, there seems to be a bug or two lingering.
Running
the test suite ('make test' from the toplevel) seems to fail for
x86/x86_64 with the patches applied -- it looks like we're gaining an
initrd in cases where it's unexpected (copy-default shouldn't be copying
the initrd, but it looks like it might be). Also, it looks like kernel
arguments might be getting lost in the multiboot case. I'll try to look
at this a little more later in the afternoon, but given how the past two
weeks have been "later in the afternoon" could end up being Friday :/
I hadn't even noticed the existence of the test suite, silly me. :-|
My testing had all been manual. I'll run through the test suite and
fix up the errors.
Also, it'd be nice to have some test cases to add to the test
suite just
so that we can more easily ensure that minor bugfixes don't cause
regressions.
Will do. Thanks for the review.
Regards,
Aron