16 bit WHEFS_ID_TYPE_BITS breaks whio_dev_encode_cstring return type

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Change WHEFS_ID_TYPE_BITS from 32 to 16
2. Compile
3. See errors

What is the expected output? What do you see instead?

no compile errors. :)

What version of the product are you using? On what operating system?

libwhefs-20111012

Please provide any additional information below.

change

include/wh/whio/whio_encode.h:uint32_t whio_dev_encode_cstring( whio_dev * dev, 
char const * s, uint32_t n );

from uint32_t to whio_size_t

Likewise

src/whefs_fs.c: In function 'whefs_openfs_stage2':
src/whefs_fs.c:1495: warning: passing argument 2 of 'whio_dev_decode_uint32' 
from incompatible pointer type
src/whio_encode.c:437: note: expected 'uint32_t *' but argument is of type 
'whio_size_t *'

probably needs to be fixed as well

Original issue reported on code.google.com by dan.schm...@gmail.com on 2 Nov 2011 at 8:47

[GoogleCodeExporter]

Doh, thank you very much. i believe that problem was fixed in the 2nd 
generation of the whio API, but i'll get it fixed in this one tonight.

Original comment by sgbeal@googlemail.com on 2 Nov 2011 at 8:55

• Changed state: Accepted
• Added labels: Priority-High
• Removed labels: Priority-Medium

[GoogleCodeExporter]

LOL! The comment above the offending line reveals something:

    // FIXME: add whio_dev_size_t_en/decode()
    rc = whio_dev_decode_uint32( fs->dev, &opt->block_size );

This change has broken other stuff, so i cannot commit the code right now. i 
will take a closer look at it tomorrow or Friday (when i have access to my real 
dev PC instead of this netbook).

Your report revealed an option-parsing bug in the whefs applications, by the 
way (that fix was just committed in r359).

Original comment by sgbeal@googlemail.com on 2 Nov 2011 at 9:15

[GoogleCodeExporter]

Reminder to self: whio_dev_encode_cstr() is only used in test code - it can be 
removed entirely. But the encoding/decoding of opt->block_size certainly needs 
to be fixed to use whio_size_t.

Original comment by sgbeal@googlemail.com on 2 Nov 2011 at 9:17

[GoogleCodeExporter]

I'm hesitant to give up the 16 bit type size (we make lots of small files so 
the 32 gets a little expensive.)

Original comment by dan.schm...@gmail.com on 2 Nov 2011 at 9:19

[GoogleCodeExporter]

It can be fixed easily enough, i just need to figure out which 
algos/preconditions i've broken by changing the size of the type. On Friday 
i'll have my dev laptop with me and can run it through the debugger and whatnot 
to find what i've missed. Until then i'm on my netbook, which is really tedious 
to develop on.

A bit of background: each of the on-storage entities (e.g. inode/block metadata 
and whatnot) is encoded as a fixed-sized number, and changing from uint32 to 
whio_size_t changes those sizes in 16-bit mode. Somewhere (not yet sure where) 
i have not accounted for this change (it's still counting/expecting 4 bytes but 
only has 2), which causes a down-stream assertion in some of my test code 
(which is a good thing, actually, or i might have missed this problem).

Original comment by sgbeal@googlemail.com on 2 Nov 2011 at 9:24

[GoogleCodeExporter]

DOH! Reminder to self: the report is about the WHEFS_ID_TYPE_BITS, NOT 
WHIO_SIZE_T_BITS. i was changing the wrong stuff :/.

PS: in whefs' successor, epfs, 16-bit is the default.

Original comment by sgbeal@googlemail.com on 2 Nov 2011 at 9:26

[GoogleCodeExporter]

The encode/decode_uint32 calls for the block_size are now encode/decode_size_t, 
as of r360. i will likely remove whio_dev_encode/decode_cstr() altogether - 
they are only used in one test function. That part will wait until tomorrow, 
though.

This "should" be file-format compatible when in 32-bit mode, but is 
incompatible with containers created in 16-bit mode before this change (reading 
the EFS header will fail when opening).

Was WHEFS_ID_TYPE_BITS really 32-bits in that download? It was 16 bits in my 
local copy (svn trunk), and if it was 32-bits in the download, that was an 
accident (or artifact of test runs). WHIO_SIZE_T_BITS, on the other hand, is 
fairly useless when <32 except for very specialized cases.

Original comment by sgbeal@googlemail.com on 2 Nov 2011 at 10:00

