[RFC,V1,2/2] arm_boot: conditionalised dtb command line update

The dtb command line should only be overwritten if the user provides a command
line. Otherwise whatever command line was in the dtb should stay unchanged.

Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
---
 hw/arm_boot.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

On 1 June 2012 02:16, Peter A. G. Crosthwaite
<peter.crosthwaite@petalogix.com> wrote:
> @@ -240,10 +241,13 @@ static int load_dtb(target_phys_addr_t addr, const struct arm_boot_info *binfo)
>         fprintf(stderr, "couldn't set /memory/reg\n");
>     }
>
> -    rc = qemu_devtree_setprop_string(fdt, "/chosen", "bootargs",
> -                                      binfo->kernel_cmdline);
> -    if (rc < 0) {
> -        fprintf(stderr, "couldn't set /chosen/bootargs\n");
> +    machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
> +    if (machine_opts && qemu_opt_get(machine_opts, "append")) {
> +        rc = qemu_devtree_setprop_string(fdt, "/chosen", "bootargs",
> +                                          binfo->kernel_cmdline);
> +        if (rc < 0) {
> +            fprintf(stderr, "couldn't set /chosen/bootargs\n");
> +        }
>     }

Can you just check for binfo->kernel_cmdline being NULL or not
rather than rereading the option via qemu_opt_get? The latter
seems pretty ugly.

-- PMM

On Fri, Jun 1, 2012 at 11:44 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 1 June 2012 02:16, Peter A. G. Crosthwaite
> <peter.crosthwaite@petalogix.com> wrote:
>> @@ -240,10 +241,13 @@ static int load_dtb(target_phys_addr_t addr, const struct arm_boot_info *binfo)
>>         fprintf(stderr, "couldn't set /memory/reg\n");
>>     }
>>
>> -    rc = qemu_devtree_setprop_string(fdt, "/chosen", "bootargs",
>> -                                      binfo->kernel_cmdline);
>> -    if (rc < 0) {
>> -        fprintf(stderr, "couldn't set /chosen/bootargs\n");
>> +    machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>> +    if (machine_opts && qemu_opt_get(machine_opts, "append")) {
>> +        rc = qemu_devtree_setprop_string(fdt, "/chosen", "bootargs",
>> +                                          binfo->kernel_cmdline);
>> +        if (rc < 0) {
>> +            fprintf(stderr, "couldn't set /chosen/bootargs\n");
>> +        }
>>     }
>
> Can you just check for binfo->kernel_cmdline being NULL or not
> rather than rereading the option via qemu_opt_get? The latter
> seems pretty ugly.
>

No, it wont work,

vl.c will populate it with "\"\"":

    if (machine_opts) {
...
        kernel_cmdline = qemu_opt_get(machine_opts, "append");
    } else {
        kernel_filename = initrd_filename = kernel_cmdline = NULL;
    }

    if (!kernel_cmdline) {
        kernel_cmdline = "";
    }


binfo->kernel_cmdline will not be null on the omission of -append.

I did it this way as its the only way I can see where you can
determine whether or not -apend happened. You could strcmp with "\"\""
and use that as your condition, but then you have a possibly piece of
policy that an empty command means no update (i.e. you should still be
able to explictly -apend "\"\"" to wipe out the dtb command line).

Regards,

Peter

> -- PMM

On Fri, Jun 1, 2012 at 11:56 AM, Peter Crosthwaite
<peter.crosthwaite@petalogix.com> wrote:
> On Fri, Jun 1, 2012 at 11:44 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 1 June 2012 02:16, Peter A. G. Crosthwaite
>> <peter.crosthwaite@petalogix.com> wrote:
>>> @@ -240,10 +241,13 @@ static int load_dtb(target_phys_addr_t addr, const struct arm_boot_info *binfo)
>>>         fprintf(stderr, "couldn't set /memory/reg\n");
>>>     }
>>>
>>> -    rc = qemu_devtree_setprop_string(fdt, "/chosen", "bootargs",
>>> -                                      binfo->kernel_cmdline);
>>> -    if (rc < 0) {
>>> -        fprintf(stderr, "couldn't set /chosen/bootargs\n");
>>> +    machine_opts = qemu_opts_find(qemu_find_opts("machine"), 0);
>>> +    if (machine_opts && qemu_opt_get(machine_opts, "append")) {
>>> +        rc = qemu_devtree_setprop_string(fdt, "/chosen", "bootargs",
>>> +                                          binfo->kernel_cmdline);
>>> +        if (rc < 0) {
>>> +            fprintf(stderr, "couldn't set /chosen/bootargs\n");
>>> +        }
>>>     }
>>
>> Can you just check for binfo->kernel_cmdline being NULL or not
>> rather than rereading the option via qemu_opt_get? The latter
>> seems pretty ugly.
>>
>
> No, it wont work,
>
> vl.c will populate it with "\"\"":
>

Correction,

vl.c will populate it with an empty string. The problem still stands
though that an empty string explicitly passed to -append should mean
wipe out dtb command line.

>    if (machine_opts) {
> ...
>        kernel_cmdline = qemu_opt_get(machine_opts, "append");
>    } else {
>        kernel_filename = initrd_filename = kernel_cmdline = NULL;
>    }
>
>    if (!kernel_cmdline) {
>        kernel_cmdline = "";
>    }
>
>
> binfo->kernel_cmdline will not be null on the omission of -append.
>
> I did it this way as its the only way I can see where you can
> determine whether or not -apend happened. You could strcmp with "\"\""
> and use that as your condition, but then you have a possibly piece of
> policy that an empty command means no update (i.e. you should still be
> able to explictly -apend "\"\"" to wipe out the dtb command line).
>
> Regards,
>
> Peter
>
>> -- PMM