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

[Peter Maydell]

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:

Reviewed-by: Peter Maydell <address@hidden>

-- PMM