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

[Michael Walker]

>>  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