>> 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 :-)
Ok. I sometimes include multiple small logical changes in one commit,
I'll have to get out of that habit.
>> -
>> +
>
> Avoid such gratuitous changes please...
Oops.
>> new file mode 100644
>> index 0000000..1a441ca
>> --- /dev/null
>> +++ b/monitor.c
>> @@ -0,0 +1,86 @@
>> +#include "monitor.h"
> [...]
>
> Missing Copyright/License header.
Ok, I wasn't entirely sure what to put as the xmlfs code I started
with had HurdFR stuff, so I decided to leave it until I learned what
the copyright situation for the xmlfs code is.
>> +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?
Hmm, I definitely need to look up commenting/documentation in the GNU
coding style.
>> + 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()?
Yes, this should 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?
I originally wrote the monitoring code as a standalone program,
intending to extend it. Forgot to remove some of the irrelevant
code/comments, it seems.
>> +{
>> + 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()?
Yes. I don't really use asserts for some reason, so I'll have to get
into the habit.
>> + if (err)
>> + return err;
>
> I think this will also leak bucket and class in the error case?
>
Will fix.
>> + 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...
Argh, argh, argh. Note to self: before committing, grep for "TODO"
>
>> + 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?
I'll definitely need to have a look at that. Almost certainly
(developing seems so much harder with no valgrind to run after every
change :P)
>> - 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?
Yes, I'll add a comment explaining that. Basically, it's because I
need to pass a port to file_notice_changes.
--
Michael Walker (http://www.barrucadu.co.uk)
Arch Hurd Developer; GNU Webmaster; FSF member #8385
http://www.archhurd.org http://www.gnu.org http://www.fsf.org
