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

[Sören Brinkmann]

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.

What would be your suggested change?

        Soren