usr/src/common/ficl/loader.c (Diff revision 1)

Show all issues

uuids is not the clearest variable name. Can we make it clearer?

usr/src/common/ficl/loader.c (Diff revision 1)

Show all issues

It seems like uuids is trying to tell us how many bytes to read from uuidp. What are the expectations if uuidp is less than uuids. In the more case, we're covered and that makes semantic sense. What's the API supposed to be here?

usr/src/common/ficl/loader.c (Diff revision 1)

Show all issues

There are 2 types of strings used in forth, counted strings, which are represented as pascal string - first byte is the length and the rest is the string itself, and the limit is also obvious. Second variant is address and length, pointer and int pair, in comments its usually denoted as ( addr n ), stating also the stack layout, with rightmost side for stack top.
So this implementation is expecting string ( addr n ) from stack and will put address of binary representation of uuid into the stack, so the full specification is ( addr n -- addr' ).
As an general note, there is even no way to distinguish the data, as all words are 32 or 64 bit fields (depending on machine word), so while we certainly can make some inspection for the data (here we know the string length should be 36), in practice it does not matter too much because if there is wrong stack layout, all bets are off anyhow:)

1. 

   Robert Mustacchi Nov. 8, 2016, 7:35 p.m.

   What I'm not sure of is whether it's expecting to say that I have a string which is n bytes long or I have a string that is *up to* n bytes long. From the code, it sounds like the latter, is that right?

2. 

   Toomas Soome Nov. 8, 2016, 7:53 p.m.

   Actually the first, its string, n bytes long, as it is not "C" string with terminating '\0'. Which is why it needs to allocate local instance to create zero terminated string for uuid api. In this context, the \0 has no special meaning; essentially this notation is just telling us that we have memory area of n bytes (with whatever content).

3. 

   Robert Mustacchi Nov. 11, 2016, 4:42 p.m.

   If that's the case, should we really be using strncpy? Shouldn't we just use bcopy to ensure that we get everything correct and if for some reason there's an embedded null, we don't lose part of the data?

4. 

   Toomas Soome Nov. 11, 2016, 4:53 p.m.

   This is good point, will do.

Ship It!

Ship It!

Ship It!