Re: [patch 0/4] elilo multiboot support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Attachment: pgpoaGXOtu3Fl.pgp
Description: PGP signature

--

Fedora-xen@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/fedora-xen

[Index of Archives]     [Fedora General]     [Fedora Music]     [Linux Kernel]     [Fedora Desktop]     [Fedora Directory]     [PAM]     [Big List of Linux Books]     [Gimp]     [Yosemite News]

  Powered by Linux