Hello Sean, thanks for the report. 2012/5/24 Sean Bartell <wingedtachik...@gmail.com>: > Hello, everyone, > > I've made the first commit to lp:~wtachi/helenos/bithenge with partial > support for binary blob data sources. It uses an object-oriented design > to make adding new data sources relatively easy. Only block devices are > 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?
> documentation and style are incomplete, I would like feedback on the > parts I've written. I have a few comments (mostly details). First of all, I am not able to compile the code for 32bit architecture (make PROFILE=ia32 HANDS_OFF=y): blob.c: In function 'sequential_buffer': blob.c:71:2: error: passing argument 3 of 'blob->ops->read' from incompatible pointer type [-Werror] 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. 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". E.g. something like: assert(blob); assert(blob->ops); assert(blob->ops->read); return blob->ops->read(...) 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. If you are calling malloc() or realloc() and they fail, return ENOMEM. Our implementation of malloc() does not set errno. 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. And I would also like to see simple ASCII char (for example, GIF header starts with version that could be directly displayed as an ASCII string, 6 characters long). > 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...? > 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 :-). Bye and thanks again. - Vojta > > Thanks, > Sean Bartell > > _______________________________________________ > HelenOS-devel mailing list > HelenOS-devel@lists.modry.cz > http://lists.modry.cz/cgi-bin/listinfo/helenos-devel _______________________________________________ HelenOS-devel mailing list HelenOS-devel@lists.modry.cz http://lists.modry.cz/cgi-bin/listinfo/helenos-devel
