Re: Updated HHG

[olafBuddenhagen]

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-