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
pgpqSkPXTFHF0.pgp
Description: PGP signature
|
