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

[Sören Brinkmann]

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.
I introduced making it mandatory for the ramdisk case.

> 
> > What would be your suggested change?
> 
> I don't mind as long as we're consistent one way or the other
> about whether non-optional pointer arguments are checked for
> NULL.
As I said, depending on whether we call this function to load a kernel
or ramdisk that argument is optional or mandatory.
When it's optional we do a NULL check. In the mandatory case the caller
ensures validity.

So, if leaving it as is, is not an option.
How about this:
Moving the NULL check to load_kernel() and passing an always valid
pointer to load_uboot_image() and removing the NULL check there?
Then load_kernel() can pass on this value or not depending on the
validity of the pointer it received.

        Soren