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

[BALATON Zoltan]

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>

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) {
+        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");

[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.

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

[BALATON Zoltan]

On Sun, 18 May 2014, Mark Cave-Ayland wrote:
> On 18/05/14 00:18, BALATON Zoltan wrote:
> > Signed-off-by: BALATON Zoltan <balaton at eik.bme.hu>
>
> Obfuscation? ;)

I've copied it from the web archives (I still can't subscribe to the list 
by the way). I will correct it in the next version.
> > 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?

By default it's set to "disk" in forth/admin/nvram.fs I think.
> 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 did not test that the user can set it to "" but if so I agree it's 
better to check for a NULL value. I'll test this and send a v2.
> I'm just wondering if we shouldn't always assume that a value exists and is 
> non-zero...

So you mean that the default value should also be set when it's empty not 
only when not set explicitely? Now I wonder if the default should be 
changed to empty instead but I don't know what else would that affect.

Regards,
BALATON Zoltan

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

[BALATON Zoltan]

On Sun, 18 May 2014, Mark Cave-Ayland wrote:
> On 18/05/14 00:18, BALATON Zoltan wrote:
> > +    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:

I've tested it and the user can specify an empty string by not giving any 
value "-prom-env boot-device=" but this does not lead to a crash because 
the NULL value is only used in strcmp where it's not equal and in free 
where it does nothing. So you could apply this patch as is (with 
deobfuscating my email address in the commit message) unless you want 
me to do some other changes.

Regards,
BALATON Zoltan
> 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);

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

[BALATON Zoltan]

On Sun, 18 May 2014, BALATON Zoltan wrote:
> value "-prom-env boot-device=" but this does not lead to a crash because the 
> NULL value is only used in strcmp where it's not equal and in free where it

Now I looked at the strcmp implementation just to double check and it 
seems to dereference the pointer without checking for NULL. It does not 
crash and seems to work but better to not call it like that so I'm 
resending with your suggestion. Sorry about the noise.

Regards,
BALATON Zoltan