scannedonly vfs module ported to samba 3.5

[Volker Lendecke]

Hi, Olivier!

On Sat, Dec 05, 2009 at 08:53:00PM +0100, Olivier Sessink wrote:
> with some help I ported the scannedonly module to the samba 3.5 vfs API:
>
> http://scannedonly.svn.sourceforge.net/viewvc/scannedonly/branches/samba3.5compat/src/vfs_scannedonly.c?revision=61&view=markup
>
> please have a look at the code and let me know if more changes are
> required to include it in the samba source tree.

Cosmetic: Can you please switch your editor to 8-space tab
indentation? You seem to have tabs with 4 spaces, this
messes up indentation quite a bit. Alternatively, convert
the tabs to spaces.

Now some hints to simplify the code. I know we have a huge
runtime lib that is hard to wrap your head around, so take
these not as criticisms but as hints into our code base:

Routines cachefile_name and name_w_ending_slash: I think
talloc_asprintf() would tremendously help here. If I read it
right, cachefile_name would look like

talloc_asprintf(ctx, "%s.scanned:%s", basename, shortname)

This would also take care of checking for talloc_size
failing more easy.

The else branch in name_w_ending_slash() could be simplified
as talloc_asprintf(ctx, "%s/", name);

Similar for path_plus_name().

construct_full_path(): Is it really the case that we still
get "./" as path components here? And, do they really hurt
at this level?

connect_to_scanner: We have the routine open_socket_out() in
lib/util_sock.c that does pretty much what you do here.

For the fcntl(so->socket, F_SETFL, O_NDELAY) we have
set_blocking().

timespec_is_newer: This might be simplified by calling
timespec_compare()

is_scannedonly_file: We tend to use type bool for these
kinds of functions.

scannedonly_allow_access: For the two strcmps, we have ISDOT
and ISDOTDOT which should be faster than calling strcmp.

Now for two real concerns I have, first a simple one:

scannedonly_allow_access: There's a 

                while (dire) {
			...
                } while (dire) ;

loop. Is that correct C?

Then the active_files[1024] array in struct Tscannedonly: My
impression is that these are used to notify the scanner at
close time. Is that right? If so, you can't restrict that to
1024, we have to deal with many more file descriptors. Then,
with your "memset(so->activefiles, 0, 1024);" you don't
initialize the whole array if that was your intention.
1024*sizeof(char *) would have been right I think.

If my assumption about its purpose (use it at close time) is
right, then you don't need the array at all: We have the
fsp->fsp_name available in the vfs_close call that you might
want to access.

> one thing: how does the samba team handle copyright? is all copyright
> transferred to a foundation, or do individual contributers keep their
> own copyright, or is there another way?

You can keep your own copyright. Our only restriction is
that it must be personal and not corporate.

Volker
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20091205/e53e5c46/attachment.pgp>