Re: [OpenBIOS] [PATCH] arch/ppc/qemu: do not override boot-device if already set

[Mark Cave-Ayland]

On 18/05/14 00:18, BALATON Zoltan wrote:
> Only set the boot-device env variable if it is not yet set, do not
> override values set e.g. by -prom-env option of QEMU.
>
> Signed-off-by: BALATON Zoltan <balaton at eik.bme.hu>

Obfuscation? ;)
> Index: openbios-devel/arch/ppc/qemu/init.c
>
> --- openbios-devel/arch/ppc/qemu/init.c	(revision 1298)
> +++ openbios-devel/arch/ppc/qemu/init.c	(working copy)
> @@ -659,7 +689,7 @@
>       char buf[64], qemu_uuid[16];
>       const char *stdin_path, *stdout_path, *boot_path;
>       uint32_t temp = 0;
>
> char *boot_device;
>  ofmem_t *ofmem = ofmem_arch_get_private();
>
> openbios_init();
>
> @@ -867,24 +903,28 @@
>       push_str("/options");
>       fword("find-device");
> uint16_t boot_device = fw_cfg_read_i16(FW_CFG_BOOT_DEVICE);
> switch (boot_device) {
>    case 'c':
>        boot_path = "hd";
>        break;
>    default:
>    case 'd':
>        boot_path = "cd";
>        break;
> /* Setup default boot devices (not overriding user settings) */
> fword("boot-device");
> boot_device = pop_fstr_copy();
> if (strcmp(boot_device, "disk") == 0) {

What is boot-device normally set to if no override is specified? 
pop_fstr_copy() has a little gotcha in that a zero length string becomes 
a NULL on conversion, so generally I tend to use a pattern like this:

if (boot_device && strcmp(boot_device, "disk") == 0) {
        ...
    }

I'm just wondering if we shouldn't always assume that a value exists and 
is non-zero...
>    switch (fw_cfg_read_i16(FW_CFG_BOOT_DEVICE)) {
>        case 'c':
>            boot_path = "hd";
>            break;
>        default:
>        case 'd':
>            boot_path = "cd";
>            break;
>    }
>    snprintf(buf, sizeof(buf), "%s:,\\\\:tbxi %s:,\\ppc\\bootinfo.txt %s:,%%BOOT", boot_path, boot_path, boot_path);
>    push_str(buf);
>    fword("encode-string");
>    push_str("boot-device");
>    fword("property");
> }
> free(boot_device);
>
> /* Setup default boot devices */
>
> snprintf(buf, sizeof(buf), "%s:,\\:tbxi %s:,\ppc\bootinfo.txt %s:,%%BOOT", boot_path, boot_path, boot_path);
>
> push_str(buf);
>
> fword("encode-string");
>
> push_str("boot-device");
>
> fword("property");
> /* Set up other properties */
>
> push_str("/chosen");

Other than that, looks reasonable to me and I'd be okay to apply this 
unless someone vociferously objects.

ATB,

Mark.