• Added labels: Priority-Medium
• Removed labels: Priority-High

[GoogleCodeExporter]

Hi, Dan!

A long time ago i remember C89 vs C99 being a point of interest for you. As of 
r364 the lib-level code (not app/test-code) compiles in C89 mode with the 
caveat that we still need a couple C99 headers for bool and fixed-size 
integers. (Still can't believe C89 didn't have a bool type.)

Original comment by sgbeal@googlemail.com on 3 Nov 2011 at 3:02

[GoogleCodeExporter]

I fail, my change that showed the errors was:

< #define WHIO_SIZE_T_BITS 32
---
> #define WHIO_SIZE_T_BITS 16

That'll teach me to search for 16 and think I found what I wanted.
The WHEFS_ID_TYPE_BITS was 16 in the download.

I think there have been some other issues introduced (because just changing the 
uint32 to whefs_size_t and ignoring the dev_decode warning fails our porting 
layer tests when attempting to create a file on the image.)

Original comment by dan.schm...@gmail.com on 3 Nov 2011 at 3:13

[GoogleCodeExporter]

And i failed because you wrote WHEFS_ID_TYPE_BITS and i tried WHIO_SIZE_T_BITS 
instead ;).

i would be somewhat surprised if WHIO_SIZE_T_BITS=16 actually works out of the 
box on whefs, largely due to one or two outstanding uses of uint32_t which 
should be whio_size_t. In any case, such a setup would be drastically limited 
(e.g. max EFS size of 64kb). i know it works in EPFS, so in principal it can be 
made to work for whefs as well, perhaps requiring a minor file format 
incompatibility.

Just to clarify: which did you _want_ to change in the first place, ID_T_BITS 
or SIZE_T_BITS?

Original comment by sgbeal@googlemail.com on 3 Nov 2011 at 3:31

[GoogleCodeExporter]

Ok, I went back to my old 20100126 distribution to see what I changed.  It was 
really WHEFS_ID_TYPE_BITS to 16 from the default 32.  The new default looks to 
be 16 and that's what confused me because I thought I had to change something.  
I'm not sure there is a bug that is affecting me now (rebuild and test in 
progress.)

Original comment by dan.schm...@gmail.com on 3 Nov 2011 at 4:35

[GoogleCodeExporter]

The current release passes our porting layer regression tests.  If this is an 
issue, it's not critical to me.  Sorry if I got you worried.

Original comment by dan.schm...@gmail.com on 3 Nov 2011 at 4:58

[GoogleCodeExporter]

Great :-D. i _hope_ i didn't invalidate your old EFS files with this fix, but i 
suspect that i did. i was using the wrong encode/decode routine for the 
block_count parameter, and i'm absolutely appalled that gcc didn't tell me that 
sooner (i always run it with -Wall -Werror -pedantic).

Do you use whio_dev_encode/decode_cstr()? If not, i'll go ahead and remove it. 
i think it was planned for use at some point but it seems it is not currently 
used.

Original comment by sgbeal@googlemail.com on 3 Nov 2011 at 5:11

[GoogleCodeExporter]

whio_dev_encode/decode_cstr can go away.

The whefs is created for by the application and used as a transitory store 
before data uploads, and a new install means new fs, so I'm not affected by 
version change invalidations, so no worries there either.

Original comment by dan.schm...@gmail.com on 3 Nov 2011 at 5:35

[GoogleCodeExporter]

i started to remove it but figured i might break _someone's_ code if i do, so 
i've left it in. i'll go ahead and close this ticket.

Thank you once again for taking the time to report problems. It is mainly 
through such reports that improvements are made.

Original comment by sgbeal@googlemail.com on 3 Nov 2011 at 5:55

• Changed state: Verified

[GoogleCodeExporter]

A final clarification, after getting my head straight...

there is NOT a file-format incompatibility for various WHEFS_ID_TYPE_BITS here, 
but there is one for clients who previously built with WHIO_SIZE_T_BITS != 32 
(32 is the default, so i don't expect that anyone actually did that). The 
problem is that the block_size EFS property was explicitly being encoded as a 
32-bit int, as opposed to a whio_size_t (which has a fixed size of 
(WHIO_SIZE_T_BITS/8)). For 32-bit WHIO_SIZE_T_BITS this is identical, but for 
other values it is not.

Original comment by sgbeal@googlemail.com on 3 Nov 2011 at 6:12