[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Updated HHG
Re: Updated HHG
Mon, 29 Oct 2007 15:16:41 +0100
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
- 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
- Memfile is a nice idea. Wouldn't that also be a good example for a
- 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
- 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...
- Re: Updated HHG,