[Qemu-ppc] Revert "Update OpenBIOS images"

On 14.02.2013, at 01:04, Alexander Graf wrote:

> 
> On 14.02.2013, at 01:01, Mark Cave-Ayland wrote:
> 
>> On 13/02/13 23:45, Alexander Graf wrote:
>> 
>>>> The release is basically done. I don't think we have time for any fix beyond reverting the commit. And I'd rather have it reverted, since we regress heavily against 1.3 with the updated OpenBIOS.
>>> 
>>> [12:43am]aliguori:agraf, i can wait until the very start (7am my time) of tomorrow morning to tag -rc2 if you think having a little more time would be helpful here
>>> [12:43am]agraf:aliguori: but to me a regression weighs more than missing bug fixes
>>> [12:43am]agraf:aliguori: if mark can debug this down within that time, would that work for you?
>>> [12:43am]aliguori:agraf, yes
>>> 
>>> Mark, do you think you could narrow this down within the next few hours? Or rather - would you like to give it a try?
>> 
>> Hmmmm if I had to guess which patch may stop quik from booting then I'd go with this one: http://git.qemu.org/?p=openbios.git;a=commit;h=3caf41bf4a0f9ef7c8b294aca69fbe3366aec21b.
> 
> Nope, that's not the one. Reverting it doesn't help.

65bbf2e226266d8f7de0e23b584e184bac5fd273 is first bad commit
commit 65bbf2e226266d8f7de0e23b584e184bac5fd273
Author: mcayland <mcayland@f158a5a8-5612-0410-a976-696ce0be7e32>
Date:   Sat Nov 24 14:43:09 2012 +0000

    Fix dir cd:,\ (no partition specified) when reading from Mac partitions.
    
    The existing checks in mac-parts,c were wrong; regardless of whether or not we
    have an argument string specified, if a partition is not specified then we
    must still search for the first valid partition.
    
    Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
    
    git-svn-id: svn://openfirmware.info/openbios/trunk/openbios-devel@1072 f158a5a8-5612-0410-a976-696ce0be7e32

:040000 040000 18be4df16bc546c313051b01cd0587b2a6faacdf f4815007b57d11018edc2293db3ccd1e01adf6cb M	packages


With the following patch fixing the issue at hand for me. Though I don't fully understand why str would be NULL yet:



Alex

On 14/02/13 00:17, Alexander Graf wrote:

> With the following patch fixing the issue at hand for me. Though I don't fully understand why str would be NULL yet:
>
>
> diff --git a/packages/mac-parts.c b/packages/mac-parts.c
> index a286870..443455e 100644
> --- a/packages/mac-parts.c
> +++ b/packages/mac-parts.c
> @@ -140,7 +140,7 @@ macparts_open( macparts_info_t *di )
>           * Implement partition selection as per the PowerPC Microprocessor CHRP bindings
>           */
>
> -       if (parnum == 0) {
> +       if (str == NULL || parnum == 0) {
>                  /* According to the spec, partition 0 as well as no arguments means the whole disk */
>                  offs = (long long)0;
>                  size = (long long)__be32_to_cpu(dmap.sbBlkCount) * bs;
>
> Alex

Ah okay. It's actually caused by this bit of logic in 
libopenbios/bindings.c and assuming that my_args() is a zero length 
Forth string:

char *
pop_fstr_copy( void )
{
         int len = POP();
         char *str, *p = (char*)cell2pointer(POP());
         if( !len )
                 return NULL;
         str = malloc( len + 1 );
         if( !str )
                 return NULL;
         memcpy( str, p, len );
         str[len] = 0;
         return str;
}

The check for a zero length string and returning NULL has caused me 
problems before when round-tripping strings between Forth and C.

Without testing the patch myself, I'd say that it looks good. I can run 
it over my complete set of test images tomorrow evening if that would be 
acceptable? Can you post a git diff version to the OpenBIOS mailing list 
too?


ATB,

Mark.

On 14.02.2013, at 01:54, Mark Cave-Ayland wrote:

> On 14/02/13 00:17, Alexander Graf wrote:
> 
>> With the following patch fixing the issue at hand for me. Though I don't fully understand why str would be NULL yet:
>> 
>> 
>> diff --git a/packages/mac-parts.c b/packages/mac-parts.c
>> index a286870..443455e 100644
>> --- a/packages/mac-parts.c
>> +++ b/packages/mac-parts.c
>> @@ -140,7 +140,7 @@ macparts_open( macparts_info_t *di )
>>          * Implement partition selection as per the PowerPC Microprocessor CHRP bindings
>>          */
>> 
>> -       if (parnum == 0) {
>> +       if (str == NULL || parnum == 0) {
>>                 /* According to the spec, partition 0 as well as no arguments means the whole disk */
>>                 offs = (long long)0;
>>                 size = (long long)__be32_to_cpu(dmap.sbBlkCount) * bs;
>> 
>> Alex
> 
> Ah okay. It's actually caused by this bit of logic in libopenbios/bindings.c and assuming that my_args() is a zero length Forth string:
> 
> char *
> pop_fstr_copy( void )
> {
>        int len = POP();
>        char *str, *p = (char*)cell2pointer(POP());
>        if( !len )
>                return NULL;
>        str = malloc( len + 1 );
>        if( !str )
>                return NULL;
>        memcpy( str, p, len );
>        str[len] = 0;
>        return str;
> }
> 
> The check for a zero length string and returning NULL has caused me problems before when round-tripping strings between Forth and C.
> 
> Without testing the patch myself, I'd say that it looks good. I can run it over my complete set of test images tomorrow evening if that would be acceptable?

Either we get the fix in in the next few hours (and thus agree that it's the right way forward) or there's no rush - QEMU 1.5 is quite a while to go.

> Can you post a git diff version to the OpenBIOS mailing list too?

Sure!


Alex