Hi, On Thu, Sep 20, 2007 at 07:08:31PM -0400, Ben Asselstine wrote:
> I would appreciate any feedback at all. I finally got around to reading through it. Nice work -- I like the examples you added :-) Here are a couple of nitpicks (about everything that is in the new version; don't remember which parts are yours, and which were already there in the old version): - "The services of a network stack can be accessed through the node in the filesystem where the network stack was attached." -> Might be helpful to mention the standard location (/servers/socket/2) - I find the section on Mach ports really hard to grasp -- even though I went through the IPC section in the gnumach manual once. I guess it's just do dense; maybe it should be made more verbose -- or maybe even better only explain the concepts superficially, and refer to the manual for all the details about types etc... - Although it is not really related to MiG, it might be useful to explain what the variables do in the example given in the MiG section. Also it would be useful for understanding to show/explain what MiG translates the definition to. - "When writing translators it usually makes more sense to use the hurdish routines rather than their Glibc equivalents." -> Is it really better to use low-level I/O interfaces in translators? If so, why? My feeling is that it should be perfectly fine to use the standard libc facilities when talking to other things as a client, even from a translator. (Obviously you need special interfaces for stuff that the translator is serving; but that's a different story...) - When referring to files in the source tree (like $(HURD)/trans/hello.c), it would be nice to link to the files in webcvs - It seems important to explain what the mmap() does, and why we are using mmap() here. In fact there should be a bit on VM in the concepts chapter -- most people are probably not very familar with this stuff; and understanding how the VM works in the context of Mach and Hurd is crucial for a Hurd hacker. - It would be nice to give some list with explanations what error codes translators are supposed to return in certain situations, or link to one if it exists elsewhere. - "To prevent a port leak, the bootstrap port must be deallocated." This requires some explanation. Why would that be a port leak? The port is not automatically cleaned on exit, or what? - It would be nice to explain the parameters of trivfs_startup() and ports_manage_port_operatoins_one_thread() - Memfile is a nice idea. Wouldn't that also be a good example for a store? - Not sure, but per-open might need a bit more explanation - io_write() in memfile seems wrong to me. I don't think a write should ever truncate the buffer. It only should grow if offs+data_len > content_len, but otherwise keep the old size. - Is the memmove in io_write really necessary? I'm not a VM expert, but my feeling is that there should be a way to extend the buffer without physically copying bits... - I don't think it's really necessary to wrap the memcpy() with if(data_len) -- memcpy should just be a no-op if len is 0... - Shouldn't offs be advanced by data_len rather than the passed-in amount? Or, if amount is actually supposed to take precedence, shouldn't it also dominate in all the other places?... - "[...]when a program opens a file for writing; the file is truncated to have a length of zero bytes." -> Of course that is only true it O_TRUNC is used... - The only special handling really necassary for the case of truncating to 0, is replacing the mmap() by munmap(). (Sadly, mmap() doesn't seem to deal with 0-size mapping requests in a useful manner, as malloc() does.) I don't see any need for other special handling. I especially don't think the workaround creating a 1-byte buffer is useful -- content should never get dereferenced if content_len is 0. If special handling of NULL content is really necessary in some place, it can still be checked. (In fact, you even *do* check it in some places in io_write() and file_set_size() -- though I don't think that's really necessary... You even check for contents being NULL in the special handling code itself!) - I don't think it's true that memfile_argp can't be static because of being used by trivfs. trivfs only uses trivfs_runtime_argp (which indeed must be global); it doesn't care about the private alias memfile_argp at all. (Actually, why is that alias created at all -- why not just work with trivfs_runtime_argp directly?) - The mention of state->hook only confuses IMHO. As it's not really explained, better not mention it at all. - Shouldn't the buffer resizing in parse_opt() also check for 0-size before mmap()?... - Some of the FAQ entries seem outdated, e.g. about pthreads How about putting this into the wiki? That would make it much easier for others to contribute. If it was already a wiki, I could have fixed some of the things mentioned above, as well various minor issues directly... -antrik- _______________________________________________ Bug-hurd mailing list Bug-hurd@gnu.org http://lists.gnu.org/mailman/listinfo/bug-hurd
