bug-hurd
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: PATCH 2/2 - monitor backing store changes and update.


From: olafBuddenhagen
Subject: Re: PATCH 2/2 - monitor backing store changes and update.
Date: Wed, 6 Apr 2011 03:55:26 +0200
User-agent: Mutt/1.5.20 (2009-06-14)

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-



reply via email to

[Prev in Thread] Current Thread [Next in Thread]