Menu ▾ ▴

Possible os9.exe & toolshed.c errors

Open Discussion
Robert Gault
2013-07-23
2016-03-18

  • Robert Gault

    Robert Gault - 2013-07-23

    There are some lines in toolshed.c and various os9 source modules that seem suspect.
    toolshed.c starting at line 820
    /* walk bitmap for 'bytes_in_bitmap * 8' times */
    There is a test for the largest free block which never resets the count when a cluster is not free. I think a new line is needed.

                /* bit is set, sector not free */
            if (*largest_count > *largest_free_block)
            {
                *largest_free_block = *largest_count;
    /* I think the largest count should be reset as soon as a non-free cluster is found. */
                *largest_count = 0;

    os9format.c about line 464

    /* There appears to be an error here. Since bitmapSectors is already the correct number
           there is no reason to do anything except multiply it by sectorSize.
        size = NextHighestMultiple((bitmapSectors) * sectorSize, clusterSize); */
        size = (bitmapSectors) * sectorSize;

    I don't see why (bitmapSectors) * sectorSize is not the value needed. Cluster size should be irrelevant.

    os9gen.c about line 216
    There is no provision for allocating the correct bits in the map if the cluster size is not 1. I think a change is needed.

            /* Old line that ignored DD.BIT
        _os9_allbit(opath->bitmap, startlsn, (size+255)/256); */
        /* New line to take into account DD.BIT. */
        _os9_allbit(opath->bitmap, (startlsn+cluster_size-1)/cluster_size, (((size+255)/256)+cluster_size-1)/cluster_size);

    You also need to define a new variable for the routine at L132

        /* New variable needed if DD.BIT not 1 */
    int cluster_size;
     

    Last edit: Tormod Volden 2016-03-15

  • Tormod Volden

    Tormod Volden - 2016-03-16

    You are actually reporting 3 different issues here.

    1. libtoolshed/toolshed.c: Resetting largest_count
      I have committed your suggested fix, thanks. I believe it was wrongly returning the total number of free sectors instead of the largest free block.

    2. os9format.c: Calculation of bit map size (in bytes)
      I think you are right. The confusion is maybe over whether to pad the the bitmap so that it covers a whole number of clusters?

    3. os9gen.c: Allocating in bit map if cluster size is not 1
      Yes, obviously the cluster size not 1 possibility was added to the code as an afterthought, and it went missing here.

    While studying this I found a few more errors:

    .4. os9free reports free clusters and not free sectors
    TSRBFFree() should return the number of free sectors, and not of free clusters, I believe?
    The code and variables surrounding (1) above refers to sectors when it means clusters, and should be fixed up to make the code more understandable. And to calculate the number of free sectors from the number of free clusters.

    .5. I found what I believe is a typo in os9format. Please review my committed change.

    I feel like reformatting and rewriting stuff in os9format.c but let's fix the bugs first.

    References: OS-9 manual chapter 7

     

  • Tormod Volden

    Tormod Volden - 2016-03-16

    This patch addresses (4) above. However TSRBFFree() is not documented so I am not sure what it is expected to return through its parameters. Although it seems none of its callers are using e.g. largest_count and sector_count. Maybe they should be removed.

     
    20-toolshed-TSRBFFree-clustersize.patch

  • Tormod Volden

    Tormod Volden - 2016-03-18

    I have pushed your fix in (3) above, plus added support for different sector sizes to os9gen.c. I also applied the above patch for os9free.c (4) with some changes. And I made a follow-up for (5) since fixing the typo was not everything that was needed.

     

Anonymous
Anonymous

Add attachments
Cancel