overdue-scratch

Author Topic: Support for volume UUID and/or device tree lookup in "Default Partition"  (Read 19530 times)

0 Members and 1 Guest are viewing this topic.

danielkza

  • VoodooLabs
  • Posts: 9
Re: Support for volume UUID and/or device tree lookup in "Default Partition"
« Reply #15 on: August 22, 2010, 02:48:55 AM »
@zef:

I found quite a bit of problems that I intend to fix soon. I've started working on them yesterday after I found out renaming a partition whose name is a substring of the alias doesn't work. The only way to fix that is to actually parse the string instead of (lazily) looking up substrings and hoping it works.

Edit: I think everything is working as it should now. I'll commit to my branch after I do some cleanup.
Edit2: Just clarifying, the problems were not your fault. They're now fixed in my branch anyway: all the basic tests I could think of worked, but I obviously need confirmation from other people as well.
« Last Edit: August 23, 2010, 08:14:29 AM by danielkza »

zef

  • Administrator
  • Posts: 265
Re: Support for volume UUID and/or device tree lookup in "Default Partition"
« Reply #16 on: August 26, 2010, 11:05:13 AM »
Thx! I will merge back your changes as soon as I can! :)
ASUS P8Z68-V PRO/GEN3 | i5-2500k | 16GB RAM | GTX560 | Keyboard | Mouse | Devilsound DAC

Azimutz

  • VoodooLabs
  • Posts: 420
  • Paranoid Android
Re: Support for volume UUID and/or device tree lookup in "Default Partition"
« Reply #17 on: August 28, 2010, 02:21:07 AM »
Hi Danielkza, Zef,

Zane reported this stuff here, on the pic; i managed to isolate it to when the Default volume (showed on Timeout) is renamed. It happens just on Timeout screen and used theme seems irrelevant (used Legacy & Embed).
It was definitely introduced with rev 432.
Waiting on Zane's feedback...

Also "Rename Partition" related, the alias needs to be wrapped in "" if it contains spaces, else
Quote
OS X Install
for instance, will only show
Quote
OS

Feel reported :)
See ya later...
« Last Edit: August 28, 2010, 02:37:21 AM by Azimutz »
 System & Patches: http://goo.gl/i961
 Chameleon:
- trunk builds: http://goo.gl/9G1Hq
- pref pane: http://goo.gl/OL2UT

danielkza

  • VoodooLabs
  • Posts: 9
Re: Support for volume UUID and/or device tree lookup in "Default Partition"
« Reply #18 on: August 28, 2010, 03:03:26 PM »
@Azimutz:
I found out the second bug myself just yesterday, and luckily it's an easy fix. I'll have to investigate the first one further: maybe I'm overwriting memory somehow?!

Thanks for the feedback.

zef

  • Administrator
  • Posts: 265
Re: Support for volume UUID and/or device tree lookup in "Default Partition"
« Reply #19 on: August 28, 2010, 05:55:44 PM »
@Danielkza:

Please review the usage of sizeof() calls as well:

For example:

Code: [Select]
char dummy[80];
getBootVolumeDescription( gBootVolume, dummy, sizeof(dummy) - 1, true );

sizeof(dummy) is 4 since dummy is a pointer.
ASUS P8Z68-V PRO/GEN3 | i5-2500k | 16GB RAM | GTX560 | Keyboard | Mouse | Devilsound DAC

zef

  • Administrator
  • Posts: 265
Re: Support for volume UUID and/or device tree lookup in "Default Partition"
« Reply #20 on: August 28, 2010, 07:32:58 PM »
Hi Danielkza, Zef,

Zane reported this stuff here, on the pic; i managed to isolate it to when the Default volume (showed on Timeout) is renamed. It happens just on Timeout screen and used theme seems irrelevant (used Legacy & Embed).

Roger that, I'm working on it! :)

EDIT: Try this one: http://forge.voodooprojects.org/p/chameleon/source/commit/438/

Also "Rename Partition" related, the alias needs to be wrapped in "" if it contains spaces, else
Quote
OS X Install
for instance, will only show
Quote
OS

I don't see any issue here since space is a delimiter, you need to enclose in quotes in other places as well.
« Last Edit: August 28, 2010, 08:10:03 PM by zef »
ASUS P8Z68-V PRO/GEN3 | i5-2500k | 16GB RAM | GTX560 | Keyboard | Mouse | Devilsound DAC

