[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 1/2] hw/loader: Support ramdisk with u-boot h
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH v3 1/2] hw/loader: Support ramdisk with u-boot header |
Date: |
Fri, 19 Jul 2013 18:46:57 +0100 |
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...
> 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.
-- PMM
[Qemu-devel] [PATCH v3 2/2] hw/arm: Use 'load_ramdisk()' for loading ramdisks w/ U-Boot header, Soren Brinkmann, 2013/07/08
Re: [Qemu-devel] [PATCH v3 0/2] Support ramdisks with U-Boot header, Sören Brinkmann, 2013/07/17