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");
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.
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
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);
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
-
BALATON Zoltan -
Mark Cave-Ayland