[Fedora-xen] Re: [patch 0/4] elilo multiboot support

[Aron Griffis]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/fedora-xen/attachments/20060626/a98f66e6/attachment.sig>