Re: [Qemu-devel] [PATCH v3 1/2] hw/loader: Support ramdisk with u-boot header

[Sören Brinkmann]

On Sat, Jul 20, 2013 at 12:20:48AM +0100, Peter Maydell wrote:
> On 19 July 2013 18:53, Sören Brinkmann <address@hidden> wrote:
> > On Fri, Jul 19, 2013 at 06:46:57PM +0100, Peter Maydell wrote:
> >> On 19 July 2013 18:39, Sören Brinkmann <address@hidden> wrote:
> >> > On Fri, Jul 19, 2013 at 01:04:20PM +0100, Peter Maydell wrote:
> >> >> On 8 July 2013 23:40, Soren Brinkmann <address@hidden> wrote:
> >> >> > +
> >> >> > +        if (ep) {
> >> >> > +            *ep = hdr->ih_ep;
> >> >> > +        }
> >> >>
> >> >> (Allowing ep to be NULL for IH_TYPE_KERNEL is new behaviour,
> >> >> but it makes sense for consistency with the other pointer
> >> >> argument handling.)
> >> >>
> >> >> > +
> >> >> > +        /* TODO: Check CPU type.  */
> >> >> > +        if (is_linux) {
> >> >> > +            if (hdr->ih_os == IH_OS_LINUX) {
> >> >> > +                *is_linux = 1;
> >> >> > +            } else {
> >> >> > +                *is_linux = 0;
> >> >> > +            }
> >> >> > +        }
> >> >> > +
> >> >> > +        break;
> >> >> > +    case IH_TYPE_RAMDISK:
> >> >> > +        address = *loadaddr;
> >> >>
> >> >> This is inconsistent -- for IH_TYPE_KERNEL we accept
> >> >> a NULL loadaddr, but for IH_TYPE_RAMDISK we don't.
> >> > The thing is in case of a ramdisk, it's a mandatory input argument which 
> >> > has
> >> > to be provided, whereas for the kernel, it's an optional output value.
> >> > And other than the load_ramdisk() and load_kernel() routines nobody is
> >> > supposed to call this function directly, IMHO. And I think plausibility
> >> > checks should be done in those routines when possible. And
> >> > load_ramdisk() ensures that the loadaddr pointer is valid.
> >>
> >> Well, by that argument you shouldn't introduce the "allow
> >> ep to be NULL" change...
> > I didn't introduce it, that is the current state for loading a kernel.
> 
> Current code:
>     *ep = hdr->ih_ep;
> 
> Your patch:
> +        if (ep) {
> +            *ep = hdr->ih_ep;
> +        }
> 
> On reflection I feel like this is making a mountain out of
> a molehill, though, so:
I'm sorry you're right. I was looking at 'loadaddr' and 'is_linux'. Looking
at ep, I wonder why I changed it, too.
Maybe I wanted to make it a little more robust against wrongly calling
load_ramdisk() with a kernel image. In my original patch that would have
gone through and resulted in a seg-fault because of a missing NULL
check, I think. And it would have been easily triggered just by mixing
up the '-initrd' and '-kernel' command line parameters.
But in v2, I added your proposed header type checking. So, this error
scenario would be recognized properly and the check for 'ep' is
obsolete.

Up to you, leaving as is and have consistent NULL checking on all
pointer arguments, or changing this back to what it was?

        Sören