danielkza

  • VoodooLabs
  • Posts: 9
Re: Support for volume UUID and/or device tree lookup in "Default Partition"
« Reply #21 on: August 28, 2010, 09:23:21 PM »
@Danielkza:

Please review the usage of sizeof() calls as well:

For example:

Code: [Select]
char dummy[80];
getBootVolumeDescription( gBootVolume, dummy, sizeof(dummy) - 1, true );

sizeof(dummy) is 4 since dummy is a pointer.
dummy is an array of characters, and sizeof works as it should.

http://en.wikipedia.org/wiki/Sizeof#Using_sizeof_with_arrays

I can understand if you want to avoid it, but you need to use (size of the array)-1 for it to be correct, unlike what's in the modifications you made before commiting:

Code: ('cpp') [Select]
char dummy[80];
// mine (right)
getBootVolumeDescription( gBootVolume, dummy, sizeof(dummy) - 1, true );
// trunk (wrong)
getBootVolumeDescription( gBootVolume, dummy, 80, true );

Edit: Uh oh, after browsing a bit through the source, I found quite many instances of the similar mistakes. Luckily Chameleon runs on its own: we'd have tons of buffer overflows otherwise. An example:

dumpPhysAddr @ i386/libsaio/mem.c

Code: [Select]
// ...
        strncat(buffer, str, sizeof(buffer));
    }
    strncat(buffer,"  ", sizeof(buffer));
    for (j=0; j < (len%STEP); j++)  {
        sprintf(str, "%c", DC(ad[i+j])); 
        strncat(buffer, str, sizeof(buffer));
// ...

Quote from: 'The Open Group'
char *strncat(char *restrict s1, const char *restrict s2, size_t n);

The strncat() function shall append not more than n bytes (a null byte and bytes that follow it are not appended) from the array pointed to by s2 to the end of the string pointed to by s1. The initial byte of s2 overwrites the null byte at the end of s1. A terminating null byte is always appended to the result. If copying takes place between objects that overlap, the behavior is undefined.

If n chars from s2 are copied plus the terminating null, s1 should be at least n+1 chars in size. Or, if you prefer, n should be sizeof(buffer)-1.


« Last Edit: August 28, 2010, 10:14:09 PM by danielkza »

zef

  • Administrator
  • Posts: 265
Re: Support for volume UUID and/or device tree lookup in "Default Partition"
« Reply #22 on: August 28, 2010, 11:29:34 PM »
If n chars from s2 are copied plus the terminating null, s1 should be at least n+1 chars in size. Or, if you prefer, n should be sizeof(buffer)-1.

Sorry, you're totally right! :)
ASUS P8Z68-V PRO/GEN3 | i5-2500k | 16GB RAM | GTX560 | Keyboard | Mouse | Devilsound DAC

Azimutz

  • VoodooLabs
  • Posts: 420
  • Paranoid Android
Re: Support for volume UUID and/or device tree lookup in "Default Partition"
« Reply #23 on: August 29, 2010, 03:08:34 AM »
Zef, Danielkza,

confirming the fix on rev 438

The other issue... well, it's more like issues :) as i already had my hands on it, took some time to check the trio
Default/Rename/Hide Partition and some other...

Default Partition:
At least BootHelp.txt (BH.txt) needs edit; "label" (enclosed in quotes) doesn't work , even if the label has spaces!
No quotes works, even if the label has spaces.

Rename Partition:
the stuff i mentioned on the last post; this
Code: [Select]
<key>Rename Partition</key>
<string>"Mac OS X Install DVD" OS X Install</string>
always worked fine till rev 432 (without quotes) and that's how i thought it worked based on previous BH.txt info.
Judging by Danielkza answer and the actual BH.txt, i assume that's how he meant it to work!?

Hide Partition:
this
Code: [Select]
<key>Hide Partition</key>
<string>Windows 7 Mac OS X Install DVD</string>
works perfectly! (yep, i removed "& kBVFlagForeignBoot" to test)

So, it's mainly a BootHelp thing (or not); when compared with the comments on the code, things get a bit confuse.
On a slightly diff note... none of the keys with spaces on the name (on BH.txt), work quoted.
Didn't tested Rescan Prompt and Scan Single Drive, but i guess...

That's it.. Thanks.
 System & Patches: http://goo.gl/i961
 Chameleon:
- trunk builds: http://goo.gl/9G1Hq
- pref pane: http://goo.gl/OL2UT

danielkza

  • VoodooLabs
  • Posts: 9
Re: Support for volume UUID and/or device tree lookup in "Default Partition"
« Reply #24 on: August 29, 2010, 03:49:08 AM »
@Azimuts:

Default Partition: it worked for me, but maybe I missed something. I'll take a look at it, together with all the other stuff, now that I have some spare time.

Rename Partition: I intend a quoted name plus an unquoted alias to work, but now that I think about it, I'm not sure if it's actually a good idea. We'll have to choose beetween consistency (every partition with spaces must be quoted) or ease of use (quotes aren't strictly needed: a semicolon or the end of the string will delimit the alias just fine).

