Hello, Vojtech,
On Thu, May 24, 2012 at 11:24:37AM +0200, Vojtech Horky wrote:
> 2012/5/24 Sean Bartell <[email protected]>:
> > currently supported; in the next few days, I will add support for files
> > and task memory and improve style and documentation. Although the
> Looks good. By "task memory" you mean memory of other task or just
> malloc()ed memory of the same task?
I was thinking of other tasks' memory, but you reminded me I have to
implement a memory_blob too.
> I have a few comments (mostly details).
>
> First of all, I am not able to compile the code for 32bit architecture
> ...
> blob.c:71:2: note: expected 'aoff64_t *' but argument is of type 'size_t *'
>
> When writing functions, the opening brace shall be on next line and
> when the parameters do not fit on a single line, they are indented
> with 4 spaces instead of two tabs.
>
> In the bithenge/Makefile, feel free to change the author to yourself ;-).
>
> In block.h, I would recommend to use #include <loc.h> instead of
> "loc.h" to emphasize the fact that you are including a library header
> instead of a local one.
> If you are calling malloc() or realloc() and they fail, return ENOMEM.
> Our implementation of malloc() does not set errno.
All fixed.
> In the bithenge_blob_*() in blob.h I would put explicit assert()
> before accessing blob->ops->* to ensure none of the members are NULL.
> That would make it easier to spot which member was actually NULL if
> the task collapses on "Page fault at address 0". ...
Good idea. I made bithenge_new_* check everything, including
blob->ops->*, so bithenge_blob_* only needs to check blob and blob->ops.
> I like the way you are hiding the details when working with block
> device but I think it would be better to hide more the direct
> typecasting:
> block_blob_t *blob = (block_blob_t *)base;
> with something like
> block_blob_t *blob = BLOCK_FROM_BLOB(base);
> to ensure smoother change if (for whatever reason) you would have to
> move "base" from the beginning of the structure.
I'm not sure why I would need to do that, but okay. I used static inline
functions instead of macros for cleanliness.
> Otherwise, the code as a whole looks nice and clean to me. And I
> really appreciate that you comment all the public functions.
>
> >
> > I have also decided on the data structure the library will represent.
> > Primitive values will be binary blobs, integers, or Unicode strings.
> I am not sure that is enough. At least, you have to have a way to
> distinguish integers of various sizes, signness and endianess.
The tree only contains the final result of decoding the binary data. The
converter (which is the part I'm still thinking about) may know that
some data is a 4-byte little-endian unsigned integer, but after it's
decoded it's represented in the tree just like any other integer.
> > The data structure will be a tree; each edge will be labelled with a
> > primitive value, and each leaf node will store a single primitive value.
> And inner node would be...?
It only has edges to other nodes. An example tree for a FAT filesystem
could look like this, where 'O' represents an internal node and
"fat", "0", "0xfff0", ... are primitive values:
O
|
+---fat-->O
| +---0--->0xfff0
| +---1--->0xffff
| +---2--->0x0000
|
+---root-->O
+---0--->O
| +---name--->README.TXT
| +---size--->0x1351
|
+---1--->O
+---name--->KERNEL.ELF
+---size--->0x38e9a2
> > Each node is responsible for allocating and freeing its children. It may
> > be necessary later to add reference counting or use a DAG instead of a
> > tree, but I will start with this simple model.
> >
> > I have some ideas for the design of format specifications, but I still
> > need to spend more time thinking them through. I will write a minimal
> > demo program in Python to determine whether they are viable.
> I hope you will share it with us :-).
Thanks,
Sean Bartell
_______________________________________________
HelenOS-devel mailing list
[email protected]
http://lists.modry.cz/cgi-bin/listinfo/helenos-devel
