Support for volume UUID and/or device tree lookup in "Default Partition"

[danielkza]

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

[zef]

Thx! I will merge back your changes as soon as I can!

[Azimutz]

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

OS X Install

for instance, will only show

OS

Feel reported
See ya later...

[danielkza]

@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]

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

[zef]

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

OS X Install

for instance, will only show

OS

I don't see any issue here since space is a delimiter, you need to enclose in quotes in other places as well.

[danielkza]

@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));
// ...


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.

[zef]

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!

[Azimutz]

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.

[danielkza]

@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]

Danielkza,

this is the real issue i was pointing:

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

[danielkza]

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]

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!

[Azimutz]

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.

[Azimutz]

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.