Hi,
On Mon, Apr 04, 2011 at 01:37:12PM +0100, Michael Walker wrote:
> The below patch monitors the backing store for changes (if it's a
> separate file rather than the underlying node) and updates the
> presented directory hierarchy when a change is detected.
I'm not familiar with the notification interface nor with xmlfs; so this
review will mostly be my usual round of nitpicks... Hope that won't
discourage you :-)
> all: $(OBJS)
> - $(CC) -o $(BINARY) $(OBJS) $(LDFLAGS)
> + @$(CC) -o $(BINARY) $(OBJS) $(LDFLAGS)
>
> .c.o:
> - $(COMPILE) -c $<
> + @$(COMPILE) -c $<
>
> clean:
> - rm -f *.o core *.obj *~ $(BINARY)
> + @rm -f *.o core *.obj *~ $(BINARY) fs_notify.c
This change is not related to the purpose of the patch... Please put it
in an extra patch :-)
> -
> +
Avoid such gratuitous changes please...
> new file mode 100644
> index 0000000..1a441ca
> --- /dev/null
> +++ b/monitor.c
> @@ -0,0 +1,86 @@
> +#include "monitor.h"
[...]
Missing Copyright/License header.
> +int
> +notice_change (mach_msg_header_t *inp, mach_msg_header_t *outp)
> +{
IIRC most Hurd code has a copy of the function documentation in the .c
file... Don't know about the existing xmlfs code though?
> + if (handler != NULL)
> + (*handler) (params);
I don't think we should ever get into a situation where the notification
is set up but the handler not? So I guess that should rather be an
assert()?
> +/* Only works with one file for now. TODO: work with multiple files */
> +error_t
> +set_file_monitor (file_t thefile, void (*thehandler) (void*), void
> *theparams)
Err... Why would we need to monitor multiple files in xmlfs?
> +{
> + mach_port_t notify;
> + error_t err;
> + notice_t noticedata;
> + cthread_t noticethread;
> +
> + if (thefile == MACH_PORT_NULL)
> + error (1, 0, "Null file port given\n");
Again, shouldn't that rather be an assert()?
> + if (handler != NULL || params != NULL)
> + DEBUG ("NOTICE: Overwrote previous handler.\n");
Unless I'm missing something, we never actually try to set up the
handler more than once?...
(Otherwise, you would be leaking a lot of stuff I think...)
> + bucket = ports_create_bucket ();
> + class = ports_create_class (NULL, NULL);
Like here for example.
> + if (err)
> + return err;
I think this will also leak bucket and class in the error case?
> + noticethread = cthread_fork (&managefork, NULL);
If this is indeed called multiple times, you would even leak a thread
here I believe?...
> + char filename[1024]; /* Hard filename length limit of 1024, TODO:
> better solution */
Augh, augh, augh! :-)
I think you should fix this before the patch is commited...
> + xmlfile = (file_t) open (xmlfilename, O_READ);
> +
> + err = xmlfs_create (xmlfile, xmlfs);
I believe this will leak a lot of stuff on each file change?
> - mach_port_t bootstrap, underlying_node;
> + mach_port_t bootstrap, underlying_node, xmlport;
I'm somewhat confused by the xmlport/xmlfile duality... Perhaps you
could add a comment explaining it?
> - file_t xmlfile;
> error_t err;
> + file_t xmlfile;
Gratuitous change...
Apart from the few nitpicks, you code seems very clean :-)
-antrik-