BootHelp.txt indeed needs some work. But this should be postponed until the final behavior is decided.

Hide Partition: What you mentioned was actually an accidental match from the 'lazy' old code. It looped through each partition checking if the label was present in the 'Default Partition' key. Something like 'Windows 7LOLWUT' would (wrongfully) match a partition labeled 'Windows 7'.

Azimutz

  • VoodooLabs
  • Posts: 420
  • Paranoid Android
Re: Support for volume UUID and/or device tree lookup in "Default Partition"
« Reply #25 on: August 29, 2010, 05:37:24 AM »
Danielkza,

this is the real issue i was pointing:
Quote
We'll have to choose beetween consistency (every partition with spaces   must be quoted) or ease of use (quotes aren't strictly needed: a   semicolon or the end of the string will delimit the alias just fine).
on the user side, my opinion is the simple the better! But ultimately, quotes or not, consistency and efficiency win.
On the code side, i guess ease of maintenance wins.

Let's see what Zef has to say...
 System & Patches: http://goo.gl/i961
 Chameleon:
- trunk builds: http://goo.gl/9G1Hq
- pref pane: http://goo.gl/OL2UT

danielkza

  • VoodooLabs
  • Posts: 9
Re: Support for volume UUID and/or device tree lookup in "Default Partition"
« Reply #26 on: August 29, 2010, 07:33:34 AM »
I just though of something that could be useful, and has the indirect consequence of solving the matter by needing all labels with spaces to be quoted.

Would specifying multiple default partitions (the first one, left-to-right, that matches anything makes that partition default) be of use to someone besides me? I'd use it to boot from my USB drive automatically if it's inserted.

zef

  • Administrator
  • Posts: 265
Re: Support for volume UUID and/or device tree lookup in "Default Partition"
« Reply #27 on: August 29, 2010, 08:40:53 AM »
I just though of something that could be useful, and has the indirect consequence of solving the matter by needing all labels with spaces to be quoted.

Would specifying multiple default partitions (the first one, left-to-right, that matches anything makes that partition default) be of use to someone besides me? I'd use it to boot from my USB drive automatically if it's inserted.

It would be great!

And I would vote for using quotes with labels containing spaces in all cases.

Thanks for your efforts! :)
ASUS P8Z68-V PRO/GEN3 | i5-2500k | 16GB RAM | GTX560 | Keyboard | Mouse | Devilsound DAC

Azimutz

  • VoodooLabs
  • Posts: 420
  • Paranoid Android
Re: Support for volume UUID and/or device tree lookup in "Default Partition"
« Reply #28 on: August 29, 2010, 01:54:13 PM »
Yeah, that could be handy... never crossed my mind :) good inspiration, Danielkza.

Hey, i'm ok with quotes on stuff with spaces. Just make it a standart.
 System & Patches: http://goo.gl/i961
 Chameleon:
- trunk builds: http://goo.gl/9G1Hq
- pref pane: http://goo.gl/OL2UT

Azimutz

  • VoodooLabs
  • Posts: 420
  • Paranoid Android
Re: Support for volume UUID and/or device tree lookup in "Default Partition"
« Reply #29 on: September 10, 2010, 07:58:24 PM »
I just though of something that could be useful, and has the indirect consequence of solving the matter by needing all labels with spaces to be quoted.

Would specifying multiple default partitions (the first one, left-to-right, that matches anything makes that partition default) be of use to someone besides me? I'd use it to boot from my USB drive automatically if it's inserted.
Hi Danielkza,

what's the status on the quoted stuff? mainly the quotes + spaces stuff...
No pressure :)
Thanks.
 System & Patches: http://goo.gl/i961
 Chameleon:
- trunk builds: http://goo.gl/9G1Hq
- pref pane: http://goo.gl/OL2UT